Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test_runner: add coverage support to run function #53937

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

atlowChemi
Copy link
Member

@atlowChemi atlowChemi commented Jul 19, 2024

Fixes: #53867
Refs: #53924

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Jul 19, 2024
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple comments regarding a possible longer term strategy, but this is looking good. Thank you for picking this up!

lib/internal/test_runner/harness.js Outdated Show resolved Hide resolved
lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved
lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
@atlowChemi atlowChemi force-pushed the coverage-support-via-run branch 3 times, most recently from 2907841 to 7ef36f2 Compare July 29, 2024 18:35
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these changes are moving in the right direction, but I think this should be split up into multiple PRs.

lib/internal/main/test_runner.js Outdated Show resolved Hide resolved
lib/internal/test_runner/harness.js Outdated Show resolved Hide resolved
lib/internal/test_runner/harness.js Outdated Show resolved Hide resolved
lib/internal/test_runner/harness.js Outdated Show resolved Hide resolved
lib/internal/test_runner/harness.js Outdated Show resolved Hide resolved
lib/internal/test_runner/test.js Outdated Show resolved Hide resolved
lib/internal/test_runner/test.js Outdated Show resolved Hide resolved
lib/internal/test_runner/test.js Outdated Show resolved Hide resolved
test/parallel/test-runner-run.mjs Outdated Show resolved Hide resolved
lib/internal/test_runner/test.js Outdated Show resolved Hide resolved
@cjihrig
Copy link
Contributor

cjihrig commented Aug 22, 2024

@atlowChemi just a heads up if you come back to this PR - there will likely be new flags to account for from #54429. A lot of internal refactoring also happened, so the diff here should be a lot simpler.

@RedYetiDev RedYetiDev added wip Issues and PRs that are still a work in progress. coverage Issues and PRs related to native coverage support. labels Aug 22, 2024
@atlowChemi
Copy link
Member Author

@atlowChemi just a heads up if you come back to this PR - there will likely be new flags to account for from #54429. A lot of internal refactoring also happened, so the diff here should be a lot simpler.

@cjihrig Thanks for the heads up!

I am trying yo get back to this, been pretty busy in work & personal life lately, hoping to get back to this soon 🙏🏽

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than docs, this looks pretty much done!

@atlowChemi

This comment was marked as resolved.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 30, 2024

I have some issues with the tests

Do @RedYetiDev's comments make a difference? If they still fail with those changes, I can take a look.

@atlowChemi
Copy link
Member Author

I have some issues with the tests

Do @RedYetiDev's comments make a difference? If they still fail with those changes, I can take a look.

They do! Thanks @RedYetiDev 🙂
I actually noticed it just after I wrote here seeking for help, but took some time to get to it..

@atlowChemi atlowChemi marked this pull request as ready for review September 5, 2024 05:03
Comment on lines 547 to 549
lineCoverage,
branchCoverage,
functionCoverage,
Copy link
Member

@jasnell jasnell Sep 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
lineCoverage,
branchCoverage,
functionCoverage,
lineCoverage = 0,
branchCoverage = 0,
functionCoverage = 0,

lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved
lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved
@atlowChemi
Copy link
Member Author

@atlowChemi I don't think this should land until #54851, given that the coverage thresholds are currently modifying the exit code

@RedYetiDev I don't think that this PR should depend on #54851 as it is semver-major, and we can make such changes later to avoid backporting issues.
@cjihrig WDYT?

@cjihrig
Copy link
Contributor

cjihrig commented Sep 14, 2024

We don't need to wait on #54851. It's OK to change the process exit code for now. We'll have to change that behavior in a semver major anyway.

@atlowChemi atlowChemi added semver-minor PRs that contain new features and should be released in the next minor version. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 14, 2024
@RedYetiDev RedYetiDev removed their request for review September 14, 2024 20:38
@RedYetiDev
Copy link
Member

RedYetiDev commented Sep 14, 2024

🎉 Looks good imo!

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 14, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@RedYetiDev RedYetiDev removed the wip Issues and PRs that are still a work in progress. label Sep 17, 2024
@RedYetiDev
Copy link
Member

RedYetiDev commented Sep 18, 2024

@atlowChemi

https://ci.nodejs.org/job/node-test-binary-windows-js-suites/30322/RUN_SUBSET=3,nodes=win2019-COMPILED_BY-vs2022/testReport/junit/(root)/parallel/test_runner_run_coverage/ seems related:

00:02:15.600 not ok 686 parallel/test-runner-run-coverage
00:02:15.600   ---
00:02:15.600   duration_ms: 4266.02000
00:02:15.600   severity: fail
00:02:15.600   exitcode: 1
00:02:15.600   stack: |-
00:02:15.600     â–¶ require('node:test').run coverage settings
00:02:15.601       â–¶ validation
00:02:15.601         ✔ should only allow boolean in options.coverage (63.9542ms)
00:02:15.601         ✔ should only allow string|string[] in options.coverageExcludeGlobs (60.1064ms)
00:02:15.601         ✔ should only allow string|string[] in options.coverageIncludeGlobs (54.7812ms)
00:02:15.601         ✔ should only allow an int within range in options.lineCoverage (51.6314ms)
00:02:15.601         ✔ should only allow an int within range in options.branchCoverage (47.7735ms)
00:02:15.601         ✔ should only allow an int within range in options.functionCoverage (45.1432ms)
00:02:15.601       â–¶ validation (66.871ms)
00:02:15.601       â–¶ run with coverage
00:02:15.601         ✔ should run with coverage (868.6549ms)
00:02:15.601         ✔ should run with coverage and exclude by glob (1350.5099ms)
00:02:15.601         ✖ should run with coverage and include by glob (600.2586ms)
00:02:15.602           AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
00:02:15.602           
00:02:15.602           false !== true
00:02:15.602           
00:02:15.602               at TestsStream.<anonymous> (file:///C:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-runner-run-coverage.mjs:131:16)
00:02:15.602               at TestsStream.<anonymous> (C:\workspace\node-test-binary-windows-js-suites\node\test\common\index.js:493:15)
00:02:15.602               at TestsStream.emit (node:events:519:28)
00:02:15.602               at [kEmitMessage] (node:internal/test_runner/tests_stream:140:10)
00:02:15.602               at TestsStream.coverage (node:internal/test_runner/tests_stream:127:23)
00:02:15.602               at Test.postRun (node:internal/test_runner/test:1075:18)
00:02:15.602               at postRun (node:internal/test_runner/runner:700:28)
00:02:15.602               at process.processTicksAndRejections (node:internal/process/task_queues:105:5) {
00:02:15.602             generatedMessage: true,
00:02:15.602             code: 'ERR_ASSERTION',
00:02:15.603             actual: false,
00:02:15.603             expected: true,
00:02:15.603             operator: 'strictEqual'
00:02:15.603           }
00:02:15.603     
00:02:15.603         ✖ should run while including and excluding globs (412.3843ms)
00:02:15.603           AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
00:02:15.603           
00:02:15.604           false !== true
00:02:15.604           
00:02:15.604               at TestsStream.<anonymous> (file:///C:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-runner-run-coverage.mjs:149:16)
00:02:15.604               at TestsStream.<anonymous> (C:\workspace\node-test-binary-windows-js-suites\node\test\common\index.js:493:15)
00:02:15.604               at TestsStream.emit (node:events:519:28)
00:02:15.604               at [kEmitMessage] (node:internal/test_runner/tests_stream:140:10)
00:02:15.604               at TestsStream.coverage (node:internal/test_runner/tests_stream:127:23)
00:02:15.605               at Test.postRun (node:internal/test_runner/test:1075:18)
00:02:15.605               at postRun (node:internal/test_runner/runner:700:28)
00:02:15.605               at process.processTicksAndRejections (node:internal/process/task_queues:105:5) {
00:02:15.605             generatedMessage: true,
00:02:15.605             code: 'ERR_ASSERTION',
00:02:15.605             actual: false,
00:02:15.605             expected: true,
00:02:15.605             operator: 'strictEqual'
00:02:15.606           }
00:02:15.606     
00:02:15.606         ✖ should run with coverage and fail when below line threshold (1.1888ms)
00:02:15.606           AssertionError [ERR_ASSERTION]: Expected "actual" to be strictly unequal to: 1
00:02:15.606               at TestContext.<anonymous> (file:///C:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-runner-run-coverage.mjs:158:14)
00:02:15.606               at Test.runInAsyncScope (node:async_hooks:211:14)
00:02:15.606               at Test.run (node:internal/test_runner/test:930:25)
00:02:15.606               at Suite.processPendingSubtests (node:internal/test_runner/test:629:18)
00:02:15.606               at Test.postRun (node:internal/test_runner/test:1026:19)
00:02:15.606               at Test.run (node:internal/test_runner/test:969:12)
00:02:15.607               at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
00:02:15.607               at async Suite.processPendingSubtests (node:internal/test_runner/test:629:7) {
00:02:15.607             generatedMessage: true,
00:02:15.607             code: 'ERR_ASSERTION',
00:02:15.607             actual: 1,
00:02:15.607             expected: 1,
00:02:15.607             operator: 'notStrictEqual'
00:02:15.608           }
00:02:15.608     
00:02:15.608       â–¶ run with coverage (3266.002ms)
00:02:15.608     â–¶ require('node:test').run coverage settings (3268.4502ms)
00:02:15.608     ℹ tests 11
00:02:15.608     ℹ suites 3
00:02:15.608     ℹ pass 8
00:02:15.608     ℹ fail 3
00:02:15.608     ℹ cancelled 0
00:02:15.608     ℹ skipped 0
00:02:15.609     ℹ todo 0
00:02:15.609     ℹ duration_ms 3288.5163
00:02:15.609     
00:02:15.609     ✖ failing tests:
00:02:15.609     
00:02:15.609     test at test\parallel\test-runner-run-coverage.mjs:121:11
00:02:15.609     ✖ should run with coverage and include by glob (600.2586ms)
00:02:15.609       AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
00:02:15.609       
00:02:15.609       false !== true
00:02:15.609       
00:02:15.610           at TestsStream.<anonymous> (file:///C:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-runner-run-coverage.mjs:131:16)
00:02:15.610           at TestsStream.<anonymous> (C:\workspace\node-test-binary-windows-js-suites\node\test\common\index.js:493:15)
00:02:15.610           at TestsStream.emit (node:events:519:28)
00:02:15.610           at [kEmitMessage] (node:internal/test_runner/tests_stream:140:10)
00:02:15.610           at TestsStream.coverage (node:internal/test_runner/tests_stream:127:23)
00:02:15.610           at Test.postRun (node:internal/test_runner/test:1075:18)
00:02:15.610           at postRun (node:internal/test_runner/runner:700:28)
00:02:15.610           at process.processTicksAndRejections (node:internal/process/task_queues:105:5) {
00:02:15.610         generatedMessage: true,
00:02:15.610         code: 'ERR_ASSERTION',
00:02:15.610         actual: false,
00:02:15.610         expected: true,
00:02:15.611         operator: 'strictEqual'
00:02:15.611       }
00:02:15.611     
00:02:15.611     test at test\parallel\test-runner-run-coverage.mjs:137:11
00:02:15.611     ✖ should run while including and excluding globs (412.3843ms)
00:02:15.611       AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
00:02:15.611       
00:02:15.612       false !== true
00:02:15.612       
00:02:15.612           at TestsStream.<anonymous> (file:///C:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-runner-run-coverage.mjs:149:16)
00:02:15.612           at TestsStream.<anonymous> (C:\workspace\node-test-binary-windows-js-suites\node\test\common\index.js:493:15)
00:02:15.612           at TestsStream.emit (node:events:519:28)
00:02:15.612           at [kEmitMessage] (node:internal/test_runner/tests_stream:140:10)
00:02:15.612           at TestsStream.coverage (node:internal/test_runner/tests_stream:127:23)
00:02:15.612           at Test.postRun (node:internal/test_runner/test:1075:18)
00:02:15.612           at postRun (node:internal/test_runner/runner:700:28)
00:02:15.612           at process.processTicksAndRejections (node:internal/process/task_queues:105:5) {
00:02:15.612         generatedMessage: true,
00:02:15.612         code: 'ERR_ASSERTION',
00:02:15.612         actual: false,
00:02:15.612         expected: true,
00:02:15.612         operator: 'strictEqual'
00:02:15.612       }
00:02:15.612     
00:02:15.613     test at test\parallel\test-runner-run-coverage.mjs:155:11
00:02:15.613     ✖ should run with coverage and fail when below line threshold (1.1888ms)
00:02:15.613       AssertionError [ERR_ASSERTION]: Expected "actual" to be strictly unequal to: 1
00:02:15.613           at TestContext.<anonymous> (file:///C:/workspace/node-test-binary-windows-js-suites/node/test/parallel/test-runner-run-coverage.mjs:158:14)
00:02:15.613           at Test.runInAsyncScope (node:async_hooks:211:14)
00:02:15.613           at Test.run (node:internal/test_runner/test:930:25)
00:02:15.613           at Suite.processPendingSubtests (node:internal/test_runner/test:629:18)
00:02:15.613           at Test.postRun (node:internal/test_runner/test:1026:19)
00:02:15.613           at Test.run (node:internal/test_runner/test:969:12)
00:02:15.613           at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
00:02:15.613           at async Suite.processPendingSubtests (node:internal/test_runner/test:629:7) {
00:02:15.613         generatedMessage: true,
00:02:15.613         code: 'ERR_ASSERTION',
00:02:15.614         actual: 1,
00:02:15.614         expected: 1,
00:02:15.614         operator: 'notStrictEqual'
00:02:15.614       }
00:02:15.614     (node:4360) ExperimentalWarning: glob is an experimental feature and might change at any time
00:02:15.615     (Use `node --trace-warnings ...` to show where the warning was created)

@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 20, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 20, 2024
@nodejs-github-bot
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coverage Issues and PRs related to native coverage support. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_runner: do not read from process.argv and process.cwd() in run()
9 participants