From bb52656fc627e4f48a0f706756873b593d81372a Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 12 Aug 2023 06:19:05 +0200 Subject: [PATCH] Revert "test_runner: run global after() hook earlier" This reverts commit 6346bdc526eddefea72ed32e9ec9755cba3fa706. Reason for revert: breaking CI PR-URL: https://github.com/nodejs/node/pull/49110 Reviewed-By: Colin Ihrig Reviewed-By: Yagiz Nizipli Reviewed-By: LiviaMedeiros Reviewed-By: Debadree Chatterjee Reviewed-By: Benjamin Gruenbaum Reviewed-By: Luigi Pinca --- 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, 9 insertions(+), 104 deletions(-) delete mode 100644 test/fixtures/test-runner/output/async-test-scheduling.mjs delete mode 100644 test/fixtures/test-runner/output/async-test-scheduling.snapshot delete 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 357347627fc..4eb6458b23e 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 = () => { - root.postRun(new ERR_TEST_FAILURE( + const exitHandler = async () => { + await root.run(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 = () => { - exitHandler(); + const terminationHandler = async () => { + await exitHandler(); process.exit(); }; diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index f8c9087f6cd..58f1de711f3 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() { + async run(pendingSubtestsError) { if (this.parent !== null) { this.parent.activeSubtests++; } @@ -662,16 +662,9 @@ class Test extends AsyncResource { } } - 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(); - } + // Clean up the test. Then, try to report the results and execute any + // tests that were pending due to available concurrency. + this.postRun(pendingSubtestsError); } postRun(pendingSubtestsError) { @@ -713,18 +706,6 @@ 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 deleted file mode 100644 index 7c7a9f91208..00000000000 --- a/test/fixtures/test-runner/output/async-test-scheduling.mjs +++ /dev/null @@ -1,13 +0,0 @@ -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 deleted file mode 100644 index 64c3004d268..00000000000 --- a/test/fixtures/test-runner/output/async-test-scheduling.snapshot +++ /dev/null @@ -1,37 +0,0 @@ -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 3196f377b3d..845aba58edd 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,6 +22,7 @@ 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 8db41bff38a..85d3131490a 100644 --- a/test/parallel/test-runner-output.mjs +++ b/test/parallel/test-runner-output.mjs @@ -74,7 +74,6 @@ 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 deleted file mode 100644 index c6b205602f7..00000000000 --- a/test/parallel/test-runner-root-after-with-refed-handles.js +++ /dev/null @@ -1,26 +0,0 @@ -'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();