Skip to content

Commit

Permalink
test_runner: reland run global after() hook earlier
Browse files Browse the repository at this point in the history
This commit reverts the revert in
bb52656. It also includes the
fix for the issue that required the revert
(nodejs#49059 (comment))
and an additional common.mustCall() in the added test.

Refs: nodejs#49059
Refs: nodejs#49110
PR-URL: nodejs#49116
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
  • Loading branch information
cjihrig authored and RafaelGSS committed Aug 15, 2023
1 parent de95c3d commit b9407ff
Show file tree
Hide file tree
Showing 7 changed files with 104 additions and 9 deletions.
8 changes: 4 additions & 4 deletions lib/internal/test_runner/harness.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,8 @@ function setup(root) {
const rejectionHandler =
createProcessEventHandler('unhandledRejection', root);
const coverage = configureCoverage(root, globalOptions);
const exitHandler = async () => {
await root.run(new ERR_TEST_FAILURE(
const exitHandler = () => {
root.postRun(new ERR_TEST_FAILURE(
'Promise resolution is still pending but the event loop has already resolved',
kCancelledByParent));

Expand All @@ -152,8 +152,8 @@ function setup(root) {
process.removeListener('uncaughtException', exceptionHandler);
};

const terminationHandler = async () => {
await exitHandler();
const terminationHandler = () => {
exitHandler();
process.exit();
};

Expand Down
27 changes: 23 additions & 4 deletions lib/internal/test_runner/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -574,7 +574,7 @@ class Test extends AsyncResource {
}
}

async run(pendingSubtestsError) {
async run() {
if (this.parent !== null) {
this.parent.activeSubtests++;
}
Expand Down Expand Up @@ -662,9 +662,16 @@ class Test extends AsyncResource {
}
}

// Clean up the test. Then, try to report the results and execute any
// tests that were pending due to available concurrency.
this.postRun(pendingSubtestsError);
if (this.parent !== null || typeof this.hookType === 'string') {
// Clean up the test. Then, try to report the results and execute any
// tests that were pending due to available concurrency.
//
// The root test is skipped here because it is a special case. Its
// postRun() method is called when the process is getting ready to exit.
// This helps catch any asynchronous activity that occurs after the tests
// have finished executing.
this.postRun();
}
}

postRun(pendingSubtestsError) {
Expand Down Expand Up @@ -706,6 +713,18 @@ class Test extends AsyncResource {
this.parent.addReadySubtest(this);
this.parent.processReadySubtestRange(false);
this.parent.processPendingSubtests();

if (this.parent === this.root &&
this.root.activeSubtests === 0 &&
this.root.pendingSubtests.length === 0 &&
this.root.readySubtests.size === 0 &&
this.root.hooks.after.length > 0) {
// This is done so that any global after() hooks are run. At this point
// all of the tests have finished running. However, there might be
// ref'ed handles keeping the event loop alive. This gives the global
// after() hook a chance to clean them up.
this.root.run();
}
} else if (!this.reported) {
const {
diagnostics,
Expand Down
13 changes: 13 additions & 0 deletions test/fixtures/test-runner/output/async-test-scheduling.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import * as common from '../../../common/index.mjs';
import { describe, test } from 'node:test';
import { setTimeout } from 'node:timers/promises';

test('test', common.mustCall());
describe('suite', common.mustCall(async () => {
test('test', common.mustCall());
await setTimeout(10);
test('scheduled async', common.mustCall());
}));

await setTimeout(10);
test('scheduled async', common.mustCall());
37 changes: 37 additions & 0 deletions test/fixtures/test-runner/output/async-test-scheduling.snapshot
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
TAP version 13
# Subtest: test
ok 1 - test
---
duration_ms: *
...
# Subtest: suite
# Subtest: test
ok 1 - test
---
duration_ms: *
...
# Subtest: scheduled async
ok 2 - scheduled async
---
duration_ms: *
...
1..2
ok 2 - suite
---
duration_ms: *
type: 'suite'
...
# Subtest: scheduled async
ok 3 - scheduled async
---
duration_ms: *
...
1..3
# tests 4
# suites 1
# pass 4
# fail 0
# cancelled 0
# skipped 0
# todo 0
# duration_ms *
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ not ok 2 - /test/fixtures/test-runner/output/global_after_should_fail_the_test.j
*
*
*
*
...
1..1
# tests 1
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-runner-output.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ const tests = [
{ name: 'test-runner/output/unresolved_promise.js' },
{ name: 'test-runner/output/default_output.js', transform: specTransform, tty: true },
{ name: 'test-runner/output/arbitrary-output.js' },
{ name: 'test-runner/output/async-test-scheduling.mjs' },
!skipForceColors ? {
name: 'test-runner/output/arbitrary-output-colored.js',
transform: snapshot.transform(specTransform, replaceTestDuration), tty: true
Expand Down
26 changes: 26 additions & 0 deletions test/parallel/test-runner-root-after-with-refed-handles.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
'use strict';
const common = require('../common');
const { before, after, test } = require('node:test');
const { createServer } = require('node:http');

let server;

before(common.mustCall(() => {
server = createServer();

return new Promise(common.mustCall((resolve, reject) => {
server.listen(0, common.mustCall((err) => {
if (err) {
reject(err);
} else {
resolve();
}
}));
}));
}));

after(common.mustCall(() => {
server.close(common.mustCall());
}));

test();

0 comments on commit b9407ff

Please sign in to comment.