Repository navigation
node: keep Node's default test-file coverage exclusion when nub injects its own - #798
Conversation
…ts its own Node applies its default coverage exclusion — kDefaultPattern in lib/internal/test_runner/utils.js — only when no --test-coverage-exclude is set at all. nub injects one to keep its preloaded runtime out of the report, which switched that default off, so `nub --test --experimental-test-coverage` listed the user's own *.test.js files where plain node does not. Both injection sites now emit Node's default pattern alongside the runtime exclude, and suppress it when the user supplied an exclude of their own — which turns the default off for stock node too. The default pattern's TypeScript extensions are unconditional where Node appends them only under --strip-types, because nub transpiles TS across the whole supported band rather than only where that flag is on. Known limit: a grandchild spawned by absolute process.execPath never passes through nub, so its own --test-coverage-exclude cannot suppress the pattern nub puts in NODE_OPTIONS. Restoring the default for the far commoner grandchild that overrides nothing is the deliberate trade.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Node's default coverage exclusion does not exist before v23.5.0, but nub injects the pattern from 22.5.0. On the whole Node 22 line — including nub's own fast-tier floor 22.15 and the current maintenance LTS — this PR makes nub hide the user's test files from a report where plain node shows them. That is the mirror image of the bug being fixed.
Reviewed changes — the full diff at df2ec6ad, plus the surrounding NODE_OPTIONS assembly, the flags.rs version bands, and a differential check of the hardcoded pattern against Node's own source at 14 release tags.
- Node's default pattern is re-stated beside nub's runtime exclude.
NODE_DEFAULT_COVERAGE_EXCLUDE(spawn.rs:2850) is a hardcoded copy of Node'skDefaultPattern, emitted at both injection sites so nub's own--test-coverage-excludeno longer silently switches Node's default off. - The compensation is conditional. New
user_supplied_coverage_exclude(spawn.rs:2866) suppresses the default when the user supplied an exclude on argv or through an inheritedNODE_OPTIONS, matching Node's own all-or-nothing rule. coverage_exclude_glob→coverage_exclude_globs. Return type widensOption<String>→Vec<String>; the argv call site atspawn.rs:1096becomes aforloop. One non-test caller, updated.- New differential integration test.
crates/nub-cli/tests/test_coverage_default_exclusion.rscompares nub's TAP coverage table against the livenodeonPATHrather than a hardcoded expectation.
⚠️ No CI leg can ever exercise coverage parity on the Node 22 band
Every cargo test leg in .github/workflows/ci.yml pins node-version: "24", so the new differential test only ever runs against a Node that has the default. node-matrix.yml:203 does carry "22.16", but that job runs tests/node-matrix/real-apps.sh, not the Rust suite. The version split this PR depends on is therefore structurally invisible to CI — which is why the 22.x direction slipped through.
Technical details
# Coverage-parity behavior is untested off the Node 24 band
## Affected sites
- `.github/workflows/ci.yml` — all `cargo test` legs pin `node-version: "24"`.
- `.github/workflows/node-matrix.yml:203` — the only `22.16` leg runs `real-apps.sh`, not `cargo test`.
## Required outcome
- Coverage-exclusion parity is exercised on at least one pre-23.5 Node and one post-23.5 Node, so the version gate cannot regress in either direction unobserved.
## Suggested approach (optional)
- A `PATH`-pinned sweep is the cheapest form and needs no new CI leg: `AGENTS.md` documents `PATH="$HOME/.nvm/versions/node/vX/bin:$PATH" nub …` for exactly this. A `tests/<feature>/` harness in the style of `tests/pnp/` would make it repeatable.
- Alternatively add a `node-version` axis to whichever `cargo test` leg is cheapest, covering 22.x and 24.x.
## Open questions for the human
- Is Node 22 parity worth a standing CI leg, or is a documented manual sweep the right weight here?ℹ️ Nitpicks
user_supplied_coverage_exclude(spawn.rs:2866) splitsNODE_OPTIONSwithsplit_whitespacerather than the quote-awareflags::node_options_tokens, so a fully-quoted"--test-coverage-exclude=a b"token would be missed.coverage_activejust above does the same, so this matches the file's idiom and no in-repo producer of that shape exists — noting it only so the divergence from the quote-aware splitter is a deliberate choice rather than an oversight.
Claude Opus ｜ 𝕏
| let temp = tempfile::tempdir().unwrap(); | ||
| let project = temp.path().join("project"); | ||
| std::fs::create_dir_all(&project).unwrap(); | ||
| std::fs::write(project.join("logic.js"), "exports.add = (a, b) => a + b;\n").unwrap(); |
There was a problem hiding this comment.
The fixture exercises only the *[._-]test alternative of the four in Node's pattern, so drift in test, test/**/*, or test-* would pass unnoticed — and NODE_DEFAULT_COVERAGE_EXCLUDE is a frozen copy with nothing else rechecking it. Adding test.js, test-foo.js, and test/sub/bar.js to the fixture (all required from the test file so they land in the report) turns this into a real pin on the whole pattern; I confirmed the file sets still match byte-for-byte on Node 24.18 today.
…ave one Node's default test-file coverage exclusion landed in 23.5.0 (commit ea9a675f56, nodejs/node#56060) and was never backported to 22.x, so it does not share a floor with --test-coverage-exclude itself (22.5.0). Re-stating the default pattern on a Node between those floors excluded test files stock node reports — the same parity break as the original defect, in the opposite direction. Add flags::test_coverage_default_exclusion_applied with a 23.5.0 floor and gate both injection sites on it. Band edges verified empirically: 18.19.0, 20.11.0, 22.13.0, 22.15.0, 22.23.2 and 23.4.0 report the test file; 23.5.0, 23.11.0, 24.19.0 and 26.7.0 exclude it. Rewrite the integration tests to diff nub's reported file list against the live host node's rather than a fixed table, so they hold on both sides of the floor, and skip the user-exclude case when the host node predates the flag. The fixture is ESM: a CJS require('node:test') trips an unrelated load-hook defect on the 22.15 tier.
There was a problem hiding this comment.
ℹ️ The 23.5.0 gate is right — I re-verified the floor empirically rather than from docs. Two minor suggestions inline.
Reviewed changes — the delta since df2ec6ad, i.e. commit 2c01e75d, plus an independent check of the version floor it introduces and of which CI legs actually exercise it.
- A second, higher version floor.
MIN_TEST_COVERAGE_DEFAULT_EXCLUSION(23.5.0) andtest_coverage_default_exclusion_appliedland inflags.rs;should_restate_default_coverage_exclude(spawn.rs:2924) pairs it with the existing user-exclude check, and both injection sites now route through it. Below 23.5 only the runtime exclude goes out. - The integration test became a pure differential. The precondition
assert!s are gone — it diffs nub's reported file set against the livenode's and reduces each table to fixture file names, so it asserts parity rather than a fixed expectation and holds on both sides of the 23.5 boundary. - Fixture switched to ESM, with a
--test-coverage-excludesupport probe that skips the user-exclude case below 22.5 instead of panicking there. - Floor verified independently. Running the PR's own
logic.js/logic.test.jsfixture under downloaded tarballs: 18.19.0, 20.11.0, 22.15.0, 22.23.2 (latest 22.x) and 23.4.0 all reportlogic.test.js; 23.5.0 and 24.18.0 exclude it. The gate is exactly right and the fallback was never backported. The probe also behaves — exit 9 on 18.19/20.11, exit 0 on 22.15 — and the TAP table format differs across the band (18/20 unpadded, 22+ padded with# ----separators), which the new parser handles. - Correcting my prior review's CI claim: the Node 22 band is covered.
ci.yml:984is a barecargo testrunning on the matrixmatrix-planassembles atci.yml:128, which unconditionally appends ubuntu × 22.15 / 22.13 / 20.11 / 18.19 to the24legs on every PR. The 22.15 and 22.13 legs are precisely the flag-without-default band and would have gone red ondf2ec6ad. No new CI leg is needed.
Claude Opus ｜ 𝕏
|
Shipped in v0.8.1: https://github.com/nubjs/nub/releases/tag/v0.8.1 |
Node applies its default coverage exclusion (
kDefaultPatterninlib/internal/test_runner/utils.js) only when no--test-coverage-excludeis set. Nub injects one to keep its preloaded runtime out of the report, which switched that default off, sonub --test --experimental-test-coveragelisted the user's own test files where plain node does not.Both injection sites now emit Node's default pattern beside the runtime exclude — but only on 23.5.0+, where Node has such a default (it was never backported to 22.x), and only when the user supplied no exclude of their own.
Known limit: a grandchild spawned by absolute
process.execPathnever passes through Nub, so its own exclude cannot suppress the pattern inNODE_OPTIONS.