From f57d416b4b404a6cbb00fe4b9471ed5d2ee14621 Mon Sep 17 00:00:00 2001 From: Raz Luvaton <16746759+rluvaton@users.noreply.github.com> Date: Tue, 1 Aug 2023 01:13:02 +0300 Subject: [PATCH] test_runner: fix timeout in *Each hook failing further tests PR-URL: https://github.com/nodejs/node/pull/48925 Reviewed-By: Moshe Atlow Reviewed-By: Chemi Atlow --- lib/internal/test_runner/test.js | 30 ++- .../test-runner/output/abort_hooks.js | 63 ++++++ .../test-runner/output/abort_hooks.snapshot | 188 ++++++++++++++++++ .../test-runner/output/hooks.snapshot | 4 - ...re_each_should_not_affect_further_tests.js | 46 +++++ ...h_should_not_affect_further_tests.snapshot | 67 +++++++ test/parallel/test-runner-output.mjs | 2 + 7 files changed, 385 insertions(+), 15 deletions(-) create mode 100644 test/fixtures/test-runner/output/abort_hooks.js create mode 100644 test/fixtures/test-runner/output/abort_hooks.snapshot create mode 100644 test/fixtures/test-runner/output/timeout_in_before_each_should_not_affect_further_tests.js create mode 100644 test/fixtures/test-runner/output/timeout_in_before_each_should_not_affect_further_tests.snapshot diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index 3d4df848cca293..c3fa0c2ff39721 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -193,8 +193,8 @@ class SuiteContext { } class Test extends AsyncResource { - #abortController; - #outerSignal; + abortController; + outerSignal; #reportedSubtest; constructor(options) { @@ -292,16 +292,16 @@ class Test extends AsyncResource { fn = noop; } - this.#abortController = new AbortController(); - this.#outerSignal = signal; - this.signal = this.#abortController.signal; + this.abortController = new AbortController(); + this.outerSignal = signal; + this.signal = this.abortController.signal; validateAbortSignal(signal, 'options.signal'); if (signal) { kResistStopPropagation ??= require('internal/event_target').kResistStopPropagation; } - this.#outerSignal?.addEventListener( + this.outerSignal?.addEventListener( 'abort', this.#abortHandler, { __proto__: null, [kResistStopPropagation]: true }, @@ -441,7 +441,7 @@ class Test extends AsyncResource { } #abortHandler = () => { - const error = this.#outerSignal?.reason || new AbortError('The test was aborted'); + const error = this.outerSignal?.reason || new AbortError('The test was aborted'); error.failureType = kAborted; this.#cancel(error); }; @@ -459,7 +459,7 @@ class Test extends AsyncResource { ); this.startTime = this.startTime || this.endTime; // If a test was canceled before it was started, e.g inside a hook this.cancelled = true; - this.#abortController.abort(); + this.abortController.abort(); } createHook(name, fn, options) { @@ -527,7 +527,7 @@ class Test extends AsyncResource { if (this.signal.aborted) { return true; } - if (this.#outerSignal?.aborted) { + if (this.outerSignal?.aborted) { this.#abortHandler(); return true; } @@ -639,7 +639,7 @@ class Test extends AsyncResource { // Do not abort hooks and the root test as hooks instance are shared between tests suite so aborting them will // cause them to not run for further tests. if (this.parent !== null) { - this.#abortController.abort(); + this.abortController.abort(); } } @@ -679,7 +679,7 @@ class Test extends AsyncResource { this.fail(new ERR_TEST_FAILURE(msg, kSubtestsFailed)); } - this.#outerSignal?.removeEventListener('abort', this.#abortHandler); + this.outerSignal?.removeEventListener('abort', this.#abortHandler); this.mock?.reset(); if (this.parent !== null) { @@ -795,6 +795,14 @@ class TestHook extends Test { super({ __proto__: null, fn, timeout, signal }); } run(args) { + if (this.error && !this.outerSignal?.aborted) { + this.passed = false; + this.error = null; + this.abortController.abort(); + this.abortController = new AbortController(); + this.signal = this.abortController.signal; + } + this.#args = args; return super.run(); } diff --git a/test/fixtures/test-runner/output/abort_hooks.js b/test/fixtures/test-runner/output/abort_hooks.js new file mode 100644 index 00000000000000..b0f1da80d62719 --- /dev/null +++ b/test/fixtures/test-runner/output/abort_hooks.js @@ -0,0 +1,63 @@ +// Flags: --no-warnings +'use strict'; +const { before, beforeEach, describe, it, after, afterEach } = require('node:test'); + +describe('1 before describe', () => { + const ac = new AbortController(); + before(() => { + console.log('before'); + ac.abort() + }, {signal: ac.signal}); + + it('test 1', () => { + console.log('1.1'); + }); + it('test 2', () => { + console.log('1.2'); + }); +}); + +describe('2 after describe', () => { + const ac = new AbortController(); + after(() => { + console.log('after'); + ac.abort() + }, {signal: ac.signal}); + + it('test 1', () => { + console.log('2.1'); + }); + it('test 2', () => { + console.log('2.2'); + }); +}); + +describe('3 beforeEach describe', () => { + const ac = new AbortController(); + beforeEach(() => { + console.log('beforeEach'); + ac.abort() + }, {signal: ac.signal}); + + it('test 1', () => { + console.log('3.1'); + }); + it('test 2', () => { + console.log('3.2'); + }); +}); + +describe('4 afterEach describe', () => { + const ac = new AbortController(); + afterEach(() => { + console.log('afterEach'); + ac.abort() + }, {signal: ac.signal}); + + it('test 1', () => { + console.log('4.1'); + }); + it('test 2', () => { + console.log('4.2'); + }); +}); diff --git a/test/fixtures/test-runner/output/abort_hooks.snapshot b/test/fixtures/test-runner/output/abort_hooks.snapshot new file mode 100644 index 00000000000000..a1b5ddcd5f1908 --- /dev/null +++ b/test/fixtures/test-runner/output/abort_hooks.snapshot @@ -0,0 +1,188 @@ +before +2.1 +2.2 +after +beforeEach +4.1 +afterEach +4.2 +TAP version 13 +# Subtest: 1 before describe + # Subtest: test 1 + not ok 1 - test 1 + --- + duration_ms: ZERO + failureType: 'cancelledByParent' + error: 'test did not finish before its parent and was cancelled' + code: 'ERR_TEST_FAILURE' + ... + # Subtest: test 2 + not ok 2 - test 2 + --- + duration_ms: ZERO + failureType: 'cancelledByParent' + error: 'test did not finish before its parent and was cancelled' + code: 'ERR_TEST_FAILURE' + ... + 1..2 +not ok 1 - 1 before describe + --- + duration_ms: * + type: 'suite' + failureType: 'hookFailed' + error: 'This operation was aborted' + code: 20 + name: 'AbortError' + stack: |- + * + * + * + * + * + * + * + * + * + * + ... +# Subtest: 2 after describe + # Subtest: test 1 + ok 1 - test 1 + --- + duration_ms: * + ... + # Subtest: test 2 + ok 2 - test 2 + --- + duration_ms: * + ... + 1..2 +not ok 2 - 2 after describe + --- + duration_ms: * + type: 'suite' + failureType: 'hookFailed' + error: 'This operation was aborted' + code: 20 + name: 'AbortError' + stack: |- + * + * + * + * + * + * + * + * + * + * + ... +# Subtest: 3 beforeEach describe + # Subtest: test 1 + not ok 1 - test 1 + --- + duration_ms: * + failureType: 'hookFailed' + error: 'This operation was aborted' + code: 20 + name: 'AbortError' + stack: |- + * + * + * + * + * + * + * + * + * + async Promise.all (index 0) + ... + # Subtest: test 2 + not ok 2 - test 2 + --- + duration_ms: * + failureType: 'hookFailed' + error: 'This operation was aborted' + code: 20 + name: 'AbortError' + stack: |- + * + * + * + * + * + * + * + * + * + async Promise.all (index 0) + ... + 1..2 +not ok 3 - 3 beforeEach describe + --- + duration_ms: * + type: 'suite' + failureType: 'subtestsFailed' + error: '2 subtests failed' + code: 'ERR_TEST_FAILURE' + ... +# Subtest: 4 afterEach describe + # Subtest: test 1 + not ok 1 - test 1 + --- + duration_ms: * + failureType: 'hookFailed' + error: 'This operation was aborted' + code: 20 + name: 'AbortError' + stack: |- + * + * + * + * + * + * + * + * + * + * + ... + # Subtest: test 2 + not ok 2 - test 2 + --- + duration_ms: * + failureType: 'hookFailed' + error: 'This operation was aborted' + code: 20 + name: 'AbortError' + stack: |- + * + * + * + * + * + * + * + * + * + * + ... + 1..2 +not ok 4 - 4 afterEach describe + --- + duration_ms: * + type: 'suite' + failureType: 'subtestsFailed' + error: '2 subtests failed' + code: 'ERR_TEST_FAILURE' + ... +1..4 +# tests 8 +# suites 4 +# pass 2 +# fail 4 +# cancelled 2 +# skipped 0 +# todo 0 +# duration_ms * diff --git a/test/fixtures/test-runner/output/hooks.snapshot b/test/fixtures/test-runner/output/hooks.snapshot index b9fd23640373de..676e1c7a3287e3 100644 --- a/test/fixtures/test-runner/output/hooks.snapshot +++ b/test/fixtures/test-runner/output/hooks.snapshot @@ -134,8 +134,6 @@ not ok 3 - after throws * * * - async Promise.all (index 0) - * * ... 1..2 @@ -183,7 +181,6 @@ not ok 4 - beforeEach throws * * * - async Promise.all (index 0) * ... 1..2 @@ -265,7 +262,6 @@ not ok 6 - afterEach when test fails * * * - async Promise.all (index 0) * ... 1..2 diff --git a/test/fixtures/test-runner/output/timeout_in_before_each_should_not_affect_further_tests.js b/test/fixtures/test-runner/output/timeout_in_before_each_should_not_affect_further_tests.js new file mode 100644 index 00000000000000..6205e2c403fc86 --- /dev/null +++ b/test/fixtures/test-runner/output/timeout_in_before_each_should_not_affect_further_tests.js @@ -0,0 +1,46 @@ +const {describe, test, beforeEach, afterEach} = require("node:test"); +const {setTimeout} = require("timers/promises"); + + +describe('before each timeout', () => { + let i = 0; + + beforeEach(async () => { + if (i++ === 0) { + console.log('gonna timeout'); + await setTimeout(700); + return; + } + console.log('not gonna timeout'); + }, {timeout: 500}); + + test('first describe first test', () => { + console.log('before each test first ' + i); + }); + + test('first describe second test', () => { + console.log('before each test second ' + i); + }); +}); + + +describe('after each timeout', () => { + let i = 0; + + afterEach(async function afterEach1() { + if (i++ === 0) { + console.log('gonna timeout'); + await setTimeout(700); + return; + } + console.log('not gonna timeout'); + }, {timeout: 500}); + + test('second describe first test', () => { + console.log('after each test first ' + i); + }); + + test('second describe second test', () => { + console.log('after each test second ' + i); + }); +}); diff --git a/test/fixtures/test-runner/output/timeout_in_before_each_should_not_affect_further_tests.snapshot b/test/fixtures/test-runner/output/timeout_in_before_each_should_not_affect_further_tests.snapshot new file mode 100644 index 00000000000000..cac7facf893309 --- /dev/null +++ b/test/fixtures/test-runner/output/timeout_in_before_each_should_not_affect_further_tests.snapshot @@ -0,0 +1,67 @@ +gonna timeout +TAP version 13 +not gonna timeout +before each test second 2 +after each test first 0 +gonna timeout +# Subtest: before each timeout + # Subtest: first describe first test + not ok 1 - first describe first test + --- + duration_ms: * + failureType: 'hookFailed' + error: 'failed running beforeEach hook' + code: 'ERR_TEST_FAILURE' + stack: |- + async Promise.all (index 0) + ... + # Subtest: first describe second test + ok 2 - first describe second test + --- + duration_ms: * + ... + 1..2 +not ok 1 - before each timeout + --- + duration_ms: * + type: 'suite' + failureType: 'subtestsFailed' + error: '1 subtest failed' + code: 'ERR_TEST_FAILURE' + ... +after each test second 1 +not gonna timeout +# Subtest: after each timeout + # Subtest: second describe first test + not ok 1 - second describe first test + --- + duration_ms: * + failureType: 'hookFailed' + error: 'failed running afterEach hook' + code: 'ERR_TEST_FAILURE' + stack: |- + async Promise.all (index 0) + ... + # Subtest: second describe second test + ok 2 - second describe second test + --- + duration_ms: * + ... + 1..2 +not ok 2 - after each timeout + --- + duration_ms: * + type: 'suite' + failureType: 'subtestsFailed' + error: '1 subtest failed' + code: 'ERR_TEST_FAILURE' + ... +1..2 +# tests 4 +# suites 2 +# pass 2 +# fail 2 +# cancelled 0 +# skipped 0 +# todo 0 +# duration_ms * diff --git a/test/parallel/test-runner-output.mjs b/test/parallel/test-runner-output.mjs index c4f7ce1d536f73..84fb4b1824dc34 100644 --- a/test/parallel/test-runner-output.mjs +++ b/test/parallel/test-runner-output.mjs @@ -33,9 +33,11 @@ const specTransform = snapshot const tests = [ { name: 'test-runner/output/abort.js' }, { name: 'test-runner/output/abort_suite.js' }, + { name: 'test-runner/output/abort_hooks.js' }, { name: 'test-runner/output/describe_it.js' }, { name: 'test-runner/output/describe_nested.js' }, { name: 'test-runner/output/hooks.js' }, + { name: 'test-runner/output/timeout_in_before_each_should_not_affect_further_tests.js' }, { name: 'test-runner/output/hooks-with-no-global-test.js' }, { name: 'test-runner/output/before-and-after-each-too-many-listeners.js' }, { name: 'test-runner/output/before-and-after-each-with-timeout-too-many-listeners.js' },