ci(browser): forward SIGTERM and SIGINT from frontend-coverage.mjs to its child
In the coverage job npm signals only frontend-coverage.mjs. It blocked in spawnSync with no listener, so Playwright never saw a step timeout. It now runs each step's child with async spawn, in its own process group, and forwards SIGTERM and SIGINT to the running child. Step order, exit codes and the "Command failed (N)" message are unchanged. Locally, in the pinned image, `timeout -s TERM` on test:coverage printed the still-running tests and wrote both blob reports. TESTING.md records that a SIGTERM to npm alone stops at npm's `sh -c`, and that delivery through `docker exec` in CI is not yet observed. Refs #425
This commit is contained in:
Vito Sansevero
+9
-1
@@ -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
|
||||
|
||||
+47
-2
@@ -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);
|
||||
});
|
||||
|
||||
Vendored
+26
-9
@@ -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']);
|
||||
},
|
||||
|
||||
Reference in New Issue
Block a user