Skip to content

Commit

Permalink
Don't force-exit after tests have completed
Browse files Browse the repository at this point in the history
Fixes #1718.

With worker threads, this seems to stop AVA from detecting that the thread has exited, causing a hang.

Also remove flush logic implemented in #1722. Let's hope that current Node.js versions are better at flushing IPC before exiting.
  • Loading branch information
novemberborn authored Dec 4, 2023
1 parent 88e4333 commit af5684d
Show file tree
Hide file tree
Showing 11 changed files with 47 additions and 55 deletions.
5 changes: 0 additions & 5 deletions lib/fork.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,11 +122,6 @@ export default function loadFork(file, options, execArgv = process.execArgv) {
break;
}

case 'ping': {
send({type: 'pong'});
break;
}

default: {
emitStateChange(message.ava);
}
Expand Down
1 change: 1 addition & 0 deletions lib/reporters/default.js
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,7 @@ export default class Reporter {
writePendingTests(evt) {
for (const [file, testsInFile] of evt.pendingTests) {
if (testsInFile.size === 0) {
this.lineWriter.writeLine(`Failed to exit when running ${this.relativeFile(file)}\n`);
continue;
}

Expand Down
4 changes: 4 additions & 0 deletions lib/run-status.js
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,10 @@ export default class RunStatus extends Emittery {

case 'worker-finished': {
stats.finishedWorkers++;
if (this.pendingTests.get(event.testFile)?.size === 0) {
this.pendingTests.delete(event.testFile);
}

break;
}

Expand Down
58 changes: 27 additions & 31 deletions lib/worker/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,35 +23,26 @@ import {isRunningInThread, isRunningInChildProcess} from './utils.cjs';
const currentlyUnhandled = setUpCurrentlyUnhandled();
let runner;

let forcingExit = false;

const forceExit = () => {
forcingExit = true;
process.exit(1);
};

// Override process.exit with an undetectable replacement
// to report when it is called from a test (which it should never be).
const {apply} = Reflect;
const realExit = process.exit;

async function exit(code, forceSync = false) {
const flushing = channel.flush();
if (!forceSync) {
await flushing;
const handleProcessExit = (target, thisArg, args) => {
if (!forcingExit) {
const error = new Error('Unexpected process.exit()');
Error.captureStackTrace(error, handleProcessExit);
channel.send({type: 'process-exit', stack: error.stack});
}

apply(realExit, process, [code]);
}

const handleProcessExit = (fn, receiver, args) => {
const error = new Error('Unexpected process.exit()');
Error.captureStackTrace(error, handleProcessExit);
channel.send({type: 'process-exit', stack: error.stack});

// Make sure to extract the code only from `args` rather than e.g. `Array.prototype`.
// This level of paranoia is usually unwarranted, but we're dealing with test code
// that has already colored outside the lines.
const code = args.length > 0 ? args[0] : undefined;

// Force a synchronous exit as guaranteed by the real process.exit().
exit(code, true);
target.apply(thisArg, args);
};

process.exit = new Proxy(realExit, {
process.exit = new Proxy(process.exit, {
apply: handleProcessExit,
});

Expand Down Expand Up @@ -101,7 +92,7 @@ const run = async options => {

runner.on('error', error => {
channel.send({type: 'internal-error', err: serializeError(error)});
exit(1);
forceExit();
});

runner.on('finish', async () => {
Expand All @@ -112,30 +103,35 @@ const run = async options => {
}
} catch (error) {
channel.send({type: 'internal-error', err: serializeError(error)});
exit(1);
forceExit();
return;
}

try {
await Promise.all(sharedWorkerTeardowns.map(fn => fn()));
} catch (error) {
channel.send({type: 'uncaught-exception', err: serializeError(error)});
exit(1);
forceExit();
return;
}

nowAndTimers.setImmediate(() => {
for (const rejection of currentlyUnhandled()) {
const unhandled = currentlyUnhandled();
if (unhandled.length === 0) {
return;
}

for (const rejection of unhandled) {
channel.send({type: 'unhandled-rejection', err: serializeError(rejection.reason, {testFile: options.file})});
}

exit(0);
forceExit();
});
});

process.on('uncaughtException', error => {
channel.send({type: 'uncaught-exception', err: serializeError(error, {testFile: options.file})});
exit(1);
forceExit();
});

// Store value to prevent required modules from modifying it.
Expand Down Expand Up @@ -248,11 +244,11 @@ const run = async options => {
channel.unref();
} else {
channel.send({type: 'missing-ava-import'});
exit(1);
forceExit();
}
} catch (error) {
channel.send({type: 'uncaught-exception', err: serializeError(error, {testFile: options.file})});
exit(1);
forceExit();
}
};

Expand Down
16 changes: 0 additions & 16 deletions lib/worker/channel.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -110,22 +110,6 @@ exports.peerFailed = selectAvaMessage(handle.channel, 'peer-failed');
exports.send = handle.send.bind(handle);
exports.unref = handle.unref.bind(handle);

let pendingPings = Promise.resolve();
async function flush() {
handle.ref();
const promise = pendingPings.then(async () => {
handle.send({type: 'ping'});
await selectAvaMessage(handle.channel, 'pong');
if (promise === pendingPings) {
handle.unref();
}
});
pendingPings = promise;
await promise;
}

exports.flush = flush;

let channelCounter = 0;
let messageCounter = 0;

Expand Down
2 changes: 2 additions & 0 deletions test-tap/reporters/default.regular.v18.log
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@

null

---tty-stream-chunk-separator
✘ unhandled-rejection.cjs exited with a non-zero exit code: 1
---tty-stream-chunk-separator
─

Expand Down
2 changes: 2 additions & 0 deletions test-tap/reporters/default.regular.v20.log
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@

null

---tty-stream-chunk-separator
✘ unhandled-rejection.cjs exited with a non-zero exit code: 1
---tty-stream-chunk-separator
─

Expand Down
2 changes: 2 additions & 0 deletions test-tap/reporters/default.regular.v21.log
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@

null

---tty-stream-chunk-separator
✘ unhandled-rejection.cjs exited with a non-zero exit code: 1
---tty-stream-chunk-separator
─

Expand Down
4 changes: 3 additions & 1 deletion test-tap/reporters/tap.regular.v18.log
Original file line number Diff line number Diff line change
Expand Up @@ -246,11 +246,13 @@ not ok 27 - unhandled-rejection
formatted: 'null'
...
---tty-stream-chunk-separator
not ok 28 - unhandled-rejection.cjs exited with a non-zero exit code: 1
---tty-stream-chunk-separator

1..21
# tests 20
# pass 6
# skip 1
# fail 20
# fail 21

---tty-stream-chunk-separator
4 changes: 3 additions & 1 deletion test-tap/reporters/tap.regular.v20.log
Original file line number Diff line number Diff line change
Expand Up @@ -246,11 +246,13 @@ not ok 27 - unhandled-rejection
formatted: 'null'
...
---tty-stream-chunk-separator
not ok 28 - unhandled-rejection.cjs exited with a non-zero exit code: 1
---tty-stream-chunk-separator

1..21
# tests 20
# pass 6
# skip 1
# fail 20
# fail 21

---tty-stream-chunk-separator
4 changes: 3 additions & 1 deletion test-tap/reporters/tap.regular.v21.log
Original file line number Diff line number Diff line change
Expand Up @@ -246,11 +246,13 @@ not ok 27 - unhandled-rejection
formatted: 'null'
...
---tty-stream-chunk-separator
not ok 28 - unhandled-rejection.cjs exited with a non-zero exit code: 1
---tty-stream-chunk-separator

1..21
# tests 20
# pass 6
# skip 1
# fail 20
# fail 21

---tty-stream-chunk-separator

0 comments on commit af5684d

Please sign in to comment.