Blitzy: test_runner: bind built-in coverage exclusions to cwd-relative paths (fixes nodejs/node#58654) - #1
Open
blitzy[bot] wants to merge 7 commits into
Open
Conversation
Add a coverage-relative-exclusion fixture directory for exercising the test runner's coverage exclusion rules against working-directory-relative paths. It is deliberately placed so that its absolute path contains a directory named test while none of its own relative paths match the default test file pattern, which is the shape that a project stored below such a directory has. logic-file.js mirrors the logic-file.js of the coverage-default-exclusion fixture so that it measures identically, reporting 66.67% line, 100.00% branch and 50.00% function coverage with lines 5-7 uncovered. node_modules/dependency.js is a dependency that must never show up in a coverage report. It covers the case of a project's own top level node_modules directory, whose relative path has no leading separator. Refs: nodejs#58654
Add the test file of the coverage-relative-exclusion fixture, which backs the regression coverage for the built-in coverage exclusion filter being matched against absolute paths instead of working directory relative ones. A single coverage run over this fixture exercises all three exclusion rules at once: logic-file.js is included, this file is excluded by the default test file pattern through its relative path, and node_modules/dependency.js is excluded by the node_modules rule through its relative path. Importing the nested dependency is what makes one run cover all three. The fixture deliberately lives outside test/.tmp.*, because matchGlobPattern does not pass the dot option and so ** cannot traverse a dot-prefixed segment. A fixture staged below the temporary directory would be masked, and its regression test would then pass both before and after the fix. Refs: nodejs#58654
The built-in default test-file exclusion glob and the built-in node_modules filter in TestCoverage.prototype.shouldSkipFileCoverage() were evaluated against absolute paths, so any project stored below a directory named test or node_modules had its entire coverage report silently emptied and its --test-coverage-lines/-branches/-functions gates satisfied vacuously. Add three cases to the default exclusion suite: - an ancestor directory named test must not exclude the project. This case deliberately runs in place at the fixture directory, which sits below the repository's own test/ directory, because the temporary directory is dot prefixed (test/.tmp.N) and ** cannot traverse a dot prefixed segment without the dot option, which would make the case inert. - an ancestor directory named node_modules must not exclude the project. - a user supplied absolute --test-coverage-exclude glob must still be honoured, guarding the documented contract that user globs match both absolute and relative paths. Stage the shared fixture twice below tmpdir's node_modules so the three pre-existing cases, which run from tmpdir.path, cannot discover the new test file: discovery skips node_modules by directory name, so their asserted report tables stay byte identical. Refs: nodejs#58654
…paths TestCoverage.prototype.shouldSkipFileCoverage() made two path decisions against absolute paths, so a directory name anywhere above the project could silently empty the whole coverage report. The exclude loop applied every entry of coverageExcludeGlobs to the absolute path as well as the working directory relative path. That array also carries the framework's own default pattern, which was authored to be resolved relative to the working directory: it is the very same pattern that test discovery hands to the glob walker with an explicit cwd. Matched against an absolute path, its 'test/**/*' alternative combined with the leading '**/' reaches any ancestor directory named 'test', so every source file of a project stored at, say, /home/test/my-project was classified as a test file and dropped. The terminal node_modules filter had the same defect in a different form: it was a substring test on the absolute file: URL, so any project stored below an ancestor node_modules directory was excluded entirely. Both decisions now use the working directory relative path. The built-in default is discriminated by identity against the exported kDefaultPattern, so user supplied globs keep matching both absolute and relative paths as documented, and the default pattern's text is left untouched because test discovery shares the constant. The node_modules check uses two path.sep derived constants: a prefix for the project's own top-level dependency directory, whose relative path has no leading separator, and a segment for nested and out-of-tree ones. This mirrors discovery, which has always skipped node_modules by directory name. Every exclusion the default is meant to perform still fires through the relative arm, and the failure was silent because toPercentage() maps a 0/0 ratio to 100, which also let --test-coverage-lines, --test-coverage-branches and --test-coverage-functions gates pass vacuously on an empty report. Refs: nodejs#58654
The regression case for the working directory relative node_modules exclusion ran with the staged project itself as the working directory, so the fixture's own dependency was always relative as node_modules/dependency.js. That exercised the top level check only: an implementation which inspected just the start of the relative path still satisfied the case, while a nested dependency such as packages/a/node_modules/x.js could leak into the report unnoticed. Run the case from the ancestor node_modules directory and name the staged project explicitly instead, so the dependency is relative as project/node_modules/dependency.js and the interior segment check is the only thing that can exclude it. The expected report becomes the grouped tree the renderer produces for that layout, and the assertion stays a comparison of the whole table. Refs: nodejs#58654
shouldSkipFileCoverage() assumed that path.relative() always yields a path relative to the working directory. On Windows it cannot express a file that lives on another volume that way and returns an absolute path instead, so the built-in default test-file pattern and the built-in node_modules rule were still matched against an absolute path for cross-drive and cross-UNC-server files. An ancestor directory named test or node_modules on that volume then excluded every measured file again, emptying the report and silently satisfying the --test-coverage-lines, --test-coverage-branches and --test-coverage-functions gates. Only apply the two built-in exclusions when the derived path really is relative to the working directory. User supplied globs are unchanged and keep matching absolute paths as documented, so an explicit --test-coverage-exclude still excludes such a file. Refs: nodejs#58654
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a silent path-domain logic error in the test runner's coverage exclusion filter.
shouldSkipFileCoverage()matched the framework's own default test-file glob and itsnode_modulesfilter against absolute paths. Because that pattern is prefixed with**/, itstest/**/*alternative anchored at any depth — so every source file of any project stored below a directory namedtest(ornode_modules) was misclassified as a test file and dropped from the coverage report.The report still printed, tests still passed, the process still exited
0, and nothing hitstderr. Worse,toPercentage()renders0/0as100, so--test-coverage-linesCI gates passed vacuously — a correctness control defeated by nothing but a directory name.~/test/, a CI workspace or Unix account namedtest, and any vendored package all trigger it.Fixes: nodejs#58654 (open,
confirmed-bug)Root causes
matchGlobPattern(absolutePath, kDefaultPattern)— a cwd-relative pattern applied to an absolute pathStringPrototypeIncludes(url, '/node_modules/')— unscoped substring test on the absolute file URLApproach
The built-in default now decides solely on the cwd-relative path, discriminated by strict reference identity against the exported
kDefaultPattern. User globs keep dual relative+absolute matching — the documented contract (doc/api/cli.md:2642-2643, PR nodejs#53553). The pattern's text is never altered, since the same constant is the test-discovery glob. This is deliberately not PR nodejs#62362's approach, which strips absolute matching from both loops and breaks that contract.Changes — 5 files, +171/−8
lib/internal/test_runner/coverage.js— addsep/isAbsolute; importkDefaultPattern; addkNodeModulesPrefix/kNodeModulesSegment; gate the absolute arm behindexcludeGlob !== kDefaultPattern; move thenode_modulestest ontorelativePath. Includes a Windows cross-volumeisRelativeToCwdguard (fail-safe).test/parallel/test-runner-coverage-default-exclusion.mjs— 3 regression cases (ancestortest· ancestornode_modules· absolute-user-glob contract guard) + fixture staging.test/fixtures/test-runner/coverage-relative-exclusion/.No changes to
deps/,src/, docs, build config, CLI flags, exports or dependencies. The include loop,file:guard andtoPercentageare untouched.Validation
test-runner-*, 100/100 snapshots (zero diffs), 5,057/5,057 full JS+native, 179/179 cctest, 5/5 tooltest.coverage.jsand rebuilding yieldstests 6 | pass 3 | fail 3with the exact empty-table failure mode — the flaw that made the upstream attempt worthless.1withError: 63.64% line coverage does not meet threshold of 90%.Pre-fix the affected exited0.node_modules_extra/,node_modules.js, ancestortest-utilsstill reported); existing exclusions preserved.make lint-jsEXIT=0.Requires human action
Upstream PR submission, a real CI matrix (native Windows/UNC especially), review, and
semver-patchbackports to v24.x/v25.x — ~34h. Project is 67.6% complete.