From 3ae2015cf71a847186c7d6996f873a1040499c5f Mon Sep 17 00:00:00 2001 From: Phil Nash Date: Tue, 9 May 2023 17:05:04 +1000 Subject: [PATCH 1/3] test_runner: fix ordering of test hooks For tests with subtests the before hook was being run after the beforeEach hook, which is the opposite to test suites and expectations. Also, a function was being used to close over the after hooks, but at the point it was being run the after hooks were not yet set up. Fixes #47915 --- lib/internal/test_runner/test.js | 23 ++++++++++++----------- test/fixtures/test-runner/output/hooks.js | 7 ++++++- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index 1f888bc6277e27..ee780e9a8e950c 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -477,7 +477,6 @@ class Test extends AsyncResource { this.parent.addPendingSubtest(deferred); return deferred.promise; } - return this.run(); } @@ -525,11 +524,7 @@ class Test extends AsyncResource { } const { args, ctx } = this.getRunArgs(); - const after = runOnce(async () => { - if (this.hooks.after.length > 0) { - await this.runHook('after', { args, ctx }); - } - }); + const afterEach = runOnce(async () => { if (this.parent?.hooks.afterEach.length > 0) { await this.parent.runHook('afterEach', { args, ctx }); @@ -537,12 +532,12 @@ class Test extends AsyncResource { }); try { - if (this.parent?.hooks.beforeEach.length > 0) { - await this.parent.runHook('beforeEach', { args, ctx }); - } if (this.parent?.hooks.before.length > 0) { await this.parent.runHook('before', this.parent.getRunArgs()); } + if (this.parent?.hooks.beforeEach.length > 0) { + await this.parent.runHook('beforeEach', { args, ctx }); + } const stopPromise = stopTest(this.timeout, this.signal); const runArgs = ArrayPrototypeSlice(args); ArrayPrototypeUnshift(runArgs, this.fn, ctx); @@ -574,11 +569,17 @@ class Test extends AsyncResource { return; } - await after(); await afterEach(); + if (this.hooks.after.length > 0) { + await this.runHook('after', this.getRunArgs()); + } this.pass(); } catch (err) { - try { await after(); } catch { /* Ignore error. */ } + try { + if (this.hooks.after.length > 0) { + await this.runHook('after', this.getRunArgs()); + } + } catch { /* Ignore error. */ } try { await afterEach(); } catch { /* test is already failing, let's ignore the error */ } if (isTestFailureError(err)) { if (err.failureType === kTestTimeoutFailure) { diff --git a/test/fixtures/test-runner/output/hooks.js b/test/fixtures/test-runner/output/hooks.js index 30532a29ad69f4..a69506bbda5ef7 100644 --- a/test/fixtures/test-runner/output/hooks.js +++ b/test/fixtures/test-runner/output/hooks.js @@ -99,6 +99,8 @@ test('test hooks', async (t) => { await t.test('2', () => testArr.push('2')); await t.test('nested', async (t) => { + t.before((t) => testArr.push('nested before ' + t.name)); + t.after((t) => testArr.push('nested after ' + t.name)); t.beforeEach((t) => testArr.push('nested beforeEach ' + t.name)); t.afterEach((t) => testArr.push('nested afterEach ' + t.name)); await t.test('nested 1', () => testArr.push('nested1')); @@ -106,12 +108,15 @@ test('test hooks', async (t) => { }); assert.deepStrictEqual(testArr, [ - 'beforeEach 1', 'before test hooks', '1', 'afterEach 1', + 'before test hooks', + 'beforeEach 1', '1', 'afterEach 1', 'beforeEach 2', '2', 'afterEach 2', 'beforeEach nested', + 'nested before nested', 'beforeEach nested 1', 'nested beforeEach nested 1', 'nested1', 'afterEach nested 1', 'nested afterEach nested 1', 'beforeEach nested 2', 'nested beforeEach nested 2', 'nested 2', 'afterEach nested 2', 'nested afterEach nested 2', 'afterEach nested', + 'nested after nested', ]); }); From ddb7dd7ca3194d7709a285aaeb7be3ba54dad505 Mon Sep 17 00:00:00 2001 From: Phil Nash Date: Wed, 10 May 2023 09:09:00 +1000 Subject: [PATCH 2/3] test_runner: call after hook using runOnce --- lib/internal/test_runner/test.js | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index ee780e9a8e950c..ae9ad7e355ae7c 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -530,6 +530,11 @@ class Test extends AsyncResource { await this.parent.runHook('afterEach', { args, ctx }); } }); + const after = runOnce(async () => { + if (this.hooks.after.length > 0) { + await this.runHook('after', { args, ctx }); + } + }); try { if (this.parent?.hooks.before.length > 0) { @@ -570,16 +575,10 @@ class Test extends AsyncResource { } await afterEach(); - if (this.hooks.after.length > 0) { - await this.runHook('after', this.getRunArgs()); - } + await after(); this.pass(); } catch (err) { - try { - if (this.hooks.after.length > 0) { - await this.runHook('after', this.getRunArgs()); - } - } catch { /* Ignore error. */ } + try { await after(); } catch { /* Ignore error. */ } try { await afterEach(); } catch { /* test is already failing, let's ignore the error */ } if (isTestFailureError(err)) { if (err.failureType === kTestTimeoutFailure) { From 070e9c5b2aa740dd7fbd5525fd5e6359566f25ed Mon Sep 17 00:00:00 2001 From: Phil Nash Date: Thu, 11 May 2023 08:57:17 +1000 Subject: [PATCH 3/3] test_runner: undo unnecessary changes --- lib/internal/test_runner/test.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js index ae9ad7e355ae7c..083cc441ae314c 100644 --- a/lib/internal/test_runner/test.js +++ b/lib/internal/test_runner/test.js @@ -477,6 +477,7 @@ class Test extends AsyncResource { this.parent.addPendingSubtest(deferred); return deferred.promise; } + return this.run(); } @@ -524,17 +525,16 @@ class Test extends AsyncResource { } const { args, ctx } = this.getRunArgs(); - - const afterEach = runOnce(async () => { - if (this.parent?.hooks.afterEach.length > 0) { - await this.parent.runHook('afterEach', { args, ctx }); - } - }); const after = runOnce(async () => { if (this.hooks.after.length > 0) { await this.runHook('after', { args, ctx }); } }); + const afterEach = runOnce(async () => { + if (this.parent?.hooks.afterEach.length > 0) { + await this.parent.runHook('afterEach', { args, ctx }); + } + }); try { if (this.parent?.hooks.before.length > 0) {