From 6346bdc526eddefea72ed32e9ec9755cba3fa706 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Mon, 7 Aug 2023 17:59:10 -0400 Subject: [PATCH] test_runner: run global after() hook earlier This commit moves the global after() hook execution from the 'beforeExit' event to the point where all tests have finished running. This gives the global after() a chance to clean up handles that would otherwise prevent the 'beforeExit' event from being emitted. PR-URL: https://github.com/nodejs/node/pull/49059 Fixes: https://github.com/nodejs/node/issues/49056 Reviewed-By: Chemi Atlow Reviewed-By: Moshe Atlow Reviewed-By: Matteo Collina 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 4eb6458b23e47d..357347627fcc2b 100644 --- a/lib/internal/test_runner/harness.js +++ b/lib/internal/test_runner/harness.js @@ -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)); @@ -152,8 +152,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..f8c9087f6cdef3 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) { + // 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..c6b205602f705f --- /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(); +})); + +test();