diff --git a/.changeset/sturdy-orcas-snooze.md b/.changeset/sturdy-orcas-snooze.md new file mode 100644 index 000000000..d90d983e8 --- /dev/null +++ b/.changeset/sturdy-orcas-snooze.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4173 +--- +**Fixed the coverage gate OOM-crashing on every push to `next`.** The scripts/ coverage-floor check invoked c8's `check-coverage` subcommand, whose handler silently drops the async-merge flag even when it's passed (unlike its `report` sibling, which honors it) — the same OOM class as #4068, but this third script slipped through that fix because adding the flag alone wasn't enough here. Routing the check through `c8 report --check-coverage` instead makes the async-merge flag actually take effect, so coverage now merges incrementally instead of loading every shard's raw data into memory at once and blowing the 8GB CI heap ceiling. (#4172) diff --git a/package.json b/package.json index d59f9b059..a5fed3811 100644 --- a/package.json +++ b/package.json @@ -148,7 +148,7 @@ "test:qa": "node scripts/run-tests.cjs --suite qa", "test:affected": "node scripts/run-affected-tests.cjs", "test:coverage": "c8 --check-coverage --lines 70 --branches 60 --reporter text --include 'gsd-core/bin/lib/**/*.cjs' --exclude 'tests/**' --all node scripts/run-tests.cjs", - "test:coverage:scripts-floor": "c8 check-coverage --lines 55 --include 'scripts/**/*.cjs' --exclude 'tests/**' --all", + "test:coverage:scripts-floor": "c8 report --check-coverage --lines 55 --merge-async --include 'scripts/**/*.cjs' --exclude 'tests/**' --all", "test:coverage:unit": "c8 --reporter text --reporter json-summary --merge-async --include 'gsd-core/bin/lib/**/*.cjs' --exclude 'tests/**' --all node scripts/run-tests.cjs --suite unit && node scripts/check-coverage-gate.cjs", "test:coverage:unit:raw": "c8 --reporter none node scripts/run-tests.cjs --suite unit", "test:coverage:report": "c8 report --reporter text --reporter json-summary --merge-async --include 'gsd-core/bin/lib/**/*.cjs' --exclude 'tests/**' --all && node scripts/check-coverage-gate.cjs", diff --git a/tests/c8-merge-async-flag.test.cjs b/tests/c8-merge-async-flag.test.cjs index 850f425d4..a950664ca 100644 --- a/tests/c8-merge-async-flag.test.cjs +++ b/tests/c8-merge-async-flag.test.cjs @@ -1,24 +1,50 @@ 'use strict'; -// Regression guard for #4068: c8's default (sync) coverage-merge phase loads every -// raw V8 coverage file for the whole run into memory as one array before merging -// (`Report._getMergedProcessCov`), which OOM-crashed `npm run test:coverage:unit` in -// the `release.yml` `finalize` dry-run of 1.12.0 (exit 134, SIGABRT) once the unit -// suite grew to 1785 tests / 15 chunks -- a recurrence of #199, which hit the same -// class at ~466 tests and was "fixed" by raising the heap ceiling instead of the -// merge shape. `--merge-async` switches to `_getMergedProcessCovAsync`, which reads -// and merges one raw file at a time (documented root-cause verification, including a -// real memory-shape repro against this repo's actual pinned c8@11.0.0, lives in +// Regression guard for #4068 and #4172: c8's default (sync) coverage-merge phase +// loads every raw V8 coverage file for the whole run into memory as one array before +// merging (`Report._getMergedProcessCov`), which OOM-crashed `npm run +// test:coverage:unit` in the `release.yml` `finalize` dry-run of 1.12.0 (exit 134, +// SIGABRT) once the unit suite grew to 1785 tests / 15 chunks -- a recurrence of +// #199, which hit the same class at ~466 tests and was "fixed" by raising the heap +// ceiling instead of the merge shape. `--merge-async` switches to +// `_getMergedProcessCovAsync`, which reads and merges one raw file at a time +// (documented root-cause verification, including a real memory-shape repro against +// this repo's actual pinned c8@11.0.0, lives in // .gsd/bug/fix-4068-coverage-merge-oom/10-diagnosis.md). // +// #4068's fix covered test:coverage:unit and test:coverage:report but missed the +// third script reading the same merged coverage/tmp data in the same +// "Coverage gate (merged shards)" test.yml job: test:coverage:scripts-floor. That +// gap OOM-crashed the coverage gate on every push to next starting at 4dfc46b, once +// #4068's own new test file plus other suite growth pushed the un-flagged sync merge +// over the 8192 MB heap ceiling (#4172). +// +// IMPORTANT c8@11.0.0 gotcha (verified against node_modules/c8/lib/commands/ +// check-coverage.js and report.js directly): the `check-coverage` CLI subcommand's +// handler does NOT forward `argv.mergeAsync` into the `Report(...)` constructor -- +// only the `report` subcommand's handler (and the default command, which also calls +// into report.js's outputReport) does. So `c8 check-coverage --merge-async ...` +// silently ignores the flag and still OOMs -- confirmed live on PR #4173's CI run +// after simply adding the flag to the check-coverage invocation did not fix the +// crash. The actual fix routes test:coverage:scripts-floor through the `report` +// subcommand with `--check-coverage` (a boolean flag, not a subcommand) instead of +// the `check-coverage` subcommand directly: `c8 report --check-coverage --lines 55 +// --merge-async --include ...`. This exercises the exact same threshold-checking +// logic (report.js's outputReport calls the same checkCoverages() helper from +// check-coverage.js when --check-coverage is truthy) but through the handler that +// actually wires mergeAsync. +// // This test cannot behaviorally reproduce the OOM itself: `gsd-test` never invokes -// `npm run test:coverage:unit` (it runs `node --test` directly), and a heap-ceiling -// crossover point is not a stable, portable assertion across this repo's OS x Node -// matrix on shared benches (see .gsd/bug/fix-4068-coverage-merge-oom/50-test-matrix.md -// for the full seam analysis). What IS stable and worth guarding: the flag must not -// silently disappear from the two scripts that need it, and must not be added to a -// script whose merge/report phase never runs (a no-op that would misleadingly imply -// coverage here too). +// these npm scripts (it runs `node --test` directly), and a heap-ceiling crossover +// point is not a stable, portable assertion across this repo's OS x Node matrix on +// shared benches (see .gsd/bug/fix-4068-coverage-merge-oom/50-test-matrix.md for the +// full seam analysis). What IS stable and worth guarding: the flag must not silently +// disappear from the scripts that need it, must not be added to a script whose +// merge/report phase never runs (a no-op that would misleadingly imply coverage +// here too), and -- new for #4172 -- test:coverage:scripts-floor must keep routing +// through the `report` subcommand rather than reverting to the `check-coverage` +// subcommand, which would silently re-introduce the OOM despite --merge-async still +// being present in the script string. const { describe, test } = require('node:test'); const assert = require('node:assert/strict'); @@ -27,7 +53,7 @@ const path = require('node:path'); const pkg = require(path.join(__dirname, '..', 'package.json')); const scripts = pkg.scripts; -describe('coverage-merge-async-flag (#4068)', () => { +describe('coverage-merge-async-flag (#4068, #4172)', () => { test('test:coverage:unit carries --merge-async', () => { assert.match( scripts['test:coverage:unit'], @@ -46,8 +72,37 @@ describe('coverage-merge-async-flag (#4068)', () => { ); }); + test('test:coverage:scripts-floor carries --merge-async', () => { + assert.match( + scripts['test:coverage:scripts-floor'], + /(?:^|\s)c8\s.*--merge-async/, + 'test:coverage:scripts-floor (the coverage-gate job scripts/ floor check in ' + + 'test.yml) must pass --merge-async to c8, or it reverts to loading every ' + + 'raw V8 coverage file into memory at once, the same class as #4068 (#4172)' + ); + }); + + test('test:coverage:scripts-floor routes through the report subcommand, not check-coverage', () => { + const script = scripts['test:coverage:scripts-floor']; + assert.match( + script, + /(?:^|\s)c8\s+report\b/, + 'test:coverage:scripts-floor must invoke `c8 report --check-coverage ...`, not ' + + '`c8 check-coverage ...` -- c8@11.0.0\'s check-coverage subcommand handler ' + + 'does not forward --merge-async into the Report constructor (verified against ' + + 'node_modules/c8/lib/commands/check-coverage.js), so that form silently drops ' + + 'the flag and still OOMs even with --merge-async present in the string (#4172)' + ); + assert.match( + script, + /--check-coverage\b/, + 'test:coverage:scripts-floor must pass --check-coverage to `c8 report` so the ' + + 'coverage threshold is still enforced, not just reported' + ); + }); + test('--merge-async lands in the c8 invocation, not after the node runner', () => { - for (const key of ['test:coverage:unit', 'test:coverage:report']) { + for (const key of ['test:coverage:unit', 'test:coverage:report', 'test:coverage:scripts-floor']) { const script = scripts[key]; const flagIndex = script.indexOf('--merge-async'); const nodeIndex = script.indexOf(' node ');