fix: report the real cause when check-env.cjs's npm-version check fails
Discovered blocking this PR's own Windows CI (unrelated to this PR's actual diff, fixed inline per this repo's no-defer policy): PR #4552's "full test (windows-latest, 24, shard 2/3)" job failed tests/check-env.test.cjs's npm-version subtest with "npm binary not found on PATH" under chunk 5/9's heavy load (51 concurrent files, 5+ minutes). Root-caused via the CI log: the check's spawnSync call used a 10s timeout, and every failure mode -- ENOENT, a signal-killed timeout, a non-zero exit, a thrown spawn error -- collapsed into that one message (only `res.status === 0 && res.stdout` was checked), so a genuine npm.cmd cold-start timeout under contention was indistinguishable from npm actually being absent. Extracted the reason-selection into describeNpmVersionCheckFailure, a pure function in the new scripts/lib/npm-version-check-diagnosis.cjs (kept out of check-env.cjs itself, which runs its CLI unconditionally on require with no `require.main === module` guard, so the pure logic can be unit-tested without triggering a real environment check). Reports ENOENT, signal-kill, non-zero-exit, and thrown-error cases distinctly. Does NOT raise the 10s timeout itself -- a slow subprocess under contention is a cost to reduce, not a tolerance to widen. Also fixed a stale tsconfig.build.tsbuildinfo incremental-build cache discovered while verifying this change: npm run build:lib was silently omitting gsd-core/bin/lib/markdown-table.cjs (a real, needed compiled module -- src/state-document.cts requires it), which only surfaced via npm run lint:generated-sync's gen-health-docs check failing with Cannot find module. Deleting the cache and rebuilding fresh restored it; docs/INVENTORY-MANIFEST.json needed no net change once the build was genuinely complete. Manually verified describeNpmVersionCheckFailure's five branches directly (ENOENT, signal-kill, non-zero exit, thrown error, defensive default) before wiring the test file, since this repo blocks local node --test. Re-ran node scripts/check-env.cjs directly to confirm the real success path is unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
60
tests/npm-version-check-diagnosis.test.cjs
Normal file
60
tests/npm-version-check-diagnosis.test.cjs
Normal file
@@ -0,0 +1,60 @@
|
||||
'use strict';
|
||||
|
||||
/**
|
||||
* Regression coverage for #4460 (discovered blocking this PR's own Windows
|
||||
* CI, unrelated to this PR's actual diff -- fixed inline per this repo's
|
||||
* no-defer policy): scripts/check-env.cjs's npm-version check used to
|
||||
* collapse every spawnSync failure mode into one message, "npm binary not
|
||||
* found on PATH" -- including a TIMEOUT under CI load, which looks
|
||||
* identical to a genuine ENOENT (status stays null, stdout stays empty
|
||||
* either way). describeNpmVersionCheckFailure is the extracted, pure
|
||||
* reason-selection logic; kept in scripts/lib/ (not scripts/check-env.cjs
|
||||
* itself, which runs its CLI unconditionally on require) so it can be
|
||||
* required directly here without triggering a real environment check.
|
||||
*/
|
||||
|
||||
const { describe, test } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
|
||||
const { describeNpmVersionCheckFailure } = require('../scripts/lib/npm-version-check-diagnosis.cjs');
|
||||
|
||||
describe('describeNpmVersionCheckFailure (#4460)', () => {
|
||||
test('a genuine ENOENT (npm truly absent) reports the original message', () => {
|
||||
const spawnResult = { status: null, stdout: '', signal: null, error: Object.assign(new Error('spawnSync npm.cmd ENOENT'), { code: 'ENOENT' }) };
|
||||
assert.equal(
|
||||
describeNpmVersionCheckFailure(spawnResult, null, 10_000),
|
||||
'npm binary not found on PATH',
|
||||
);
|
||||
});
|
||||
|
||||
test('a signal-killed spawn (the timeout case) is reported as a timeout, not a missing binary', () => {
|
||||
const spawnResult = { status: null, stdout: '', signal: 'SIGTERM', error: null };
|
||||
const reason = describeNpmVersionCheckFailure(spawnResult, null, 10_000);
|
||||
assert.match(reason, /killed \(signal SIGTERM\)/);
|
||||
assert.match(reason, /10000ms timeout under CI load/);
|
||||
assert.doesNotMatch(reason, /^npm binary not found on PATH$/);
|
||||
});
|
||||
|
||||
test('a non-zero exit with no stdout is reported with the actual exit code', () => {
|
||||
const spawnResult = { status: 1, stdout: '', signal: null, error: null };
|
||||
assert.equal(
|
||||
describeNpmVersionCheckFailure(spawnResult, null, 10_000),
|
||||
'npm --version exited 1 with no usable output',
|
||||
);
|
||||
});
|
||||
|
||||
test('spawnSync itself throwing (not just returning a failure result) is reported with the thrown message', () => {
|
||||
const thrown = new Error('EACCES: permission denied');
|
||||
assert.equal(
|
||||
describeNpmVersionCheckFailure(null, thrown, 10_000),
|
||||
'npm --version could not be spawned: EACCES: permission denied',
|
||||
);
|
||||
});
|
||||
|
||||
test('no spawn result and no thrown error (defensive default) falls back to the original message', () => {
|
||||
assert.equal(
|
||||
describeNpmVersionCheckFailure(null, null, 10_000),
|
||||
'npm binary not found on PATH',
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user