test_runner: support glob matching coverage files #53553

OnlyAviv · 2024-06-22T23:45:30Z

This Pull Request introduces two new command-line interface flags to Node.js:

--test-coverage-include: Allows users to specify glob patterns to include specific files in the coverage report.
--test-coverage-exclude: Allows users to specify glob patterns to exclude specific files from the coverage report.

Notable change text, if any:

Individual file patterns can now be excluded or exclusively included in coverage reports:

- To exclude a specific pattern, use `--test-coverage-exclude`. For example:
  ```bash
  node --experimental-test-coverage --test-coverage-exclude=**/*.test.js .
  ```
  This command excludes all files ending with `.test.js` from the coverage report.

- To include only files that match a specific pattern, use `--test-coverage-include`. For example:
  ```bash
  node --experimental-test-coverage --test-coverage-include=src/**/*.js .
  ```
  This command includes only `.js` files located in the `src` directory and its subdirectories in the coverage report.

Both options can be specified multiple times to match multiple glob patterns. If both options are used in unison, files will need to abide by both guidelines.

nodejs-github-bot · 2024-06-22T23:45:35Z

Review requested:

@nodejs/test_runner

OnlyAviv · 2024-06-22T23:45:45Z

Blocked by #52881 - This PR relies on the path.matchesGlob method to match the coverage files.

(Test failures will disappear once the blocking PR lands)

If a PR relies on a semver-minor change, does it become semver-minor?

doc/api/cli.md

test/parallel/test-runner-coverage.js

lib/internal/test_runner/coverage.js

test/parallel/test-runner-coverage.js

MoLow · 2024-06-23T06:09:56Z

please take a look at the test failures

OnlyAviv · 2024-06-23T11:26:28Z

Thanks! I'll made those changes!

I've also unblocked the PR because the blocking PR has landed

MoLow

LGTM

test/parallel/test-runner-coverage.js

rozzilla

Nice one! ❤️
About the run option, will those new options be supported?
Ideally, as we have files option, it'd be nice to be able to pass the coverage exclusion files too.

OnlyAviv · 2024-06-24T11:46:15Z

Nice one! ❤️

About the run option, will those new options be supported?

Ideally, as we have files option, it'd be nice to be able to pass the coverage exclusion files too.

I'm not planning on that, this is specific to coverage, not all of testing.

cjihrig

About the run option, will those new options be supported?

We should probably make all coverage options available via run() prior to moving this out of experimental status.

src/node_options.cc

doc/api/cli.md

lib/internal/test_runner/coverage.js

cjihrig · 2024-06-24T12:52:00Z

lib/internal/test_runner/coverage.js

+  for (let i = 0; i < excludeFileGlobs.length; ++i) {
+    const absolutePath = fileURLToPath(url);
+    const relativePath = relative(workingDirectory, absolutePath);
+    if (matchesGlob(relativePath, excludeFileGlobs[i]) || matchesGlob(absolutePath, excludeFileGlobs[i])) return true;


It seems pretty inefficient to check the same path as both an absolute path and a relative path. Same comment below for included globs.

What would you recommend doing instead?

I assume users would want relative checks, but if the path is somewhere completely different, they may also want absolute?

So one issue is that you seem to be computing absolutePath and relativePath for each iteration of the loop, even though I believe they only need to be computed once per function call.

The other thing is, since you already have absolute paths, shouldn't it be enough to glob against those? I guess I don't see anything in the tests added by this PR that would require converting url to both an absolute and relative path. This would have a nice side effect of not needing to pass workingDirectory around as well. Do you have an example where that wouldn't be the case?

I can fix the position of the generation, but here is my reasoning for both relative and absolute testing:

var { minimatch } = require("minimatch") var path = require("path"); const cwd = "/home/user" const absTestFile = "/home/user/path/to/file.js" const relTestFile = path.relative(cwd, absTestFile); console.log("relTestFile: " + relTestFile + " | Minimatch: " + minimatch(relTestFile, "path/to/*.js")) // true console.log("absTestFile: " + absTestFile + " | Minimatch: " + minimatch(absTestFile, "path/to/*.js")) // false

Users probably want to match the relative test file when excluding coverage files.

Ok, fair enough.

OnlyAviv · 2024-06-24T17:20:13Z

I need to review the new test failures, I'll address them when I get a chance

nodejs-github-bot · 2024-07-14T17:52:42Z

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

OnlyAviv · 2024-07-14T20:20:57Z

CI passed 🎉

github-actions · 2024-07-14T22:29:57Z

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @benjamingr.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

nodejs-github-bot · 2024-07-14T22:37:46Z

Landed in 4174b73

PR-URL: #53553 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

aduh95 · 2024-07-16T10:05:28Z

This was merged with the wrong subsystem (it should be test_runner: IIUC)

PR-URL: #53553 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Notable changes: http: * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721 lib: * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752 module: * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166 path: * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881 process: * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239 stream: * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462 test_runner: * support glob matching coverage files (Aviv Keller) #53553 worker: * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682 PR-URL: #53826

PR-URL: nodejs#53553 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Notable changes: http: * (SEMVER-MINOR) expose websockets (Natalia Venditto) nodejs#53721 lib: * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) nodejs#53752 module: * add `__esModule` to `require()`'d ESM (Joyee Cheung) nodejs#52166 path: * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) nodejs#52881 process: * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) nodejs#53239 stream: * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) nodejs#53462 test_runner: * support glob matching coverage files (Aviv Keller) nodejs#53553 worker: * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) nodejs#53682 PR-URL: nodejs#53826

targos · 2024-07-28T06:53:16Z

It looks like the commit message was edited when it landed on v22.x: 3a0fcbb

I'm marking this as backported so it doesn't show up in branch-diff's output.

JakobJingleheimer · 2024-11-25T17:01:37Z

Why should this not land on 20.x?

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Jun 22, 2024

OnlyAviv added blocked PRs that are blocked by other issues or PRs. cli Issues and PRs related to the Node.js command line interface. coverage Issues and PRs related to native coverage support. labels Jun 22, 2024