test_runner: match coverage globs against cwd-relative paths only - #65940
test_runner: match coverage globs against cwd-relative paths only#65940NAVEENKUMARKR777 wants to merge 1 commit into
Conversation
|
Review requested:
|
|
Welcome to Node.js, and thank you for your first contribution! Before review, please take a moment to read:
Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal. Caution AgentScan found account activity patterns that may be consistent with automation. This is a heuristic, not proof that this pull request was opened by an agent or violates policy. AI-assisted contributions are permitted, but automated tooling must not open pull requests without advance approval, and contributors must personally understand, test, verify, and take responsibility for every submitted change. See the AgentScan analysis, AI use policy, and automation policy for additional context. |
62d4c5e to
e16e12e
Compare
Codecov Reportβ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65940 +/- ##
==========================================
+ Coverage 90.16% 90.18% +0.01%
==========================================
Files 771 771
Lines 265445 265454 +9
Branches 50455 50463 +8
==========================================
+ Hits 239329 239389 +60
+ Misses 17056 17002 -54
- Partials 9060 9063 +3
π New features to boost your workflow:
|
MikeMcC399
left a comment
There was a problem hiding this comment.
This PR is failing linting tests. See the comments in the tests for more detail.
See also Pull requests > Step 6: Test with further details in the linked document section BUILDING > Running tests.
To run the linter, use
make lint/vcbuild lint. It will lint JavaScript, C++, and Markdown files.
e16e12e to
b255954
Compare
|
Thanks for flagging β the `lint-js-and-md` failure was `doc/node.1` being out of date after the `doc/api/cli.md` wording change (the man page is generated from the markdown and checked for staleness in CI). Ran `make node.1` to regenerate it and `make lint-md` locally to confirm it's clean, then amended the commit and force-pushed. |
--test-coverage-exclude and --test-coverage-include (including the
default exclude pattern used to drop test files from coverage
reports) were matched against both the cwd-relative path and the
absolute filesystem path of every candidate file. For a
relative-style glob such as the default
`**/{test,test/**/*,test-*,*[._-]test}.{js,mjs,cjs}`, matching
against the absolute path lets a leading `**` cross into directory
segments that have nothing to do with the project: a container
`WORKDIR` of `/test`, a home directory literally named `test`, a CI
checkout path with a `test` segment, and so on. Any project living
under such a path had every one of its files spuriously match the
default exclude glob and silently vanish from the coverage report,
even though `--experimental-test-coverage` reported 100% coverage of
nothing.
This was previously papered over in Node's own test suite by an
unrelated detail: the absolute-path matcher uses `dot: false`, and
this repository's own test tmp dir (test/.tmp.N) has a dot-prefixed
segment that happens to block the buggy match from crossing it. That
made the bug unobservable through the existing coverage-default-
exclusion fixtures despite them running from a directory nested
under this repo's own `test/` folder, and meant the dotfile-handling
fix in 22e99dc addressed a related but different problem without
touching this one.
Fix this by only matching a glob against the absolute path when the
glob pattern itself is an absolute path (e.g. an explicit
`/abs/path/**` passed to --test-coverage-include). Every
relative-style pattern, including all of the built-in default
exclude patterns, is now evaluated exclusively against each file's
path relative to the current working directory, which is what every
default pattern was actually designed to describe.
The new regression test is deliberately set up under a fresh
directory outside of test/.tmp.N, since that directory's dot-prefixed
segment is exactly what prevented the existing fixtures from
catching this bug.
Fixes: nodejs#58654
Signed-off-by: NAVEENKUMARKR777 <naveenkumarkr555@gmail.com>
b255954 to
1a0896c
Compare
|
Re the Codecov note: the missing/partial lines were the `isAbsolute(pattern)` branch and both `matcher.absolute?.match(...)` call sites β i.e. the "pattern is itself an absolute path" case, which (worth noting) had no test coverage anywhere in the codebase even before this PR. Added two tests exercising it directly, for both `--test-coverage-exclude` and `--test-coverage-include` with an absolute pattern, and verified they actually fail if that branch is broken before pushing. |
Summary
--test-coverage-excludeand--test-coverage-includeβ including the built-in default pattern used to drop test files from coverage reports β were matched against both the cwd-relative path and the absolute filesystem path of every candidate file.For a relative-style glob such as the default
**/{test,test/**/*,test-*,*[._-]test}.{js,mjs,cjs}, matching against the absolute path lets a leading**cross into directory segments that have nothing to do with the project: a containerWORKDIRof/test, a home directory literally namedtest, a CI checkout path with atestsegment, and so on. Any project living under such a path had every one of its files spuriously match the default exclude glob and silently vanish from the coverage report, even though--experimental-test-coveragereported 100% coverage of nothing.This is #58654, open since June 2025 and confirmed by a maintainer. A previous fix attempt (#62362) stalled and auto-closed because it dropped absolute-path matching entirely, which broke the (intentional, if under-justified) ability to pass an absolute
--test-coverage-include/--test-coverage-excludepattern β see the discussion in #53553.Why this wasn't caught by the existing tests
This repo's own coverage-default-exclusion tests run from
test/.tmp.N, i.e. from inside this repository's owntest/directory β which should be exactly the scenario that triggers the bug. It doesn't, purely by coincidence: the absolute-path matcher usesdot: false(added in 22e99dc / #63401, to fix a related-but-different dotfile-matching issue), and the.tmp.Nsegment is dot-prefixed, which blocks the buggy glob from crossing it. That masked the bug in-repo while leaving it fully reproducible for any real project whose path doesn't happen to contain a dotfile segment between the ancestortestdirectory and the source files (i.e. almost everyone hitting this in the wild). The new regression test is deliberately set up under a directory outside oftest/.tmp.Nso it actually exercises the bug β I verified it fails without the fix and passes with it.Fix
Only match a glob against the absolute path when the glob pattern is itself an absolute path (e.g. an explicit
/abs/path/**passed to--test-coverage-include). Every relative-style pattern, including all of the built-in default exclude patterns, is now evaluated exclusively against each file's path relative to the current working directory, which is what those patterns were actually designed to describe. This keeps the ability to use an absolute include/exclude pattern intact while eliminating the accidental ancestor-path leak.doc/api/cli.mdis updated to describe the corrected matching semantics.Test plan
lib/internal/test_runner/coverage.jschange and confirmed the new regression test fails (proving it actually catches the bug), then restored the fix and confirmed it passespython3 tools/test.py parallel/test-runner-coverage-default-exclusion parallel/test-runner-coverage parallel/test-runner-coverage-thresholds parallel/test-runner-coverage-source-map parallel/test-runner-run-coverage parallel/test-config-file parallel/test-runner-flag-propagationβ all passtest-runnerparallel suite (83 files) andtest-runneroutput-snapshot suite (109 files) β all pass, no regressionseslintclean on all changed filesFixes: #58654