test_runner: add initial code coverage support #46017

cjihrig · 2022-12-29T21:53:57Z

This commit adds code coverage functionality to the node:test module. When node:test is used in conjunction with the new --test-coverage CLI flag, a coverage report is created when the test runner finishes. The coverage summary is forwarded to any test runner reporters so that the display can be customized as desired. This new functionality is compatible with the existing NODE_V8_COVERAGE environment variable as well.

There are still several limitations, which will be addressed in subsequent pull requests:

Coverage is only reported for a single process. It is possible to merge coverage reports together. Once this is done, the --test flag will be supported as well.
Source maps are not currently supported.
Excluding specific files or directories from the coverage report is not currently supported. Node core modules and node_modules/ are excluded though.

nodejs-github-bot · 2022-12-29T21:54:02Z

Review requested:

@nodejs/test_runner

GeoffreyBooth · 2022-12-29T22:10:38Z

I’m uncomfortable with all these new APIs being added with neither experimental warnings being printed or experimental flags being required. I don’t think it’s enough that they’re marked as experimental in the docs and that the APIs are “opt in” via their own flags or imports. In general we require experimental features to have an --experimental-<feature-name> flag to be enabled at all, so that users are affirming that they’re using a feature that isn’t subject to semver, and we print a warning in addition in case the flag is masked from the user such as via a package.json script command.

JakobJingleheimer · 2022-12-29T23:38:29Z

Huzzah! I'm excited for this to be available!

RE Geoffrey's comment: Things have generally seemed a little more fast and loose recently. I agree experimental features should print a related warning and their flag should be prefixed with the experimental.

That said, how experimental is this though and how far is it from "stable"?

GeoffreyBooth · 2022-12-30T00:28:02Z

That said, how experimental is this though and how far is it from “stable”?

Arguably the test runner overall might be considered fairly close; it’s used internally by Node, so it’s probably what I would consider “release candidate” per #45900. But while it might be considered stable from a “doesn’t have too many bugs” perspective, I think it’s definitely still experimental in that we might easily ship breaking changes to the test runner as we continue to expand its featureset; so a warning would be good to add. The newer parts of the test runner, like the reporters and the code coverage, feel like what I would consider “beta” and arguably the new flags should be --experimental-test-reporter and --experimental-test-coverage.

lib/internal/test_runner/coverage.js

GeoffreyBooth · 2022-12-30T01:49:04Z

To be clear @cjihrig and @MoLow I think we should have a discussion with the full TSC about both #45900 and the experimental status of the various test runner features (or the test runner overall). I’m not going to block any test runner-related PRs for that reason because I think the status question should be a group decision rather than me using my block like a veto; though we might want to wait on this PR landing before that discussion happens in case the consensus is that this new flag should be --experimental-test-coverage instead of --test-coverage. cc @nodejs/tsc

doc/api/cli.md

vdeturckheim

I love it a lot!
I don't have much context on the experimental warning part so please take this review "as is" ^^

doc/api/test.md

cjihrig · 2022-12-30T21:40:20Z

Regarding a flag and warning - I don't recall anyone asking for a CLI flag when the test runner initially landed, and I think adding one for it now would be counterproductive. A warning was emitted before, and it was removed in ad8442f for the same reasons as the args parser.

That said, how experimental is this though and how far is it from "stable"?

I would like to see the test runner stabilize before Node 20 enters LTS. We have mostly been shipping features, and code coverage is the last big missing feature that I personally care about. I'm sure there will be feature requests and bug fixes in the future, but we've (or at least I have) already been trying to avoid breaking changes. It's also worth noting that test coverage of the test runner itself is already on par with the rest of core, and could be even higher if someone dedicated a few hours to low hanging fruit.

doc/api/cli.md

MoLow · 2022-12-31T17:40:12Z

This implementation does LGTM, but without the support for coverage merging & source maps it does feel pretty experimental (in the sense of what value it adds to the existing NODE_V8_COVERAGE)

cjihrig · 2022-12-31T18:39:13Z

without the support for coverage merging & source maps it does feel pretty experimental

These are definitely things that I want/plan to support. However, as a counterpoint, the entire hapi org (which requires very high code coverage) is written in plain JavaScript and currently has no support for getting coverage from child processes. Of course, our CLI runner is based on child processes, so I admit the coverage merging is a bigger deal here. These are also things that are straightforward to incorporate without breaking changes.

benjamingr · 2023-01-01T10:26:12Z

Wouldn't a good compromise be to enable this behavior when --test is passed alongside NODE_V8_COVERAGE which would accomplish this without any more CLI flags or the experimental status debate (as it'd be using two existing options together)?

MoLow · 2023-01-01T10:44:53Z

Wouldn't a good compromise be to enable this behavior when --test is passed alongside NODE_V8_COVERAGE which would accomplish this without any more CLI flags or the experimental status debate (as it'd be using two existing options together)?

NODE_V8_COVERAGE requires specifying a destination for coverage files, but this implementation supports temporal coverage that is just passed to any reporter you use

cjihrig · 2023-01-04T19:55:47Z

@GeoffreyBooth can the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label be removed? I had stopped development on this work because of it.

nodejs-github-bot · 2023-01-07T19:15:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/48889/

GeoffreyBooth · 2023-01-07T21:48:25Z

I think the new flag should be --experimental-test-coverage. There’s a lot left to work out regarding this and I think users should be more obviously aware that they’re using an experimental feature.

nodejs-github-bot · 2023-01-08T02:22:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/48890/

MoLow · 2023-01-26T08:27:25Z

This (and probably most other test runner commits) cannot be backported until #45712 is backported.

see #46360

This commit adds code coverage functionality to the node:test module. When node:test is used in conjunction with the new --test-coverage CLI flag, a coverage report is created when the test runner finishes. The coverage summary is forwarded to any test runner reporters so that the display can be customized as desired. This new functionality is compatible with the existing NODE_V8_COVERAGE environment variable as well. There are still several limitations, which will be addressed in subsequent pull requests: - Coverage is only reported for a single process. It is possible to merge coverage reports together. Once this is done, the --test flag will be supported as well. - Source maps are not currently supported. - Excluding specific files or directories from the coverage report is not currently supported. Node core modules and node_modules/ are excluded though. PR-URL: #46017 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

Add experimental to the name as requested during review. PR-URL: #46017 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

Notable changes: deps: * upgrade npm to 9.5.0 (npm team) #46673 * add ada as a dependency (Yagiz Nizipli) #46410 doc: * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) #46017 lib: * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 src: * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038 * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491 * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888 * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888 * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 test_runner: * add initial code coverage support (Colin Ihrig) #46017 url: * replace url-parser with ada (Yagiz Nizipli) #46410 PR-URL: TODO

Notable changes: deps: * upgrade npm to 9.5.0 (npm team) #46673 * add ada as a dependency (Yagiz Nizipli) #46410 doc: * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) #46017 lib: * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 src: * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038 * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491 * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888 * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888 * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 test_runner: * add initial code coverage support (Colin Ihrig) #46017 url: * replace url-parser with ada (Yagiz Nizipli) #46410 PR-URL: #46725

Notable changes: deps: * upgrade npm to 9.5.0 (npm team) nodejs#46673 * add ada as a dependency (Yagiz Nizipli) nodejs#46410 doc: * add debadree25 to collaborators (Debadree Chatterjee) nodejs#46716 * add deokjinkim to collaborators (Deokjin Kim) nodejs#46444 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) nodejs#46017 lib: * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) nodejs#46494 src: * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) nodejs#45038 * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) nodejs#46583 * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) nodejs#46491 * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) nodejs#45888 * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) nodejs#45888 * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) nodejs#45888 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) nodejs#46368 stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) nodejs#46273 test_runner: * add initial code coverage support (Colin Ihrig) nodejs#46017 url: * replace url-parser with ada (Yagiz Nizipli) nodejs#46410 PR-URL: nodejs#46725

Notable changes: deps: * upgrade npm to 9.5.0 (npm team) #46673 * add ada as a dependency (Yagiz Nizipli) #46410 doc: * add debadree25 to collaborators (Debadree Chatterjee) #46716 * add deokjinkim to collaborators (Deokjin Kim) #46444 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) #46017 lib: * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494 src: * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038 * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583 * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491 * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888 * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888 * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888 * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368 stream: * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273 test_runner: * add initial code coverage support (Colin Ihrig) #46017 url: * replace url-parser with ada (Yagiz Nizipli) #46410 PR-URL: #46725

This commit adds code coverage functionality to the node:test module. When node:test is used in conjunction with the new --test-coverage CLI flag, a coverage report is created when the test runner finishes. The coverage summary is forwarded to any test runner reporters so that the display can be customized as desired. This new functionality is compatible with the existing NODE_V8_COVERAGE environment variable as well. There are still several limitations, which will be addressed in subsequent pull requests: - Coverage is only reported for a single process. It is possible to merge coverage reports together. Once this is done, the --test flag will be supported as well. - Source maps are not currently supported. - Excluding specific files or directories from the coverage report is not currently supported. Node core modules and node_modules/ are excluded though. PR-URL: nodejs#46017 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

Add experimental to the name as requested during review. PR-URL: nodejs#46017 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

This commit adds code coverage functionality to the node:test module. When node:test is used in conjunction with the new --test-coverage CLI flag, a coverage report is created when the test runner finishes. The coverage summary is forwarded to any test runner reporters so that the display can be customized as desired. This new functionality is compatible with the existing NODE_V8_COVERAGE environment variable as well. There are still several limitations, which will be addressed in subsequent pull requests: - Coverage is only reported for a single process. It is possible to merge coverage reports together. Once this is done, the --test flag will be supported as well. - Source maps are not currently supported. - Excluding specific files or directories from the coverage report is not currently supported. Node core modules and node_modules/ are excluded though. PR-URL: nodejs#46017 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

Add experimental to the name as requested during review. PR-URL: nodejs#46017 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

This commit adds code coverage functionality to the node:test module. When node:test is used in conjunction with the new --test-coverage CLI flag, a coverage report is created when the test runner finishes. The coverage summary is forwarded to any test runner reporters so that the display can be customized as desired. This new functionality is compatible with the existing NODE_V8_COVERAGE environment variable as well. There are still several limitations, which will be addressed in subsequent pull requests: - Coverage is only reported for a single process. It is possible to merge coverage reports together. Once this is done, the --test flag will be supported as well. - Source maps are not currently supported. - Excluding specific files or directories from the coverage report is not currently supported. Node core modules and node_modules/ are excluded though. PR-URL: #46017 Backport-PR-URL: #46839 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

Add experimental to the name as requested during review. PR-URL: #46017 Backport-PR-URL: #46839 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

Notable Changes: buffer: * (SEMVER-MINOR) add isAscii method (Yagiz Nizipli) #46046 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) #46017 fs: * (SEMVER-MINOR) add statfs() functions (Colin Ihrig) #46358 src,lib: * (SEMVER-MINOR) add constrainedMemory API for process (theanarkh) #46218 test_runner: * add initial code coverage support (Colin Ihrig) #46017 * (SEMVER-MINOR) add reporters (Moshe Atlow) #45712 v8: * (SEMVER-MINOR) support gc profile (theanarkh) #46255 vm: * (SEMVER-MINOR) expose cachedDataRejected for vm.compileFunction (Anna Henningsen) #46320 PR-URL: #46920

This commit adds code coverage functionality to the node:test module. When node:test is used in conjunction with the new --test-coverage CLI flag, a coverage report is created when the test runner finishes. The coverage summary is forwarded to any test runner reporters so that the display can be customized as desired. This new functionality is compatible with the existing NODE_V8_COVERAGE environment variable as well. There are still several limitations, which will be addressed in subsequent pull requests: - Coverage is only reported for a single process. It is possible to merge coverage reports together. Once this is done, the --test flag will be supported as well. - Source maps are not currently supported. - Excluding specific files or directories from the coverage report is not currently supported. Node core modules and node_modules/ are excluded though. PR-URL: #46017 Backport-PR-URL: #46839 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

Add experimental to the name as requested during review. PR-URL: #46017 Backport-PR-URL: #46839 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

Notable changes: buffer: * (SEMVER-MINOR) add isAscii method (Yagiz Nizipli) #46046 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) #46017 fs: * (SEMVER-MINOR) add statfs() functions (Colin Ihrig) #46358 src,lib: * (SEMVER-MINOR) add constrainedMemory API for process (theanarkh) #46218 test_runner: * add initial code coverage support (Colin Ihrig) #46017 * (SEMVER-MINOR) add reporters (Moshe Atlow) #45712 v8: * (SEMVER-MINOR) support gc profile (theanarkh) #46255 vm: * (SEMVER-MINOR) expose cachedDataRejected for vm.compileFunction (Anna Henningsen) #46320 PR-URL: https://github.com/nodejs/node/pull/46920/commits

Notable changes: buffer: * (SEMVER-MINOR) add isAscii method (Yagiz Nizipli) #46046 doc,lib,src,test: * rename --test-coverage (Colin Ihrig) #46017 fs: * (SEMVER-MINOR) add statfs() functions (Colin Ihrig) #46358 src,lib: * (SEMVER-MINOR) add constrainedMemory API for process (theanarkh) #46218 test_runner: * add initial code coverage support (Colin Ihrig) #46017 * (SEMVER-MINOR) add reporters (Moshe Atlow) #45712 v8: * (SEMVER-MINOR) support gc profile (theanarkh) #46255 vm: * (SEMVER-MINOR) expose cachedDataRejected for vm.compileFunction (Anna Henningsen) #46320 PR-URL: #46920

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Dec 29, 2022

GeoffreyBooth added the test_runner label Dec 30, 2022

aduh95 reviewed Dec 30, 2022

View reviewed changes

lib/internal/test_runner/coverage.js Outdated Show resolved Hide resolved

GeoffreyBooth added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Dec 30, 2022

vdeturckheim reviewed Dec 30, 2022

View reviewed changes

doc/api/cli.md Outdated Show resolved Hide resolved

vdeturckheim reviewed Dec 30, 2022

View reviewed changes

doc/api/test.md Show resolved Hide resolved

tniessen reviewed Dec 31, 2022

View reviewed changes

doc/api/cli.md Outdated Show resolved Hide resolved

MoLow approved these changes Dec 31, 2022

View reviewed changes

cjihrig marked this pull request as draft December 31, 2022 18:57

mhdawson mentioned this pull request Jan 2, 2023

Node.js Technical Steering Committee (TSC) Meeting 2023-01-04 nodejs/TSC#1323

Closed

GeoffreyBooth removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Jan 4, 2023

cjihrig force-pushed the coverage branch from 59a4ee0 to 171cb6b Compare January 7, 2023 19:09

cjihrig marked this pull request as ready for review January 7, 2023 19:14

cjihrig force-pushed the coverage branch from 171cb6b to 87018c4 Compare January 8, 2023 02:18

cjihrig force-pushed the coverage branch from 87018c4 to f84d89d Compare January 8, 2023 02:23

cjihrig removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. backport-requested-v19.x labels Feb 2, 2023

MylesBorins mentioned this pull request Feb 19, 2023

v19.7.0 proposal #46725

Merged

This comment was marked as resolved.

Sign in to view

MoLow added the backport-open-v18.x Indicate that the PR has an open backport. label Feb 25, 2023

MoLow mentioned this pull request Mar 3, 2023

V18.15.0 proposal #46920

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_runner: add initial code coverage support #46017

test_runner: add initial code coverage support #46017

cjihrig commented Dec 29, 2022 •

edited

nodejs-github-bot commented Dec 29, 2022

GeoffreyBooth commented Dec 29, 2022

JakobJingleheimer commented Dec 29, 2022

GeoffreyBooth commented Dec 30, 2022

GeoffreyBooth commented Dec 30, 2022 •

edited

vdeturckheim left a comment

cjihrig commented Dec 30, 2022 •

edited

MoLow commented Dec 31, 2022 •

edited

cjihrig commented Dec 31, 2022 •

edited

benjamingr commented Jan 1, 2023

MoLow commented Jan 1, 2023

cjihrig commented Jan 4, 2023

nodejs-github-bot commented Jan 7, 2023

GeoffreyBooth commented Jan 7, 2023

nodejs-github-bot commented Jan 8, 2023

MoLow commented Jan 26, 2023

This comment was marked as resolved.

test_runner: add initial code coverage support #46017

test_runner: add initial code coverage support #46017

Conversation

cjihrig commented Dec 29, 2022 • edited

nodejs-github-bot commented Dec 29, 2022

GeoffreyBooth commented Dec 29, 2022

JakobJingleheimer commented Dec 29, 2022

GeoffreyBooth commented Dec 30, 2022

GeoffreyBooth commented Dec 30, 2022 • edited

vdeturckheim left a comment

Choose a reason for hiding this comment

cjihrig commented Dec 30, 2022 • edited

MoLow commented Dec 31, 2022 • edited

cjihrig commented Dec 31, 2022 • edited

benjamingr commented Jan 1, 2023

MoLow commented Jan 1, 2023

cjihrig commented Jan 4, 2023

nodejs-github-bot commented Jan 7, 2023

GeoffreyBooth commented Jan 7, 2023

nodejs-github-bot commented Jan 8, 2023

MoLow commented Jan 26, 2023

This comment was marked as resolved.

cjihrig commented Dec 29, 2022 •

edited

GeoffreyBooth commented Dec 30, 2022 •

edited

cjihrig commented Dec 30, 2022 •

edited

MoLow commented Dec 31, 2022 •

edited

cjihrig commented Dec 31, 2022 •

edited