Skip to content

Commit

Permalink
test_runner: fix ordering of test hooks
Browse files Browse the repository at this point in the history
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 nodejs#47915
  • Loading branch information
philnash committed May 9, 2023
1 parent 0b3fcfc commit 3ae2015
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 12 deletions.
23 changes: 12 additions & 11 deletions lib/internal/test_runner/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,6 @@ class Test extends AsyncResource {
this.parent.addPendingSubtest(deferred);
return deferred.promise;
}

return this.run();
}

Expand Down Expand Up @@ -525,24 +524,20 @@ 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 });
}
});

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);
Expand Down Expand Up @@ -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) {
Expand Down
7 changes: 6 additions & 1 deletion test/fixtures/test-runner/output/hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,19 +99,24 @@ 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'));
await t.test('nested 2', () => testArr.push('nested 2'));
});

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',
]);
});

Expand Down

0 comments on commit 3ae2015

Please sign in to comment.