From 5d13419dbdac51f365e2493bab8c829508721236 Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Sat, 9 Mar 2024 15:19:08 +0200 Subject: [PATCH] test_runner: run before hook immediately if test started PR-URL: https://github.com/nodejs/node/pull/52003 Reviewed-By: Raz Luvaton Reviewed-By: Chemi Atlow Reviewed-By: Colin Ihrig Reviewed-By: Benjamin Gruenbaum --- lib/internal/test_runner/harness.js | 6 +- lib/internal/test_runner/test.js | 30 ++- .../output/global-hooks-with-no-tests.js | 6 + .../global-hooks-with-no-tests.snapshot | 12 ++ test/fixtures/test-runner/output/hooks.js | 64 ++++++- .../test-runner/output/hooks.snapshot | 179 +++++++++++++++--- .../output/hooks_spec_reporter.snapshot | 171 ++++++++++++++++- test/parallel/test-runner-output.mjs | 1 + 8 files changed, 431 insertions(+), 38 deletions(-) create mode 100644 test/fixtures/test-runner/output/global-hooks-with-no-tests.js create mode 100644 test/fixtures/test-runner/output/global-hooks-with-no-tests.snapshot diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js index 469ca903c7048c..f381ac3ff60a67 100644 --- a/lib/internal/test_runner/harness.js +++ b/lib/internal/test_runner/harness.js @@ -151,7 +151,11 @@ function setup(root) { const rejectionHandler = createProcessEventHandler('unhandledRejection', root); const coverage = configureCoverage(root, globalOptions); - const exitHandler = () => { + 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(); + } root.postRun(new ERR_TEST_FAILURE( 'Promise resolution is still pending but the event loop has already resolved', kCancelledByParent)); diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index 57b58d6320ab42..451032ea801c70 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -177,19 +177,23 @@ class TestContext { } before(fn, options) { - this.#test.createHook('before', fn, options); + this.#test + .createHook('before', fn, { __proto__: null, ...options, hookType: 'before', loc: getCallerLocation() }); } after(fn, options) { - this.#test.createHook('after', fn, options); + this.#test + .createHook('after', fn, { __proto__: null, ...options, hookType: 'after', loc: getCallerLocation() }); } beforeEach(fn, options) { - this.#test.createHook('beforeEach', fn, options); + this.#test + .createHook('beforeEach', fn, { __proto__: null, ...options, hookType: 'beforeEach', loc: getCallerLocation() }); } afterEach(fn, options) { - this.#test.createHook('afterEach', fn, options); + this.#test + .createHook('afterEach', fn, { __proto__: null, ...options, hookType: 'afterEach', loc: getCallerLocation() }); } } @@ -518,6 +522,14 @@ class Test extends AsyncResource { if (name === 'before' || name === 'after') { hook.run = runOnce(hook.run); } + if (name === 'before' && this.startTime !== null) { + // Test has already started, run the hook immediately + PromisePrototypeThen(hook.run(this.getRunArgs()), () => { + if (hook.error != null) { + this.fail(hook.error); + } + }); + } ArrayPrototypePush(this.hooks[name], hook); return hook; } @@ -615,15 +627,16 @@ class Test extends AsyncResource { return; } - const { args, ctx } = this.getRunArgs(); + const hookArgs = this.getRunArgs(); + const { args, ctx } = hookArgs; const after = async () => { if (this.hooks.after.length > 0) { - await this.runHook('after', { __proto__: null, args, ctx }); + await this.runHook('after', hookArgs); } }; const afterEach = runOnce(async () => { if (this.parent?.hooks.afterEach.length > 0) { - await this.parent.runHook('afterEach', { __proto__: null, args, ctx }); + await this.parent.runHook('afterEach', hookArgs); } }); @@ -631,10 +644,11 @@ class Test extends AsyncResource { try { if (this.parent?.hooks.before.length > 0) { + // This hook usually runs immediately, we need to wait for it to finish await this.parent.runHook('before', this.parent.getRunArgs()); } if (this.parent?.hooks.beforeEach.length > 0) { - await this.parent.runHook('beforeEach', { __proto__: null, args, ctx }); + await this.parent.runHook('beforeEach', hookArgs); } stopPromise = stopTest(this.timeout, this.signal); const runArgs = ArrayPrototypeSlice(args); diff --git a/test/fixtures/test-runner/output/global-hooks-with-no-tests.js b/test/fixtures/test-runner/output/global-hooks-with-no-tests.js new file mode 100644 index 00000000000000..e6b5057527799d --- /dev/null +++ b/test/fixtures/test-runner/output/global-hooks-with-no-tests.js @@ -0,0 +1,6 @@ +'use strict'; +const common = require('../../../common'); +const { before, after } = require('node:test'); + +before(common.mustCall(() => console.log('before'))); +after(common.mustCall(() => console.log('after'))); diff --git a/test/fixtures/test-runner/output/global-hooks-with-no-tests.snapshot b/test/fixtures/test-runner/output/global-hooks-with-no-tests.snapshot new file mode 100644 index 00000000000000..9d622d8f3939bf --- /dev/null +++ b/test/fixtures/test-runner/output/global-hooks-with-no-tests.snapshot @@ -0,0 +1,12 @@ +before +TAP version 13 +after +1..0 +# tests 0 +# suites 0 +# pass 0 +# fail 0 +# cancelled 0 +# skipped 0 +# todo 0 +# duration_ms * diff --git a/test/fixtures/test-runner/output/hooks.js b/test/fixtures/test-runner/output/hooks.js index 38de02e1119da4..7bc5ab14bf5557 100644 --- a/test/fixtures/test-runner/output/hooks.js +++ b/test/fixtures/test-runner/output/hooks.js @@ -11,7 +11,7 @@ describe('describe hooks', () => { before(function() { testArr.push('before ' + this.name); }); - after(function() { + after(common.mustCall(function() { testArr.push('after ' + this.name); assert.deepStrictEqual(testArr, [ 'before describe hooks', @@ -23,7 +23,7 @@ describe('describe hooks', () => { 'after nested', 'after describe hooks', ]); - }); + })); beforeEach(function() { testArr.push('beforeEach ' + this.name); }); @@ -52,18 +52,43 @@ describe('describe hooks', () => { }); }); +describe('describe hooks - no subtests', () => { + const testArr = []; + before(function() { + testArr.push('before ' + this.name); + }); + after(common.mustCall(function() { + testArr.push('after ' + this.name); + assert.deepStrictEqual(testArr, [ + 'before describe hooks - no subtests', + 'after describe hooks - no subtests', + ]); + })); + beforeEach(common.mustNotCall()); + afterEach(common.mustNotCall()); +}); + describe('before throws', () => { before(() => { throw new Error('before'); }); it('1', () => {}); test('2', () => {}); }); +describe('before throws - no subtests', () => { + before(() => { throw new Error('before'); }); + after(common.mustCall()); +}); + describe('after throws', () => { after(() => { throw new Error('after'); }); it('1', () => {}); test('2', () => {}); }); +describe('after throws - no subtests', () => { + after(() => { throw new Error('after'); }); +}); + describe('beforeEach throws', () => { beforeEach(() => { throw new Error('beforeEach'); }); it('1', () => {}); @@ -123,6 +148,23 @@ test('test hooks', async (t) => { })); }); + +test('test hooks - no subtests', async (t) => { + const testArr = []; + + t.before((t) => testArr.push('before ' + t.name)); + t.after(common.mustCall((t) => testArr.push('after ' + t.name))); + t.beforeEach(common.mustNotCall()); + t.afterEach(common.mustNotCall()); + + t.after(common.mustCall(() => { + assert.deepStrictEqual(testArr, [ + 'before test hooks - no subtests', + 'after test hooks - no subtests', + ]); + })); +}); + test('t.before throws', async (t) => { t.after(common.mustCall()); t.before(() => { throw new Error('before'); }); @@ -130,6 +172,24 @@ test('t.before throws', async (t) => { await t.test('2', () => {}); }); +test('t.before throws - no subtests', async (t) => { + t.after(common.mustCall()); + t.before(() => { throw new Error('before'); }); +}); + +test('t.after throws', async (t) => { + t.before(common.mustCall()); + t.after(() => { throw new Error('after'); }); + await t.test('1', () => {}); + await t.test('2', () => {}); +}); + +test('t.after throws - no subtests', async (t) => { + t.before(common.mustCall()); + t.after(() => { throw new Error('after'); }); +}); + + test('t.beforeEach throws', async (t) => { t.after(common.mustCall()); t.beforeEach(() => { throw new Error('beforeEach'); }); diff --git a/test/fixtures/test-runner/output/hooks.snapshot b/test/fixtures/test-runner/output/hooks.snapshot index c9c1632e2b9e45..b9fab9e186a8ca 100644 --- a/test/fixtures/test-runner/output/hooks.snapshot +++ b/test/fixtures/test-runner/output/hooks.snapshot @@ -34,6 +34,12 @@ ok 1 - describe hooks duration_ms: * type: 'suite' ... +# Subtest: describe hooks - no subtests +ok 2 - describe hooks - no subtests + --- + duration_ms: * + type: 'suite' + ... # Subtest: before throws # Subtest: 1 not ok 1 - 1 @@ -54,7 +60,27 @@ ok 1 - describe hooks code: 'ERR_TEST_FAILURE' ... 1..2 -not ok 2 - before throws +not ok 3 - before throws + --- + duration_ms: * + type: 'suite' + location: '/test/fixtures/test-runner/output/hooks.js:(LINE):1' + failureType: 'hookFailed' + error: 'before' + code: 'ERR_TEST_FAILURE' + stack: |- + * + * + * + * + * + * + * + * + * + ... +# Subtest: before throws - no subtests +not ok 4 - before throws - no subtests --- duration_ms: * type: 'suite' @@ -85,7 +111,27 @@ not ok 2 - before throws duration_ms: * ... 1..2 -not ok 3 - after throws +not ok 5 - after throws + --- + duration_ms: * + type: 'suite' + location: '/test/fixtures/test-runner/output/hooks.js:(LINE):1' + failureType: 'hookFailed' + error: 'after' + code: 'ERR_TEST_FAILURE' + stack: |- + * + * + * + * + * + * + * + * + * + ... +# Subtest: after throws - no subtests +not ok 6 - after throws - no subtests --- duration_ms: * type: 'suite' @@ -144,7 +190,7 @@ not ok 3 - after throws * ... 1..2 -not ok 4 - beforeEach throws +not ok 7 - beforeEach throws --- duration_ms: * type: 'suite' @@ -194,7 +240,7 @@ not ok 4 - beforeEach throws * ... 1..2 -not ok 5 - afterEach throws +not ok 8 - afterEach throws --- duration_ms: * type: 'suite' @@ -230,7 +276,7 @@ not ok 5 - afterEach throws duration_ms: * ... 1..2 -not ok 6 - afterEach when test fails +not ok 9 - afterEach when test fails --- duration_ms: * type: 'suite' @@ -280,7 +326,7 @@ not ok 6 - afterEach when test fails * ... 1..2 -not ok 7 - afterEach throws and test fails +not ok 10 - afterEach throws and test fails --- duration_ms: * type: 'suite' @@ -317,7 +363,12 @@ not ok 7 - afterEach throws and test fails duration_ms: * ... 1..3 -ok 8 - test hooks +ok 11 - test hooks + --- + duration_ms: * + ... +# Subtest: test hooks - no subtests +ok 12 - test hooks - no subtests --- duration_ms: * ... @@ -363,13 +414,95 @@ ok 8 - test hooks * ... 1..2 -not ok 9 - t.before throws +not ok 13 - t.before throws --- duration_ms: * location: '/test/fixtures/test-runner/output/hooks.js:(LINE):1' - failureType: 'subtestsFailed' - error: '2 subtests failed' + failureType: 'testCodeFailure' + error: 'before' code: 'ERR_TEST_FAILURE' + stack: |- + * + * + * + * + * + * + * + * + * + * + ... +# Subtest: t.before throws - no subtests +not ok 14 - t.before throws - no subtests + --- + duration_ms: * + location: '/test/fixtures/test-runner/output/hooks.js:(LINE):1' + failureType: 'testCodeFailure' + error: 'before' + code: 'ERR_TEST_FAILURE' + stack: |- + * + * + * + * + * + * + * + * + * + * + ... +# Subtest: t.after throws + # Subtest: 1 + ok 1 - 1 + --- + duration_ms: * + ... + # Subtest: 2 + ok 2 - 2 + --- + duration_ms: * + ... + 1..2 +not ok 15 - t.after throws + --- + duration_ms: * + location: '/test/fixtures/test-runner/output/hooks.js:(LINE):1' + failureType: 'hookFailed' + error: 'after' + code: 'ERR_TEST_FAILURE' + stack: |- + * + * + * + * + * + * + * + * + * + * + ... +# Subtest: t.after throws - no subtests +not ok 16 - t.after throws - no subtests + --- + duration_ms: * + location: '/test/fixtures/test-runner/output/hooks.js:(LINE):1' + failureType: 'hookFailed' + error: 'after' + code: 'ERR_TEST_FAILURE' + stack: |- + * + * + * + * + * + * + * + * + * + * ... # Subtest: t.beforeEach throws # Subtest: 1 @@ -413,7 +546,7 @@ not ok 9 - t.before throws * ... 1..2 -not ok 10 - t.beforeEach throws +not ok 17 - t.beforeEach throws --- duration_ms: * location: '/test/fixtures/test-runner/output/hooks.js:(LINE):1' @@ -463,7 +596,7 @@ not ok 10 - t.beforeEach throws * ... 1..2 -not ok 11 - t.afterEach throws +not ok 18 - t.afterEach throws --- duration_ms: * location: '/test/fixtures/test-runner/output/hooks.js:(LINE):1' @@ -497,7 +630,7 @@ not ok 11 - t.afterEach throws duration_ms: * ... 1..2 -not ok 12 - afterEach when test fails +not ok 19 - afterEach when test fails --- duration_ms: * location: '/test/fixtures/test-runner/output/hooks.js:(LINE):1' @@ -512,7 +645,7 @@ not ok 12 - afterEach when test fails duration_ms: * ... 1..1 -ok 13 - afterEach context when test passes +ok 20 - afterEach context when test passes --- duration_ms: * ... @@ -532,7 +665,7 @@ ok 13 - afterEach context when test passes * ... 1..1 -not ok 14 - afterEach context when test fails +not ok 21 - afterEach context when test fails --- duration_ms: * location: '/test/fixtures/test-runner/output/hooks.js:(LINE):1' @@ -581,7 +714,7 @@ not ok 14 - afterEach context when test fails * ... 1..2 -not ok 15 - afterEach throws and test fails +not ok 22 - afterEach throws and test fails --- duration_ms: * location: '/test/fixtures/test-runner/output/hooks.js:(LINE):1' @@ -590,7 +723,7 @@ not ok 15 - afterEach throws and test fails code: 'ERR_TEST_FAILURE' ... # Subtest: t.after() is called if test body throws -not ok 16 - t.after() is called if test body throws +not ok 23 - t.after() is called if test body throws --- duration_ms: * location: '/test/fixtures/test-runner/output/hooks.js:(LINE):1' @@ -615,7 +748,7 @@ not ok 16 - t.after() is called if test body throws code: 'ERR_TEST_FAILURE' ... 1..1 -not ok 17 - run after when before throws +not ok 24 - run after when before throws --- duration_ms: * type: 'suite' @@ -634,15 +767,15 @@ not ok 17 - run after when before throws * * ... -1..17 +1..24 # before 1 called # before 2 called # after 1 called # after 2 called -# tests 43 -# suites 9 -# pass 16 -# fail 24 +# tests 49 +# suites 12 +# pass 19 +# fail 27 # cancelled 3 # skipped 0 # todo 0 diff --git a/test/fixtures/test-runner/output/hooks_spec_reporter.snapshot b/test/fixtures/test-runner/output/hooks_spec_reporter.snapshot index c9b749d571674f..4158335409aba1 100644 --- a/test/fixtures/test-runner/output/hooks_spec_reporter.snapshot +++ b/test/fixtures/test-runner/output/hooks_spec_reporter.snapshot @@ -9,6 +9,7 @@ describe hooks (*ms) + describe hooks - no subtests (*ms) before throws 1 'test did not finish before its parent and was cancelled' @@ -29,6 +30,18 @@ * * + before throws - no subtests (*ms) + Error: before + * + * + * + * + * + * + * + * + * + after throws 1 (*ms) 2 (*ms) @@ -45,6 +58,18 @@ * * + after throws - no subtests (*ms) + Error: after + * + * + * + * + * + * + * + * + * + beforeEach throws 1 (*ms) Error: beforeEach @@ -155,6 +180,7 @@ test hooks (*ms) + test hooks - no subtests (*ms) t.before throws 1 (*ms) Error: before @@ -184,6 +210,61 @@ t.before throws (*ms) + Error: before + * + * + * + * + * + * + * + * + * + * + + t.before throws - no subtests (*ms) + Error: before + * + * + * + * + * + * + * + * + * + * + + t.after throws + 1 (*ms) + 2 (*ms) + t.after throws (*ms) + + Error: after + * + * + * + * + * + * + * + * + * + * + + t.after throws - no subtests (*ms) + Error: after + * + * + * + * + * + * + * + * + * + * + t.beforeEach throws 1 (*ms) Error: beforeEach @@ -329,10 +410,10 @@ before 2 called after 1 called after 2 called - tests 43 - suites 9 - pass 16 - fail 24 + tests 49 + suites 12 + pass 19 + fail 27 cancelled 3 skipped 0 todo 0 @@ -361,6 +442,19 @@ * * +* + before throws - no subtests (*ms) + Error: before + * + * + * + * + * + * + * + * + * + * after throws (*ms) Error: after @@ -374,6 +468,19 @@ * * +* + after throws - no subtests (*ms) + Error: after + * + * + * + * + * + * + * + * + * + * 1 (*ms) Error: beforeEach @@ -496,6 +603,62 @@ * * +* + t.before throws (*ms) + Error: before + * + * + * + * + * + * + * + * + * + * + +* + t.before throws - no subtests (*ms) + Error: before + * + * + * + * + * + * + * + * + * + * + +* + t.after throws (*ms) + Error: after + * + * + * + * + * + * + * + * + * + * + +* + t.after throws - no subtests (*ms) + Error: after + * + * + * + * + * + * + * + * + * + * + * 1 (*ms) Error: beforeEach diff --git a/test/parallel/test-runner-output.mjs b/test/parallel/test-runner-output.mjs index e40ce896bff465..6faf8e41106d29 100644 --- a/test/parallel/test-runner-output.mjs +++ b/test/parallel/test-runner-output.mjs @@ -95,6 +95,7 @@ const tests = [ { name: 'test-runner/output/hooks_spec_reporter.js', transform: specTransform }, { 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/global-hooks-with-no-tests.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' }, { name: 'test-runner/output/global_after_should_fail_the_test.js' },