From d83964031b5f9ca9c46c343cc4d65598144b9d75 Mon Sep 17 00:00:00 2001 From: Colin Ihrig Date: Mon, 14 Aug 2023 10:29:29 -0400 Subject: [PATCH] test_runner: reland run global after() hook earlier This commit reverts the revert in bb52656fc627e4f48a0f706756873b593d81372a. It also includes the fix for the issue that required the revert (https://github.com/nodejs/node/pull/49059#issuecomment-1675171959) and an additional common.mustCall() in the added test. Refs: https://github.com/nodejs/node/pull/49059 Refs: https://github.com/nodejs/node/pull/49110 PR-URL: https://github.com/nodejs/node/pull/49116 Reviewed-By: Matteo Collina Reviewed-By: Chemi Atlow Reviewed-By: Moshe Atlow Reviewed-By: Benjamin Gruenbaum --- lib/internal/test_runner/harness.js | 8 ++-- lib/internal/test_runner/test.js | 27 ++++++++++++-- .../output/async-test-scheduling.mjs | 13 +++++++ .../output/async-test-scheduling.snapshot | 37 +++++++++++++++++++ ...global_after_should_fail_the_test.snapshot | 1 - test/parallel/test-runner-output.mjs | 1 + ...st-runner-root-after-with-refed-handles.js | 26 +++++++++++++ 7 files changed, 104 insertions(+), 9 deletions(-) create mode 100644 test/fixtures/test-runner/output/async-test-scheduling.mjs create mode 100644 test/fixtures/test-runner/output/async-test-scheduling.snapshot create mode 100644 test/parallel/test-runner-root-after-with-refed-handles.js diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js index c0a1b70870b7b4..3ebf4769753c5d 100644 --- a/lib/internal/test_runner/harness.js +++ b/lib/internal/test_runner/harness.js @@ -140,8 +140,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)); @@ -150,8 +150,8 @@ function setup(root) { process.removeListener('uncaughtException', exceptionHandler); }; - const terminationHandler = async () => { - await exitHandler(); + const terminationHandler = () => { + exitHandler(); process.exit(); }; diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index 58f1de711f38f4..975ad4ac08b41f 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -574,7 +574,7 @@ class Test extends AsyncResource { } } - async run(pendingSubtestsError) { + async run() { if (this.parent !== null) { this.parent.activeSubtests++; } @@ -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) { @@ -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, diff --git a/test/fixtures/test-runner/output/async-test-scheduling.mjs b/test/fixtures/test-runner/output/async-test-scheduling.mjs new file mode 100644 index 00000000000000..7c7a9f91208911 --- /dev/null +++ b/test/fixtures/test-runner/output/async-test-scheduling.mjs @@ -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()); diff --git a/test/fixtures/test-runner/output/async-test-scheduling.snapshot b/test/fixtures/test-runner/output/async-test-scheduling.snapshot new file mode 100644 index 00000000000000..64c3004d26881d --- /dev/null +++ b/test/fixtures/test-runner/output/async-test-scheduling.snapshot @@ -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 * diff --git a/test/fixtures/test-runner/output/global_after_should_fail_the_test.snapshot b/test/fixtures/test-runner/output/global_after_should_fail_the_test.snapshot index 845aba58eddd32..3196f377b3d4bf 100644 --- a/test/fixtures/test-runner/output/global_after_should_fail_the_test.snapshot +++ b/test/fixtures/test-runner/output/global_after_should_fail_the_test.snapshot @@ -22,7 +22,6 @@ not ok 2 - /test/fixtures/test-runner/output/global_after_should_fail_the_test.j * * * - * ... 1..1 # tests 1 diff --git a/test/parallel/test-runner-output.mjs b/test/parallel/test-runner-output.mjs index 85d3131490a3db..8db41bff38a114 100644 --- a/test/parallel/test-runner-output.mjs +++ b/test/parallel/test-runner-output.mjs @@ -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 diff --git a/test/parallel/test-runner-root-after-with-refed-handles.js b/test/parallel/test-runner-root-after-with-refed-handles.js new file mode 100644 index 00000000000000..2149c2dba236cf --- /dev/null +++ b/test/parallel/test-runner-root-after-with-refed-handles.js @@ -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();