diff --git a/TESTING.md b/TESTING.md index 801a9377..a46e1a93 100644 --- a/TESTING.md +++ b/TESTING.md @@ -270,7 +270,15 @@ GitHub sends SIGTERM, which would end Playwright without any report; under `CI` `Still running when the run ended (SIGTERM): N` and one line per unfinished test (path relative to the suite's `rootDir`), then sends Playwright its own SIGINT, so the workers stop, the run ends interrupted and the blob report is -written to `tools/vendor/test-results/blob/`. That directory and the traces +written to `tools/vendor/test-results/blob/`. In the coverage job npm signals +only `frontend-coverage.mjs`, which forwards SIGTERM and SIGINT to the child it +is running (started in its own process group, so a group-wide signal still +arrives once). Delivery is proven locally in the pinned image for a SIGTERM to +the whole process group (`timeout -s TERM`). A SIGTERM to npm alone +(`timeout --foreground`) reaches nothing: npm forwards it to the `sh -c` that +runs the script, and that shell exits without passing it on. Delivery in CI, +where the step runs through `docker exec`, is not yet observed; the next real +step timeout will show it. That directory and the traces upload with `if: always()`; `npx playwright merge-reports` opens the blob. Each test keeps the 30 s default and there is no `globalTimeout`. Two limits of this: it assumes the npm cache hits (a cold cache adds up to 5 minutes to diff --git a/tests/vendor/frontend-coverage-cli.test.mjs b/tests/vendor/frontend-coverage-cli.test.mjs index 8243181b..d926f72c 100644 --- a/tests/vendor/frontend-coverage-cli.test.mjs +++ b/tests/vendor/frontend-coverage-cli.test.mjs @@ -1,9 +1,11 @@ import test from 'node:test'; import assert from 'node:assert/strict'; -import { existsSync, mkdtempSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { existsSync, mkdtempSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { EventEmitter } from 'node:events'; +import { setTimeout as delay } from 'node:timers/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { plan, runCoverage } from '../../tools/vendor/frontend-coverage.mjs'; +import { plan, runCoverage, runNode } from '../../tools/vendor/frontend-coverage.mjs'; const steps = argv => plan(argv).map(({ step, ...options }) => (Object.keys(options).length ? { step, ...options } : step)); @@ -91,3 +93,46 @@ test('shard 1 runs the node units into the shared raw dir, then the viewer suite assert.ok(calls[1].args.includes(join(paths.tools, 'playwright.config.mjs'))); assert.deepEqual(calls[2].args.slice(-2), ['--shard', '1/3']); }); + +// #425: npm and a CI step timeout signal only frontend-coverage.mjs; its running child must get it. +function hostWithSignals() { + return new EventEmitter(); +} + +test('runNode resolves on exit 0, rejects with the exit code otherwise and leaves no signal listener', async () => { + const host = hostWithSignals(); + await runNode(['-e', 'process.exit(0)'], {}, { host }); + await assert.rejects(runNode(['-e', 'process.exit(3)'], {}, { host }), /Command failed \(3\): -e process\.exit\(3\)/); + assert.equal(host.listenerCount('SIGTERM') + host.listenerCount('SIGINT'), 0); +}); + +test('runNode forwards SIGTERM to the running child, which can report before it exits', async t => { + const root = mkdtempSync(join(tmpdir(), 'pm-coverage-signal-')); + t.after(() => rmSync(root, { recursive: true, force: true })); + const ready = join(root, 'ready'); + const seen = join(root, 'seen'); + const child = `const fs = require('node:fs'); +process.on('SIGTERM', () => { fs.writeFileSync(${JSON.stringify(seen)}, 'SIGTERM'); process.exit(7); }); +fs.writeFileSync(${JSON.stringify(ready)}, ''); setInterval(() => {}, 1000);`; + const host = hostWithSignals(); + const running = runNode(['-e', child], {}, { host }); + while (!existsSync(ready)) await delay(20); + host.emit('SIGTERM'); + await assert.rejects(running, /Command failed \(7\)/); + assert.equal(readFileSync(seen, 'utf8'), 'SIGTERM', 'the child handled the forwarded signal'); + assert.equal(host.listenerCount('SIGTERM'), 0); +}); + +test('runNode forwards SIGINT and starts the child in its own process group', async () => { + const host = hostWithSignals(); + const kills = []; + let options; + const fake = new EventEmitter(); + fake.kill = signal => { kills.push(signal); queueMicrotask(() => fake.emit('exit', null, signal)); }; + const running = runNode(['x.mjs'], {}, { host, spawnChild: (command, args, spawnOptions) => { options = spawnOptions; return fake; } }); + assert.equal(options.detached, true, 'a group-wide signal reaches the child only through forwarding, once'); + host.emit('SIGINT'); + await assert.rejects(running, /Command failed \(null\): x\.mjs/); + assert.deepEqual(kills, ['SIGINT']); + assert.equal(host.listenerCount('SIGINT'), 0); +}); diff --git a/tools/vendor/frontend-coverage.mjs b/tools/vendor/frontend-coverage.mjs index 30de4152..3ade3bb5 100644 --- a/tools/vendor/frontend-coverage.mjs +++ b/tools/vendor/frontend-coverage.mjs @@ -1,7 +1,7 @@ import { existsSync, readFileSync, readdirSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'; import { dirname, join, relative } from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; -import { spawnSync } from 'node:child_process'; +import { spawn } from 'node:child_process'; import { repositoryRoot } from './vendor-viewerjs.mjs'; import { discoverSources, checkCoverage } from '@comfyassets/toolkit/testing/coverage'; @@ -10,10 +10,27 @@ const metrics = ['lines', 'branches', 'functions', 'statements']; const SHARD = /^([1-9]\d*)\/([1-9]\d*)$/; const USAGE = 'usage: frontend-coverage.mjs [--update-baseline] | --shard I/N | --merge-only [--update-baseline]'; -function runNode(args, env = {}) { - const result = spawnSync(process.execPath, args, { cwd: repositoryRoot, stdio: 'inherit', - env: { ...process.env, ...env } }); - if (result.status !== 0) throw new Error(`Command failed (${result.status}): ${args.join(' ')}`); +const FORWARDED_SIGNALS = ['SIGTERM', 'SIGINT']; + +// Runs one step's node child and waits for it. npm and a CI step timeout signal only this process, +// so SIGTERM and SIGINT are forwarded to the running child (#425): Playwright then reports the tests +// still running and writes its blob. The child gets its own process group, so a signal sent to the +// whole group (Ctrl-C, `timeout`) still reaches it exactly once, through this forwarding. +export function runNode(args, env = {}, { spawnChild = spawn, host = process } = {}) { + return new Promise((resolve, reject) => { + const child = spawnChild(process.execPath, args, { cwd: repositoryRoot, stdio: 'inherit', + env: { ...process.env, ...env }, detached: true }); + const forward = signal => () => child.kill(signal); + const handlers = FORWARDED_SIGNALS.map(signal => [signal, forward(signal)]); + for (const [signal, handler] of handlers) host.on(signal, handler); + const done = () => { for (const [signal, handler] of handlers) host.removeListener(signal, handler); }; + child.once('error', error => { done(); reject(error); }); + child.once('exit', code => { + done(); + if (code === 0) resolve(); + else reject(new Error(`Command failed (${code}): ${args.join(' ')}`)); + }); + }); } function defaultPaths() { @@ -112,17 +129,17 @@ export async function runCoverage({ argv = process.argv.slice(2), run = runNode, 'node-units'() { const tests = readdirSync(join(root, 'tests/js')).filter(file => file.endsWith('.test.js')) .map(file => `tests/js/${file}`); - run(['--test', ...tests], { NODE_V8_COVERAGE: raw }); + return run(['--test', ...tests], { NODE_V8_COVERAGE: raw }); }, viewer() { - run([playwright, 'test', '--config', join(tools, 'playwright.config.mjs')]); + return run([playwright, 'test', '--config', join(tools, 'playwright.config.mjs')]); }, workflows({ shard }) { - run([playwright, 'test', '--config', join(tools, 'playwright.workflows.config.mjs'), + return run([playwright, 'test', '--config', join(tools, 'playwright.workflows.config.mjs'), '--project', 'chromium', '--project', 'touch', ...(shard ? ['--shard', shard] : [])], { PM_JS_COVERAGE: raw }); }, report() { - run([join(tools, 'node_modules/c8/bin/c8.js'), 'report', '--all', '--src', sourceRoot, + return run([join(tools, 'node_modules/c8/bin/c8.js'), 'report', '--all', '--src', sourceRoot, '--include', 'web/js/**/*.js', '--include', 'web/js/**/*.mjs', '--temp-directory', raw, '--reports-dir', reportRoot, '--reporter', 'text', '--reporter', 'json-summary', '--reporter', 'html']); },