Skip to content

Commit b3962f7

Browse files
BridgeARsabrenner
authored andcommitted
fix(scripts): harden mocha-parallel-files against crashes and silent … (#8243)
* fix(scripts): harden mocha-parallel-files against crashes and silent failures Several paths in the parallel test runner crashed the parent or hid failures silently, mostly on Windows. Spawn errors (ENOENT, EAGAIN, EMFILE, AV blocking the binary) emit `error` not `exit`, so the missing listener turned every failed spawn into an uncaught parent-side exception. SIGINT/SIGTERM didn't reach children on Windows where there's no process group to inherit. EPIPE downgraded real test failures to exit 0. Track live children, handle all three events, forward signals (escalating to SIGKILL after 1 s), and reuse `process.exitCode` from the EPIPE path. JUnit aggregation also lost data: with `jobs === 1` each child wrote its xunit straight to `junitOutFile` and later children overwrote earlier ones, so CI got only the last file's results. Sharding is now unconditional. Smaller correctness fixes folded in: `--timeout 0` was treated as falsy, the stream-end leftover got flushed without a trailing newline, `isFailureStartLine` matched any test name containing "N failing", junit attribute parsing returned `NaN` for malformed input, junit cleanup didn't survive a Windows file lock, and `globSync` results were sorted per pattern instead of globally. * refactor(scripts): align mocha-parallel-files with project style The bigger items: extracted `Entry` / `EntryStats` / `ParseArgsResult` typedefs so the `/** @type {...} */ (null)` casts inside the entries factory and `parseArgs` body go away. Renamed single-letter callback args (`m`, `l`, `f`, `p`) to expressive names; the for-loop counter `i` in `parseArgs` stays. Switched the one-shot child events (`message`, `exit`, `error`, `stdout` / `stderr` 'end') from `.on` to `.once`. Added `@param` JSDoc on every callable the diff defines or changes — closures included — plus three boy-scout `@param` blocks on adjacent helpers (`isWarningLine`, `handleStdoutChunk`, `handleStderrChunk`). No prose lines; method names carry the contracts. The smaller items: `node:` prefix on the core imports, `error` instead of `err` in catch handlers, `??` over `||` for the stat-coalesce defaults, brace the three `if/else` sites, drop the dead `?? ''` after `parts.at(-1)`, drop the no-op `unknown`-cast on the IPC `msg`, inline `ensureDir` (its body was a single `fs.mkdirSync` call), and remove the `stableUnique` one-line helper in favour of an inline `[...new Set(...)]`.
1 parent c7c93d3 commit b3962f7

5 files changed

Lines changed: 429 additions & 117 deletions

File tree

.github/CODEOWNERS

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -283,10 +283,14 @@
283283
/integration-tests/coverage-fixtures/worker.js @DataDog/lang-platform-js
284284
/integration-tests/coverage-manual-process.spec.js @DataDog/lang-platform-js
285285
/integration-tests/init.spec.js @DataDog/lang-platform-js
286+
/integration-tests/mocha-parallel-files-fixtures/ @DataDog/lang-platform-js
287+
/integration-tests/mocha-parallel-files.spec.js @DataDog/lang-platform-js
286288
/integration-tests/package-guardrails.spec.js @DataDog/lang-platform-js
287289
/integration-tests/package-guardrails/flush.js @DataDog/lang-platform-js
288290
/integration-tests/startup.spec.js @DataDog/lang-platform-js
289291

292+
/scripts/mocha-parallel-files.js @DataDog/lang-platform-js
293+
290294
/packages/datadog-core @DataDog/lang-platform-js
291295
/packages/datadog-shimmer @DataDog/lang-platform-js
292296
/packages/dd-trace/*/crashtracking @DataDog/lang-platform-js
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
'use strict'
2+
3+
const assert = require('node:assert/strict')
4+
5+
before(() => {
6+
if (typeof process.send === 'function') {
7+
process.send({ type: 'unrelated-noise', stamp: Date.now() })
8+
}
9+
})
10+
11+
describe('extra-ipc-message fixture', () => {
12+
it('passes after an unrelated IPC payload', () => {
13+
assert.strictEqual(1, 1)
14+
})
15+
})
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
'use strict'
2+
3+
const { setTimeout: sleep } = require('node:timers/promises')
4+
5+
describe('long-running fixture', () => {
6+
it('runs long enough to be interrupted', async function () {
7+
this.timeout(10_000)
8+
await sleep(3_000)
9+
})
10+
})
Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
'use strict'
2+
3+
const assert = require('node:assert/strict')
4+
const { spawn } = require('node:child_process')
5+
const path = require('node:path')
6+
7+
const repoRoot = path.resolve(__dirname, '..')
8+
const parallelScript = path.join(repoRoot, 'scripts', 'mocha-parallel-files.js')
9+
const fixturesDir = path.join(__dirname, 'mocha-parallel-files-fixtures')
10+
11+
/**
12+
* @typedef {{
13+
* stdout: string,
14+
* stderr: string,
15+
* code: number|null,
16+
* signal: NodeJS.Signals|null
17+
* }} ChildResult
18+
*
19+
* @typedef {{
20+
* killSignal?: NodeJS.Signals,
21+
* killOnFirstStdout?: boolean,
22+
* timeoutMs?: number
23+
* }} RunOpts
24+
*/
25+
26+
/**
27+
* @param {string[]} args
28+
* @param {RunOpts} [opts]
29+
* @returns {Promise<ChildResult>}
30+
*/
31+
function runParallel (args, opts = {}) {
32+
return new Promise((resolve, reject) => {
33+
// Drop CI from the inherited env; otherwise mocha-parallel-files writes a
34+
// junit file under the repo root for every spawn here.
35+
const env = { ...process.env, CI: '' }
36+
const child = spawn(process.execPath, [parallelScript, ...args], { cwd: repoRoot, env })
37+
let stdout = ''
38+
let stderr = ''
39+
let killed = false
40+
const fireKill = () => {
41+
if (killed || !opts.killSignal) return
42+
killed = true
43+
child.kill(opts.killSignal)
44+
}
45+
46+
child.stdout.setEncoding('utf8')
47+
child.stderr.setEncoding('utf8')
48+
child.stdout.on('data', (chunk) => {
49+
stdout += chunk
50+
// The first stdout chunk proves the parent has finished its bootstrap,
51+
// registered SIGINT/SIGTERM handlers, and is forwarding output from a
52+
// running child. Sending the signal earlier races the handler setup,
53+
// which on slow CI lets the default signal terminate the parent before
54+
// it can pin its exit code.
55+
if (opts.killOnFirstStdout) fireKill()
56+
})
57+
child.stderr.on('data', (chunk) => { stderr += chunk })
58+
59+
const timeoutMs = opts.timeoutMs ?? 20_000
60+
const timer = setTimeout(() => {
61+
child.kill('SIGKILL')
62+
reject(new Error(`mocha-parallel-files did not exit within ${timeoutMs}ms`))
63+
}, timeoutMs)
64+
timer.unref()
65+
66+
child.once('error', reject)
67+
child.once('exit', (code, signal) => {
68+
clearTimeout(timer)
69+
resolve({ stdout, stderr, code, signal })
70+
})
71+
})
72+
}
73+
74+
describe('mocha-parallel-files script', function () {
75+
this.timeout(30_000)
76+
77+
it('records child stats even when the child sends an unrelated IPC payload first', async () => {
78+
const fixture = path.join(fixturesDir, 'extra-ipc-message.js')
79+
const { stdout, code, signal } = await runParallel(['--', fixture])
80+
81+
assert.strictEqual(code, 0)
82+
assert.strictEqual(signal, null)
83+
assert.match(stdout, /Total:\s+1\b/)
84+
assert.match(stdout, /Passed:\s+1\b/)
85+
assert.match(stdout, /Failed:\s+0\b/)
86+
})
87+
88+
it('preserves the SIGINT exit code on user interrupt', async function () {
89+
if (process.platform === 'win32') {
90+
this.skip()
91+
return
92+
}
93+
const fixture = path.join(fixturesDir, 'long-running.js')
94+
const { code } = await runParallel(['--', fixture], { killSignal: 'SIGINT', killOnFirstStdout: true })
95+
assert.strictEqual(code, 130)
96+
})
97+
})

0 commit comments

Comments
 (0)