test_runner: support glob matching coverage files by avivkeller · Pull Request #53553 · nodejs/node · GitHub

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.

Review requested:

@nodejs/test_runner

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?

please take a look at the test failures

Thanks! I'll made those changes!

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

LGTM

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.

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.

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.

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.

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

Hey, if someone could approve + CI, that would help this move to author-ready. Thanks!

Landed in 4174b73

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

PR-URL: #53553 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

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 <moshe@atlow.co.il> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

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

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.

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

avivkeller 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