From a8db941fb7de28f72e0725dee89b6a298683b3a7 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Mon, 24 Apr 2023 19:09:40 -0400 Subject: [PATCH] [snap tester] Handle interrupts by forking runner Run snap tester on a forked node process so runs can be interrupted. (Currently, `Ctrl+C` is not handled until after all test fixtures finish compiling). This *feels* a bit heavy-handed, but main.ts is pretty small and doesn't do much other than listen for input / signals. Would love feedback here since I haven't really worked with nodejs / JS cli tools before - main.ts new file that spawns forked runner - pipe stdin/out/err to and from the child runner process, details described in comments. - listens for exit event of child - runner.ts - added logic to listen for interrupts and clean up (not really familiar with how file and tsc watchers are implemented, so we try to call 'close' on them just in case they need to release locks / do other cleanup) --- compiler/forget/package.json | 2 +- compiler/forget/packages/snap/package.json | 2 +- compiler/forget/packages/snap/src/main.ts | 50 +++++++++ compiler/forget/packages/snap/src/runner.ts | 109 ++++++++++++++------ 4 files changed, 131 insertions(+), 32 deletions(-) create mode 100644 compiler/forget/packages/snap/src/main.ts diff --git a/compiler/forget/package.json b/compiler/forget/package.json index 773db199e5..6721176bd4 100644 --- a/compiler/forget/package.json +++ b/compiler/forget/package.json @@ -14,7 +14,7 @@ "hash": "scripts/hash-dist.sh", "playground": "cd packages/playground && yarn && yarn dev", "test": "tsc && jest", - "snap": "node packages/snap/dist/runner.js", + "snap": "node packages/snap/dist/main.js", "snap:build": "cd packages/snap && yarn build", "ts:analyze-trace": "scripts/ts-analyze-trace.sh", "test262": "yarn run --silent test262-harness --preprocessor=scripts/test262-preprocessor.js", diff --git a/compiler/forget/packages/snap/package.json b/compiler/forget/packages/snap/package.json index 6cf7183347..28df99dc2c 100644 --- a/compiler/forget/packages/snap/package.json +++ b/compiler/forget/packages/snap/package.json @@ -3,7 +3,7 @@ "version": "0.0.1", "public": false, "description": "Snapshot testing CLI tool", - "main": "dist/index.js", + "main": "dist/main.js", "license": "MIT", "files": [ "src" diff --git a/compiler/forget/packages/snap/src/main.ts b/compiler/forget/packages/snap/src/main.ts new file mode 100644 index 0000000000..9f3410dd2d --- /dev/null +++ b/compiler/forget/packages/snap/src/main.ts @@ -0,0 +1,50 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import { fork } from "child_process"; +import invariant from "invariant"; +import process from "process"; +import * as readline from "readline"; +import { hideBin } from "yargs/helpers"; + +readline.emitKeypressEvents(process.stdin); + +if (process.stdin.isTTY) { + process.stdin.setRawMode(true); +} + +process.stdin.on("keypress", function (chunk, key) { + if (key && key.name === "c" && key.ctrl) { + // handle sigint + if (childProc) { + console.log("Interrupted!!"); + childProc.kill("SIGINT"); + childProc.unref(); + process.exit(-1); + } + } +}); + +const childProc = fork(require.resolve("./runner.js"), hideBin(process.argv), { + // for some reason, keypress events aren't sent to handlers in both processes + // when we `inherit` stdin. + // pipe stdout and stderr so we can silence child process after parent exits + stdio: ["pipe", "pipe", "pipe", "ipc"], + env: { FORCE_COLOR: "true" }, +}); + +invariant( + childProc.stdin && childProc.stdout && childProc.stderr, + "Expected forked process to have piped stdio" +); +process.stdin.pipe(childProc.stdin); +childProc.stdout.pipe(process.stdout); +childProc.stderr.pipe(process.stderr); + +childProc.on("exit", (code) => { + process.exit(code ?? -1); +}); diff --git a/compiler/forget/packages/snap/src/runner.ts b/compiler/forget/packages/snap/src/runner.ts index 86b5ecd3ee..d290ebbe59 100644 --- a/compiler/forget/packages/snap/src/runner.ts +++ b/compiler/forget/packages/snap/src/runner.ts @@ -21,16 +21,29 @@ import * as compiler from "./compiler-worker.js"; readline.emitKeypressEvents(process.stdin); -if (process.stdin.isTTY) { - process.stdin.setRawMode(true); -} +process.stdin.on("keypress", function (chunk, key) { + if (key && key.name === "c" && key.ctrl) { + cleanup(-1); + } +}); +process.on("SIGINT", function () { + // Parent process may send SIGINT + cleanup(-1); +}); -const argv: { +process.on("SIGTERM", function () { + cleanup(-1); +}); + +type Results = Map; +type RunnerOptions = { sync: boolean; workerThreads: boolean; watch: boolean; update: boolean; -} = yargs +}; + +const opts: RunnerOptions = yargs .boolean("sync") .describe( "sync", @@ -56,10 +69,6 @@ const argv: { .strict() .parseSync(hideBin(process.argv)); -const PARALLEL = !argv.sync; -const ENABLE_WORKER_THREADS = argv.workerThreads; -const WATCH = argv.watch; -const UPDATE = argv.update; const WORKER_PATH = require.resolve("./compiler-worker.js"); const COMPILER_PATH = path.join( process.cwd(), @@ -75,18 +84,33 @@ const FIXTURES_PATH = path.join( "compiler" ); -const worker: Worker & typeof compiler = new Worker(WORKER_PATH, { - enableWorkerThreads: ENABLE_WORKER_THREADS, -}) as any; -worker.getStderr().pipe(process.stderr); -worker.getStdout().pipe(process.stdout); - -type Results = Map; +/** + * Cleanup / handle interrupts + */ +const cleanupTasks: Array<() => void> = new Array(); +function pushCleanupTask(fn: () => void) { + cleanupTasks.push(fn); +} +function cleanup(code: number) { + for (const task of cleanupTasks) { + task(); + } + process.exit(code); +} +function clearConsole() { + // console.clear() only works when stdout is connected to a TTY device. + // we're currently piping stdout (see main.ts), so let's do a 'hack' + console.log("\u001Bc"); +} /** * Do a test run and return the test results */ -async function run(compilerVersion: number): Promise { +async function run( + worker: Worker & typeof compiler, + opts: RunnerOptions, + compilerVersion: number +): Promise { // We could in theory be fancy about tracking the contents of the fixtures // directory via our file subscription, but it's simpler to just re-read // the directory each time. @@ -100,7 +124,7 @@ async function run(compilerVersion: number): Promise { ).sort(); let entries: Array<[string, TestResult]>; - if (PARALLEL) { + if (!opts.sync) { // Note: promise.all to ensure parallelism when enabled entries = await Promise.all( fixtures.map(async (fixture) => { @@ -288,8 +312,17 @@ enum Mode { /** * Runs the compiler in watch or single-execution mode */ -async function main(): Promise { - if (WATCH) { +export async function main(opts: RunnerOptions): Promise { + const worker: Worker & typeof compiler = new Worker(WORKER_PATH, { + enableWorkerThreads: opts.workerThreads, + }) as any; + worker.getStderr().pipe(process.stderr); + worker.getStdout().pipe(process.stdout); + pushCleanupTask(() => { + worker.end(); + }); + + if (opts.watch) { // Monotonically increasing integer to describe the 'version' of the compiler. // This is passed to `compile()` (from compiler-worker) when compiling, so // that the worker knows when it has to reset its module cache and when its @@ -306,10 +339,10 @@ async function main(): Promise { async function onChange({ mode }: { mode: Mode }) { if (isCompilerValid) { const start = performance.now(); - console.clear(); + clearConsole(); console.log("Running tests..."); - const results = await run(compilerVersion); - console.clear(); + const results = await run(worker, opts, compilerVersion); + clearConsole(); if (mode === Mode.Update) { update(results); } else { @@ -331,7 +364,7 @@ async function main(): Promise { } // Run TS in incremental watch mode - const _tsWatch = watchSrc(onStart, (isSuccess) => { + const tsWatch = watchSrc(onStart, (isSuccess) => { // Bump the compiler version after a build finishes // and re-run tests if (isSuccess) { @@ -340,12 +373,15 @@ async function main(): Promise { isCompilerValid = isSuccess; onChange({ mode: Mode.Test }); }); + pushCleanupTask(() => { + tsWatch.close(); + }); // Watch the fixtures directory for changes // TODO: ignore changes that occurred as a result of our explicitly updating // fixtures in update() - maybe keep a timestamp of last known changes, and // ignore events that occurred prior to that timestamp. - const _fileSubscription = watcher.subscribe( + const fileSubscription = watcher.subscribe( FIXTURES_PATH, async (err, _events) => { if (err) { @@ -357,6 +393,16 @@ async function main(): Promise { } ); + pushCleanupTask(() => { + fileSubscription + .then((subscription) => { + subscription.unsubscribe(); + }) + .catch((err) => { + console.log("error cleaning up file subscription", err); + }); + }); + // Basic key event handling process.stdin.on("keypress", (str, key) => { if (key.name === "u") { @@ -364,8 +410,6 @@ async function main(): Promise { onChange({ mode: Mode.Update }); } else if (key.name === "q") { process.exit(0); - } else if (key.ctrl && key.name === "c") { - process.exit(0); } else { // any other key re-runs tests onChange({ mode: Mode.Test }); @@ -380,8 +424,8 @@ async function main(): Promise { () => {}, async (isSuccess: boolean) => { if (isSuccess) { - const results = await run(0); - if (UPDATE) { + const results = await run(worker, opts, 0); + if (opts.update) { update(results); } else { report(results); @@ -393,14 +437,19 @@ async function main(): Promise { } if (tsWatch != null) { tsWatch.close(); + tsWatch = null; } await worker.end(); process.exit(isSuccess ? 0 : -1); } ); + pushCleanupTask(() => { + tsWatch?.close(); + tsWatch = null; + }); } } // I couldn't figure out the right combination of settings to allow using `await` at the top-level, // but it's easy enough to use the promise API just here -main().catch((error) => console.error(error)); +main(opts).catch((error) => console.error(error));