Skip to content

Commit

Permalink
test_runner: preserve hook promise when executed twice
Browse files Browse the repository at this point in the history
PR-URL: #52791
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
  • Loading branch information
MoLow authored and marco-ippolito committed Jun 17, 2024
1 parent a5f3dd1 commit cf817e1
Show file tree
Hide file tree
Showing 5 changed files with 86 additions and 12 deletions.
8 changes: 5 additions & 3 deletions lib/internal/test_runner/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ const {
let kResistStopPropagation;
let findSourceMap;

const kRunOnceOptions = { __proto__: null, preserveReturnValue: true };

function lazyFindSourceMap(file) {
if (findSourceMap === undefined) {
({ findSourceMap } = require('internal/source_map/source_map_cache'));
Expand Down Expand Up @@ -526,7 +528,7 @@ class Test extends AsyncResource {
// eslint-disable-next-line no-use-before-define
const hook = new TestHook(fn, options);
if (name === 'before' || name === 'after') {
hook.run = runOnce(hook.run);
hook.run = runOnce(hook.run, kRunOnceOptions);
}
if (name === 'before' && this.startTime !== null) {
// Test has already started, run the hook immediately
Expand Down Expand Up @@ -650,7 +652,7 @@ class Test extends AsyncResource {
if (this.parent?.hooks.afterEach.length > 0 && !this.skipped) {
await this.parent.runHook('afterEach', hookArgs);
}
});
}, kRunOnceOptions);

let stopPromise;

Expand Down Expand Up @@ -1004,7 +1006,7 @@ class Suite extends Test {
const hookArgs = this.getRunArgs();

let stopPromise;
const after = runOnce(() => this.runHook('after', hookArgs));
const after = runOnce(() => this.runHook('after', hookArgs), kRunOnceOptions);
try {
this.parent.activeSubtests++;
await this.buildSuite;
Expand Down
9 changes: 6 additions & 3 deletions lib/internal/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -511,12 +511,15 @@ function isInsideNodeModules() {
return false;
}

function once(callback) {
function once(callback, { preserveReturnValue = false } = kEmptyObject) {
let called = false;
let returnValue;
return function(...args) {
if (called) return;
if (called) return returnValue;
called = true;
return ReflectApply(callback, this, args);
const result = ReflectApply(callback, this, args);
returnValue = preserveReturnValue ? result : undefined;
return result;
};
}

Expand Down
51 changes: 50 additions & 1 deletion test/fixtures/test-runner/output/hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
const common = require('../../../common');
const assert = require('assert');
const { test, describe, it, before, after, beforeEach, afterEach } = require('node:test');
const { setTimeout } = require('node:timers/promises');

before((t) => t.diagnostic('before 1 called'));
after((t) => t.diagnostic('after 1 called'));
Expand Down Expand Up @@ -148,7 +149,6 @@ test('test hooks', async (t) => {
}));
});


test('test hooks - no subtests', async (t) => {
const testArr = [];

Expand Down Expand Up @@ -253,5 +253,54 @@ describe('run after when before throws', () => {
it('1', () => {});
});


test('test hooks - async', async (t) => {
const testArr = [];

t.before(async (t) => {
testArr.push('before starting ' + t.name);
await setTimeout(10);
testArr.push('before ending ' + t.name);
});
t.after(async (t) => {
testArr.push('after starting ' + t.name);
await setTimeout(10);
testArr.push('after ending ' + t.name);
});
t.beforeEach(async (t) => {
testArr.push('beforeEach starting ' + t.name);
await setTimeout(10);
testArr.push('beforeEach ending ' + t.name);
});
t.afterEach(async (t) => {
testArr.push('afterEach starting ' + t.name);
await setTimeout(10);
testArr.push('afterEach ending ' + t.name);
});
await t.test('1', async () => {
testArr.push('1 starting');
await setTimeout(10);
testArr.push('1 ending');
});
await t.test('2', async () => {
testArr.push('2 starting');
await setTimeout(10);
testArr.push('2 ending');
});

t.after(common.mustCall(() => {
assert.deepStrictEqual(testArr, [
'before starting test hooks - async', 'before ending test hooks - async',
'beforeEach starting 1', 'beforeEach ending 1',
'1 starting', '1 ending',
'afterEach starting 1', 'afterEach ending 1',
'beforeEach starting 2', 'beforeEach ending 2',
'2 starting', '2 ending',
'afterEach starting 2', 'afterEach ending 2',
'after starting test hooks - async', 'after ending test hooks - async',
]);
}));
});

before((t) => t.diagnostic('before 2 called'));
after((t) => t.diagnostic('after 2 called'));
22 changes: 19 additions & 3 deletions test/fixtures/test-runner/output/hooks.snapshot
Original file line number Diff line number Diff line change
Expand Up @@ -767,14 +767,30 @@ not ok 24 - run after when before throws
*
*
...
1..24
# Subtest: test hooks - async
# Subtest: 1
ok 1 - 1
---
duration_ms: *
...
# Subtest: 2
ok 2 - 2
---
duration_ms: *
...
1..2
ok 25 - test hooks - async
---
duration_ms: *
...
1..25
# before 1 called
# before 2 called
# after 1 called
# after 2 called
# tests 49
# tests 52
# suites 12
# pass 19
# pass 22
# fail 27
# cancelled 3
# skipped 0
Expand Down
8 changes: 6 additions & 2 deletions test/fixtures/test-runner/output/hooks_spec_reporter.snapshot
Original file line number Diff line number Diff line change
Expand Up @@ -392,13 +392,17 @@
*
*

test hooks - async
1 (*ms)
2 (*ms)
test hooks - async (*ms)
before 1 called
before 2 called
after 1 called
after 2 called
tests 49
tests 52
suites 12
pass 19
pass 22
fail 27
cancelled 3
skipped 0
Expand Down

0 comments on commit cf817e1

Please sign in to comment.