From 7a0c74b4eadca323a376abc9daa2c7d1cb76ef91 Mon Sep 17 00:00:00 2001 From: Romain Menke Date: Sun, 11 May 2025 19:19:04 +0200 Subject: [PATCH] Revert "test_runner: automatically wait for subtests to finish" This reverts commit aa3523ec22c95dfb11ef93c8333ced41cd431f89. PR-URL: https://github.com/nodejs/node/pull/58282 Fixes: https://github.com/nodejs/node/issues/58227 Reviewed-By: Matteo Collina Reviewed-By: LiviaMedeiros --- lib/internal/test_runner/harness.js | 17 +++------------- lib/internal/test_runner/test.js | 13 ------------ .../output/default_output.snapshot | 10 +++++++--- .../test-runner/output/dot_reporter.snapshot | 6 +++++- .../output/junit_reporter.snapshot | 14 ++++++++----- .../test-runner/output/no_refs.snapshot | 20 +++++++++++++++---- .../test-runner/output/output.snapshot | 18 ++++++++++++----- .../test-runner/output/output_cli.snapshot | 18 ++++++++++++----- .../test-runner/output/spec_reporter.snapshot | 10 +++++++--- .../output/spec_reporter_cli.snapshot | 10 +++++++--- test/parallel/test-runner-output.mjs | 4 ++++ 11 files changed, 84 insertions(+), 56 deletions(-) diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js index 9774326a42..518bd9b23b 100644 --- a/lib/internal/test_runner/harness.js +++ b/lib/internal/test_runner/harness.js @@ -28,8 +28,6 @@ const { setupGlobalSetupTeardownFunctions, } = require('internal/test_runner/utils'); const { queueMicrotask } = require('internal/process/task_queues'); -const { TIMEOUT_MAX } = require('internal/timers'); -const { clearInterval, setInterval } = require('timers'); const { bigint: hrtime } = process.hrtime; const testResources = new SafeMap(); let globalRoot; @@ -230,20 +228,11 @@ function setupProcessState(root, globalOptions) { const rejectionHandler = createProcessEventHandler('unhandledRejection', root); const coverage = configureCoverage(root, globalOptions); - const exitHandler = async (kill) => { + const exitHandler = async () => { if (root.subtests.length === 0 && (root.hooks.before.length > 0 || root.hooks.after.length > 0)) { // Run global before/after hooks in case there are no tests await root.run(); } - - if (kill !== true && root.subtestsPromise !== null) { - // Wait for all subtests to finish, but keep the process alive in case - // there are no ref'ed handles left. - const keepAlive = setInterval(() => {}, TIMEOUT_MAX); - await root.subtestsPromise.promise; - clearInterval(keepAlive); - } - root.postRun(new ERR_TEST_FAILURE( 'Promise resolution is still pending but the event loop has already resolved', kCancelledByParent)); @@ -263,8 +252,8 @@ function setupProcessState(root, globalOptions) { } }; - const terminationHandler = async () => { - await exitHandler(true); + const terminationHandler = () => { + exitHandler(); process.exit(); }; diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index 5b6a202761..245b3c0979 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -651,8 +651,6 @@ class Test extends AsyncResource { this.activeSubtests = 0; this.pendingSubtests = []; this.readySubtests = new SafeMap(); - this.unfinishedSubtests = new SafeSet(); - this.subtestsPromise = null; this.subtests = []; this.waitingOn = 0; this.finished = false; @@ -756,11 +754,6 @@ class Test extends AsyncResource { addReadySubtest(subtest) { this.readySubtests.set(subtest.childNumber, subtest); - - if (this.unfinishedSubtests.delete(subtest) && - this.unfinishedSubtests.size === 0) { - this.subtestsPromise.resolve(); - } } processReadySubtestRange(canSend) { @@ -822,7 +815,6 @@ class Test extends AsyncResource { if (parent.waitingOn === 0) { parent.waitingOn = test.childNumber; - parent.subtestsPromise = PromiseWithResolvers(); } if (preventAddingSubtests) { @@ -945,7 +937,6 @@ class Test extends AsyncResource { // If there is enough available concurrency to run the test now, then do // it. Otherwise, return a Promise to the caller and mark the test as // pending for later execution. - this.parent.unfinishedSubtests.add(this); this.reporter.enqueue(this.nesting, this.loc, this.name, this.reportedType); if (this.root.harness.buildPromise || !this.parent.hasConcurrency()) { const deferred = PromiseWithResolvers(); @@ -1070,10 +1061,6 @@ class Test extends AsyncResource { this[kShouldAbort](); - if (this.subtestsPromise !== null) { - await SafePromiseRace([this.subtestsPromise.promise, stopPromise]); - } - if (this.plan !== null) { const planPromise = this.plan?.check(); // If the plan returns a promise, it means that it is waiting for more assertions to be made before diff --git a/test/fixtures/test-runner/output/default_output.snapshot b/test/fixtures/test-runner/output/default_output.snapshot index a58e14346e..d0a8339573 100644 --- a/test/fixtures/test-runner/output/default_output.snapshot +++ b/test/fixtures/test-runner/output/default_output.snapshot @@ -3,13 +3,13 @@ [90m﹣ should skip [90m(*ms)[39m # SKIP[39m ▶ parent [31m✖ should fail [90m(*ms)[39m[39m - [32m✔ should pass but parent fail [90m(*ms)[39m[39m + [31m✖ should pass but parent fail [90m(*ms)[39m[39m [31m✖ parent [90m(*ms)[39m[39m [34mℹ tests 6[39m [34mℹ suites 0[39m -[34mℹ pass 2[39m +[34mℹ pass 1[39m [34mℹ fail 3[39m -[34mℹ cancelled 0[39m +[34mℹ cancelled 1[39m [34mℹ skipped 1[39m [34mℹ todo 0[39m [34mℹ duration_ms *[39m @@ -40,3 +40,7 @@ *[39m *[39m *[39m + +* +[31m✖ should pass but parent fail [90m(*ms)[39m[39m + [32m'test did not finish before its parent and was cancelled'[39m diff --git a/test/fixtures/test-runner/output/dot_reporter.snapshot b/test/fixtures/test-runner/output/dot_reporter.snapshot index 5abbb97966..533622f95d 100644 --- a/test/fixtures/test-runner/output/dot_reporter.snapshot +++ b/test/fixtures/test-runner/output/dot_reporter.snapshot @@ -1,5 +1,5 @@ ..XX...X..XXX.X..... -XXX............X.... +XXX.....X..X...X.... .....X...XXX.XX..... XXXXXXX...XXXXX @@ -93,6 +93,10 @@ Failed tests: '1 subtest failed' ✖ sync throw non-error fail (*ms) Symbol(thrown symbol from sync throw non-error fail) +✖ +long running (*ms) + 'test did not finish before its parent and was cancelled' +✖ top level (*ms) + '1 subtest failed' ✖ sync skip option is false fail (*ms) Error: this should be executed * diff --git a/test/fixtures/test-runner/output/junit_reporter.snapshot b/test/fixtures/test-runner/output/junit_reporter.snapshot index 3b1d15022a..9084fdf45d 100644 --- a/test/fixtures/test-runner/output/junit_reporter.snapshot +++ b/test/fixtures/test-runner/output/junit_reporter.snapshot @@ -186,8 +186,12 @@ Error [ERR_TEST_FAILURE]: thrown from subtest sync throw fail - - + + + +[Error [ERR_TEST_FAILURE]: test did not finish before its parent and was cancelled] { code: 'ERR_TEST_FAILURE', failureType: 'cancelledByParent', cause: 'test did not finish before its parent and was cancelled' } + + @@ -512,9 +516,9 @@ Error [ERR_TEST_FAILURE]: test could not be started because its parent finished - - - + + + diff --git a/test/fixtures/test-runner/output/no_refs.snapshot b/test/fixtures/test-runner/output/no_refs.snapshot index 8014b03438..310094947f 100644 --- a/test/fixtures/test-runner/output/no_refs.snapshot +++ b/test/fixtures/test-runner/output/no_refs.snapshot @@ -1,23 +1,35 @@ TAP version 13 # Subtest: does not keep event loop alive # Subtest: +does not keep event loop alive - ok 1 - +does not keep event loop alive + not ok 1 - +does not keep event loop alive --- duration_ms: * type: 'test' + location: '/test/fixtures/test-runner/output/no_refs.js:(LINE):11' + failureType: 'cancelledByParent' + error: 'Promise resolution is still pending but the event loop has already resolved' + code: 'ERR_TEST_FAILURE' + stack: |- + * ... 1..1 -ok 1 - does not keep event loop alive +not ok 1 - does not keep event loop alive --- duration_ms: * type: 'test' + location: '/test/fixtures/test-runner/output/no_refs.js:(LINE):1' + failureType: 'cancelledByParent' + error: 'Promise resolution is still pending but the event loop has already resolved' + code: 'ERR_TEST_FAILURE' + stack: |- + * ... 1..1 # tests 2 # suites 0 -# pass 2 +# pass 0 # fail 0 -# cancelled 0 +# cancelled 2 # skipped 0 # todo 0 # duration_ms * diff --git a/test/fixtures/test-runner/output/output.snapshot b/test/fixtures/test-runner/output/output.snapshot index ffbe91759b..cf464c09a0 100644 --- a/test/fixtures/test-runner/output/output.snapshot +++ b/test/fixtures/test-runner/output/output.snapshot @@ -288,10 +288,14 @@ ok 23 - level 0a ... # Subtest: top level # Subtest: +long running - ok 1 - +long running + not ok 1 - +long running --- duration_ms: * type: 'test' + location: '/test/fixtures/test-runner/output/output.js:(LINE):5' + failureType: 'cancelledByParent' + error: 'test did not finish before its parent and was cancelled' + code: 'ERR_TEST_FAILURE' ... # Subtest: +short running # Subtest: ++short running @@ -307,10 +311,14 @@ ok 23 - level 0a type: 'test' ... 1..2 -ok 24 - top level +not ok 24 - top level --- duration_ms: * type: 'test' + location: '/test/fixtures/test-runner/output/output.js:(LINE):1' + failureType: 'subtestsFailed' + error: '1 subtest failed' + code: 'ERR_TEST_FAILURE' ... # Subtest: invalid subtest - pass but subtest fails ok 25 - invalid subtest - pass but subtest fails @@ -777,9 +785,9 @@ not ok 62 - invalid subtest fail # Error: Test "callback async throw after done" at test/fixtures/test-runner/output/output.js:(LINE):1 generated asynchronous activity after the test ended. This activity created the error "Error: thrown from callback async throw after done" and would have caused the test to fail, but instead triggered an uncaughtException event. # tests 75 # suites 0 -# pass 36 -# fail 24 -# cancelled 2 +# pass 34 +# fail 25 +# cancelled 3 # skipped 9 # todo 4 # duration_ms * diff --git a/test/fixtures/test-runner/output/output_cli.snapshot b/test/fixtures/test-runner/output/output_cli.snapshot index 7f989f14c6..9fad5ba240 100644 --- a/test/fixtures/test-runner/output/output_cli.snapshot +++ b/test/fixtures/test-runner/output/output_cli.snapshot @@ -288,10 +288,14 @@ ok 23 - level 0a ... # Subtest: top level # Subtest: +long running - ok 1 - +long running + not ok 1 - +long running --- duration_ms: * type: 'test' + location: '/test/fixtures/test-runner/output/output.js:(LINE):5' + failureType: 'cancelledByParent' + error: 'test did not finish before its parent and was cancelled' + code: 'ERR_TEST_FAILURE' ... # Subtest: +short running # Subtest: ++short running @@ -307,10 +311,14 @@ ok 23 - level 0a type: 'test' ... 1..2 -ok 24 - top level +not ok 24 - top level --- duration_ms: * type: 'test' + location: '/test/fixtures/test-runner/output/output.js:(LINE):1' + failureType: 'subtestsFailed' + error: '1 subtest failed' + code: 'ERR_TEST_FAILURE' ... # Subtest: invalid subtest - pass but subtest fails ok 25 - invalid subtest - pass but subtest fails @@ -791,9 +799,9 @@ ok 63 - last test 1..63 # tests 77 # suites 0 -# pass 38 -# fail 24 -# cancelled 2 +# pass 36 +# fail 25 +# cancelled 3 # skipped 9 # todo 4 # duration_ms * diff --git a/test/fixtures/test-runner/output/spec_reporter.snapshot b/test/fixtures/test-runner/output/spec_reporter.snapshot index 6c11b9ba6d..7eedb8b170 100644 --- a/test/fixtures/test-runner/output/spec_reporter.snapshot +++ b/test/fixtures/test-runner/output/spec_reporter.snapshot @@ -90,9 +90,9 @@ Error: Test "callback async throw after done" at test/fixtures/test-runner/output/output.js:269:1 generated asynchronous activity after the test ended. This activity created the error "Error: thrown from callback async throw after done" and would have caused the test to fail, but instead triggered an uncaughtException event. tests 75 suites 0 - pass 36 - fail 24 - cancelled 2 + pass 34 + fail 25 + cancelled 3 skipped 9 todo 4 duration_ms * @@ -203,6 +203,10 @@ sync throw non-error fail (*ms) Symbol(thrown symbol from sync throw non-error fail) +* + +long running (*ms) + 'test did not finish before its parent and was cancelled' + * sync skip option is false fail (*ms) Error: this should be executed diff --git a/test/fixtures/test-runner/output/spec_reporter_cli.snapshot b/test/fixtures/test-runner/output/spec_reporter_cli.snapshot index a428b1140a..6a0c7e381c 100644 --- a/test/fixtures/test-runner/output/spec_reporter_cli.snapshot +++ b/test/fixtures/test-runner/output/spec_reporter_cli.snapshot @@ -93,9 +93,9 @@ Error: Test "callback async throw after done" at test/fixtures/test-runner/output/output.js:269:1 generated asynchronous activity after the test ended. This activity created the error "Error: thrown from callback async throw after done" and would have caused the test to fail, but instead triggered an uncaughtException event. tests 76 suites 0 - pass 37 - fail 24 - cancelled 2 + pass 35 + fail 25 + cancelled 3 skipped 9 todo 4 duration_ms * @@ -206,6 +206,10 @@ sync throw non-error fail (*ms) Symbol(thrown symbol from sync throw non-error fail) +* + +long running (*ms) + 'test did not finish before its parent and was cancelled' + * sync skip option is false fail (*ms) Error: this should be executed diff --git a/test/parallel/test-runner-output.mjs b/test/parallel/test-runner-output.mjs index a9ee2e36ee..90251137bc 100644 --- a/test/parallel/test-runner-output.mjs +++ b/test/parallel/test-runner-output.mjs @@ -208,6 +208,10 @@ const tests = [ name: 'test-runner/output/unfinished-suite-async-error.js', flags: ['--test-reporter=tap'], }, + { + name: 'test-runner/output/unresolved_promise.js', + flags: ['--test-reporter=tap'], + }, { name: 'test-runner/output/default_output.js', transform: specTransform, tty: true }, { name: 'test-runner/output/arbitrary-output.js',