diff --git a/packages/cli/src/index.test.ts b/packages/cli/src/index.test.ts index 3907d03..fe4b0bc 100644 --- a/packages/cli/src/index.test.ts +++ b/packages/cli/src/index.test.ts @@ -1,7 +1,12 @@ -import cp from "child_process"; +import cp from "node:child_process"; +import { URL } from "node:url"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import process from "node:process"; + import { expect } from "chai"; import { it } from "mocha"; -import { URL } from "url"; const cliPath = new URL("./cli.ts", import.meta.url).pathname; @@ -9,7 +14,15 @@ function cli(...args: string[]) { return cp.spawnSync( process.execPath, ["--import", "tsx", cliPath, ...args], - { encoding: 'utf8', env: { ...process.env, FORCE_COLOR: "false" }, timeout: 4_000 }, + { encoding: 'utf8', env: { ...process.env, FORCE_COLOR: "false" }, timeout: 5_000 }, + ); +} + +function spawnCli(...args: string[]) { + return cp.spawn( + process.execPath, + ["--import", "tsx", cliPath, ...args], + { stdio: "ignore", env: { ...process.env, FORCE_COLOR: "false" }, timeout: 5_000 }, ); } @@ -19,6 +32,41 @@ function parseJsonOutput(output: string) { return JSON.parse(output.slice(jsonStart, jsonEnd + 1)); } +function waitForFile(filePath: string) { + return new Promise(resolve => { + const listener = (stats: fs.Stats) => { + if (stats.isFile()) { + // Stop listening for changes + fs.unwatchFile(filePath, listener); + resolve(fs.readFileSync(filePath, "utf8")); + } + }; + fs.watchFile(filePath, { interval: 10 }, listener); + if (fs.existsSync(filePath)) { + // Start by doing it once + listener(fs.statSync(filePath)); + } + }); +} + +function isProcessRunning(pid: number) { + // Tried using kill to assert the process has exited: + // > This method will throw an error if the target pid does not exist. + // > As a special case, a signal of 0 can be used to test for the existence of a process. + // see https://nodejs.org/api/process.html#processkillpid-signal + // process.kill(pid, 0); + // ... but this didn't throw on Linux - instead we're calling good ol' "ps" + const result = cp.spawnSync("ps", ["-p", pid.toString(), "-o", "pid"], { encoding: "utf8" }); + const [, match] = result.stdout.trim().split("\n"); + return match === pid.toString(); +} + +async function waitForProcessToDie(pid: number, pollDelay = 100) { + while (isProcessRunning(pid)) { + await new Promise(resolve => setTimeout(resolve, pollDelay)); + } +} + describe("Mocha Remote CLI", () => { it("prints --help", () => { const output = cli("--help"); @@ -102,6 +150,46 @@ describe("Mocha Remote CLI", () => { expect(output.status).equals(0); }); + it("kills unresponsive command when interrupted", async () => { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "mocha-remote-cli-test-")); + const outFile = path.join(tempDir, "out.txt"); + expect(fs.existsSync(outFile)).equals(false, "Expected the outfile to be missing"); + const cliProcess = spawnCli("--port", "0", "--context", `endlessLoop,outFile=${outFile}`, "--", "tsx", "src/test/hanging-client.ts"); + + const outFileContent = await waitForFile(outFile); + const success = cliProcess.kill("SIGINT"); // Interrupt Mocha Remote CLI + expect(success).equals(true, "Expected mocha-remote cli to exit when interrupted"); + + // Wait a bit for Mocha Remote CLI to forcefully kill the command process + await new Promise(resolve => cliProcess.once("close", resolve)); + expect(cliProcess.exitCode).equals(0); + + // Expect the command process to have died + const { pid } = JSON.parse(outFileContent); + expect(typeof pid).equals("number"); + await waitForProcessToDie(pid); + }); + + it("exits clean and kills hanging commands", async () => { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "mocha-remote-cli-test-")); + const outFile = path.join(tempDir, "out.txt"); + expect(fs.existsSync(outFile)).equals(false, "Expected the outfile to be missing"); + // Using spawnCli because it ignores stdout, which makes the spawnSync hang for some reason + const cliProcess = spawnCli("--port", "0", "--context", `outFile=${outFile}`, "--", "tsx", "src/test/hanging-client.ts"); + await new Promise(resolve => cliProcess.once("close", resolve)); + + expect(cliProcess.exitCode).equals(0, "Expected signal of the mocha-remote cli to remain a success"); + + // Checking the "exit" of the wrapped command + expect(fs.existsSync(outFile)).equals(true, "Expected the outfile to exists"); + const outFileContent = await waitForFile(outFile); + + // Expect the command process to have died + const { pid } = JSON.parse(outFileContent); + expect(typeof pid).equals("number"); + await waitForProcessToDie(pid); + }); + describe("failures", () => { it("propagates failures as exit code", () => { const output = cli("--port", "0", "--context", "failure=Totally expected", "--", "tsx", "src/test/simple-client.ts"); diff --git a/packages/cli/src/index.ts b/packages/cli/src/index.ts index 5e2a1fc..32943c9 100644 --- a/packages/cli/src/index.ts +++ b/packages/cli/src/index.ts @@ -1,24 +1,28 @@ -import fs from "fs"; +import assert from "node:assert"; +import fs from "node:fs"; +import cp from "node:child_process"; +import { inspect } from "node:util"; + import yargs from 'yargs'; import { hideBin } from 'yargs/helpers'; -import cp from "child_process"; import chalk from "chalk"; -import { inspect } from "util"; + +import { Server, ReporterOptions, CustomContext, WebSocket, ClientError } from "mocha-remote-server"; const packageJsonPath = new URL("../package.json", import.meta.url); const packageJson = JSON.parse(fs.readFileSync(packageJsonPath, "utf8")); -import { Server, ReporterOptions, CustomContext, WebSocket, ClientError } from "mocha-remote-server"; - export type Logger = (...args: unknown[]) => void; export type CleanupTask = () => (void | Promise); -const cleanupTasks: CleanupTask[] = []; +const cleanupTasks = new Set(); -async function cleanup() { +function cleanup() { // Move the tasks into a local variable to avoid rerunning these on multiple invokations - const tasks = cleanupTasks.splice(0, cleanupTasks.length); - await Promise.all(tasks.map(task => task())).catch(err => { + const tasks = [...cleanupTasks]; + cleanupTasks.clear(); + // Execute a chain of promises + tasks.reduce((previous, task) => previous.then(task), Promise.resolve()).catch(err => { // eslint-disable-next-line no-console console.error(`Failed while cleaning up: ${err}`); }); @@ -76,10 +80,10 @@ type ServerOptions = { // Start the server export async function startServer({ log, server, command, exitOnError }: ServerOptions): Promise { - cleanupTasks.push(async () => { + cleanupTasks.add(async () => { if (server.listening) { - log("🧹 Stopping server"); await server.stop(); + log("🧹 Stopped the server"); } }); @@ -109,9 +113,7 @@ export async function startServer({ log, server, command, exitOnError }: ServerO if (code !== 1000 && exitOnError) { print(displayClient(ws), chalk.red("EXITTING"), "after an abnormal disconnect"); process.exitCode = 1; - cleanup().then(() => { - process.exit(); - }); + cleanup(); } }); @@ -158,19 +160,19 @@ export async function startServer({ log, server, command, exitOnError }: ServerO MOCHA_REMOTE_ID: server.config.id, } }); + const commandPid = commandProcess.pid; + assert(typeof commandPid === "number", "Expected command process to have a pid"); - const commandDescription = `${chalk.italic(...command)} (pid = ${chalk.bold(commandProcess.pid)})` + const commandDescription = `${chalk.italic(...command)} (pid = ${chalk.bold(commandPid)})` - cleanupTasks.push(() => { - if (commandProcess.connected) { - log(`🧹 Terminating: ${commandDescription}`); - const success = commandProcess.kill(); - if (!success) { - log(`💀 Sending SIGKILL to pid = ${commandProcess.pid}`); - commandProcess.kill("SIGKILL"); - } - } - }); + const killCommandProcess = async () => { + // There's no need to await a close if the close was caused by a cleanup + commandProcess.removeListener("close", commandCloseListener); + commandProcess.kill(); + log(`🧹 Terminated command (pid = ${chalk.bold(commandPid)})`); + }; + + cleanupTasks.add(killCommandProcess); /* eslint-disable-next-line no-console */ log( @@ -179,7 +181,10 @@ export async function startServer({ log, server, command, exitOnError }: ServerO ); // We should exit when the command process exits - and vise versa - commandProcess.once("close", (code, signal) => { + const commandCloseListener = (code: number | null, signal: string | null) => { + // Avoid killing the process if it's already closed + cleanupTasks.delete(killCommandProcess); + // Print the exit and cause to the log const cause = exitCause(code, signal); log('\n' + chalk.dim(`Command exited (${cause})`)); // Inherit exit code from sub-process if nothing has already sat it @@ -187,15 +192,12 @@ export async function startServer({ log, server, command, exitOnError }: ServerO process.exitCode = code; } cleanup(); - }); + }; - process.on("SIGINT", () => { - commandProcess.kill("SIGINT"); - }); - - process.on("exit", () => { - cleanup(); - }); + commandProcess.once("close", commandCloseListener); + + process.on("SIGINT", cleanup); + process.on("exit", cleanup); } } @@ -344,6 +346,7 @@ export function run(args = hideBin(process.argv)): void { // Run once and exit with the failures as exit code server.run(failures => { process.exitCode = failures; + cleanup(); }); } }, diff --git a/packages/cli/src/test/hanging-client.ts b/packages/cli/src/test/hanging-client.ts new file mode 100644 index 0000000..0cd8374 --- /dev/null +++ b/packages/cli/src/test/hanging-client.ts @@ -0,0 +1,24 @@ +import { Client } from "mocha-remote-client"; + +import assert from "node:assert"; +import fs from "node:fs"; + +new Client({ + tests({ outFile, endlessLoop }) { + assert(typeof outFile === "string", "Expected a 'outFile' in context"); + assert(fs.existsSync(outFile) === false, `Expected '${outFile}' to not exist`); + fs.writeFileSync(outFile, JSON.stringify({ pid: process.pid }), "utf8"); + + if (endlessLoop) { + // eslint-disable-next-line no-constant-condition + while (true) { + // Useful to simulate a process that cannot be exited gracefully + } + } + + it("succeeds but doesn't exit", () => {}); + } +}); + +// Do a long timeout to prevent an exit +setTimeout(() => {}, 60 * 1000); diff --git a/packages/client/src/serialization.ts b/packages/client/src/serialization.ts index 968924b..c5c6104 100644 --- a/packages/client/src/serialization.ts +++ b/packages/client/src/serialization.ts @@ -15,7 +15,7 @@ function toJSON(value: Record): Record { return { type: "error", message, - stack: stack ? filterStack(stack) : stack, + stack, }; } else if (typeof value === "object" && typeof value.serialize === "function") { const result = value.serialize(); @@ -32,10 +32,6 @@ function toJSON(value: Record): Record { } } -function filterStack(stack: string) { - return stack.split("\n").filter(line => line.includes("client/dist/node.bundle.cjs.js") === false).join("\n"); -} - export function createReplacer(): Replacer { return function(this: unknown, key: string, value: unknown) { debug(`Replacing %s`, value); diff --git a/packages/integration-tests/src/basic.test.ts b/packages/integration-tests/src/basic.test.ts index 5d7cfcd..48329a3 100644 --- a/packages/integration-tests/src/basic.test.ts +++ b/packages/integration-tests/src/basic.test.ts @@ -22,6 +22,7 @@ describe("basic", () => { await server.start(); expect(server.url).to.oneOf([ "ws://localhost:8090", + "ws://127.0.0.1:8090", "ws://[::1]:8090" ]); }); diff --git a/packages/server/src/Server.ts b/packages/server/src/Server.ts index aafdeae..b5d731f 100644 --- a/packages/server/src/Server.ts +++ b/packages/server/src/Server.ts @@ -143,7 +143,7 @@ export class Server extends ServerEventEmitter { this.debug("Server is stopping"); await new Promise((resolve, reject) => { if (this.wss) { - // Terminate all clients + // Close any client connections, giving a code and reason for (const ws of this.wss.clients) { ws.close(code, reason); } @@ -158,6 +158,10 @@ export class Server extends ServerEventEmitter { resolve(); } }); + // Terminate any clients still connected, allowing the server to close + for (const ws of this.wss.clients) { + ws.terminate(); + } } else { resolve(); } diff --git a/packages/server/src/serialization.test.ts b/packages/server/src/serialization.test.ts index 0cb45b8..9b86601 100644 --- a/packages/server/src/serialization.test.ts +++ b/packages/server/src/serialization.test.ts @@ -1,8 +1,8 @@ import { expect } from "chai"; -import { Suite, Test } from "mocha"; +import { Test } from "mocha"; import { stringify } from "flatted"; -import { deserialize, createReviver } from "./serialization"; +import { deserialize } from "./serialization"; describe("Server serialization", () => { diff --git a/packages/server/src/serialization.ts b/packages/server/src/serialization.ts index f7807d3..01d3e37 100644 --- a/packages/server/src/serialization.ts +++ b/packages/server/src/serialization.ts @@ -5,13 +5,21 @@ const debug = extend("serialization"); type Reviver = (this: unknown, key: string, value: unknown) => unknown; +function smellsLikeMochaRemote(line: string) { + return line.includes("/mocha-remote"); +} + +function filterStack(stack: string) { + return stack.split("\n").filter(line => !smellsLikeMochaRemote(line)).join("\n"); +} + export function createReviver(): Reviver { function reviveObject(obj: Record) { if (obj.type === "error") { if (typeof obj.message === "string" && typeof obj.stack === "string") { const err = new Error(obj.message); - err.stack = obj.stack; + err.stack = filterStack(obj.stack); return err; } else { throw new Error("Expected Error to have message and stack");