Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test_runner: execute before hook on test #47586

Merged
merged 3 commits into from
Apr 18, 2023
Merged

test_runner: execute before hook on test #47586

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4e09df4
test_runner: execute before hook on test
atlowChemi Apr 14, 2023
013a47d
add test & fix lint
atlowChemi Apr 16, 2023
80ddbaf
fix doc
atlowChemi Apr 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions doc/api/test.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1498,12 +1498,36 @@ Emitted when a test starts.
added:
- v18.0.0
- v16.17.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/47586
description: The `before` function was added to TestContext.
-->

An instance of `TestContext` is passed to each test function in order to
interact with the test runner. However, the `TestContext` constructor is not
exposed as part of the API.

### `context.before([fn][, options])`

<!-- YAML
added: REPLACEME
-->

* `fn` {Function|AsyncFunction} The hook function. The first argument
to this function is a [`TestContext`][] object. If the hook uses callbacks,
the callback function is passed as the second argument. **Default:** A no-op
function.
* `options` {Object} Configuration options for the hook. The following
properties are supported:
* `signal` {AbortSignal} Allows aborting an in-progress hook.
* `timeout` {number} A number of milliseconds the hook will fail after.
If unspecified, subtests inherit this value from their parent.
**Default:** `Infinity`.

This function is used to create a hook running before
subtest of the current test.

### `context.beforeEach([fn][, options])`

<!-- YAML
Expand Down
14 changes: 12 additions & 2 deletions lib/internal/test_runner/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,10 @@ class TestContext {
return subtest.start();
}

before(fn, options) {
this.#test.createHook('before', fn, options);
}

after(fn, options) {
this.#test.createHook('after', fn, options);
}
Expand Down Expand Up @@ -414,6 +418,9 @@ class Test extends AsyncResource {
validateOneOf(name, 'hook name', kHookNames);
// eslint-disable-next-line no-use-before-define
const hook = new TestHook(fn, options);
if (name === 'before') {
hook.run = runOnce(hook.run);
}
Comment on lines 420 to +423
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const hook = new TestHook(fn, options);
if (name === 'before') {
hook.run = runOnce(hook.run);
}
if (name === 'before' || name === 'after') {
fn = runOnce(fn);
}
const hook = new TestHook(fn, options);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can also wrap after hooks and replace

 const after = runOnce(async () => {
      if (this.hooks.after.length > 0) {
        await this.runHook('after', { args, ctx });
      }
  });

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I avoided wrapping the function directly at this point because it might not be a function yet, as the constructor handles this case:

node/lib/internal/test_runner/test.js

Lines 174 to 176 in 4e09df4

if (typeof fn !== 'function') {
fn = noop;
}

Do you think the same handling should be added here to wrap fn directly?

I think we can also wrap after hooks and replace

I believe this would change the behavior of the after hook though:
Currently, if you have multiple after hooks, and one of them throws, we will not re-run any of them in the catch block, where as by changing to wrapping each hook individually we will run all the hooks that have not yet been executed.

ArrayPrototypePush(this.hooks[name], hook);
return hook;
}
Expand Down Expand Up @@ -525,6 +532,9 @@ class Test extends AsyncResource {
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());
}
const stopPromise = stopTest(this.timeout, this.signal);
const runArgs = ArrayPrototypeSlice(args);
ArrayPrototypeUnshift(runArgs, this.fn, ctx);
Expand Down Expand Up @@ -561,7 +571,7 @@ class Test extends AsyncResource {
this.pass();
} catch (err) {
try { await after(); } catch { /* Ignore error. */ }
try { await afterEach(); } catch { /* test is already failing, let's the error */ }
try { await afterEach(); } catch { /* test is already failing, let's ignore the error */ }
if (isTestFailureError(err)) {
if (err.failureType === kTestTimeoutFailure) {
this.#cancel(err);
Expand Down Expand Up @@ -793,7 +803,7 @@ class Suite extends Test {

this.pass();
} catch (err) {
try { await afterEach(); } catch { /* test is already failing, let's the error */ }
try { await afterEach(); } catch { /* test is already failing, let's ignore the error */ }
if (isTestFailureError(err)) {
this.fail(err);
} else {
Expand Down
14 changes: 13 additions & 1 deletion test/fixtures/test-runner/output/hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ const common = require('../../../common');
const assert = require('assert');
const { test, describe, it, before, after, beforeEach, afterEach } = require('node:test');

before((t) => t.diagnostic('before 1 called'));

describe('describe hooks', () => {
const testArr = [];
before(function() {
Expand Down Expand Up @@ -91,6 +93,7 @@ describe('afterEach throws and test fails', () => {
test('test hooks', async (t) => {
const testArr = [];

t.before((t) => testArr.push('before ' + t.name));
MoLow marked this conversation as resolved.
Show resolved Hide resolved
t.after(common.mustCall((t) => testArr.push('after ' + t.name)));
t.beforeEach((t) => testArr.push('beforeEach ' + t.name));
t.afterEach((t) => testArr.push('afterEach ' + t.name));
Expand All @@ -105,7 +108,7 @@ test('test hooks', async (t) => {
});

assert.deepStrictEqual(testArr, [
'beforeEach 1', '1', 'afterEach 1',
'beforeEach 1', 'before test hooks', '1', 'afterEach 1',
'beforeEach 2', '2', 'afterEach 2',
'beforeEach nested',
'nested beforeEach nested 1', 'nested1', 'nested afterEach nested 1',
Expand All @@ -114,6 +117,13 @@ test('test hooks', async (t) => {
]);
});

test('t.before throws', async (t) => {
t.after(common.mustCall());
t.before(() => { throw new Error('before'); });
await t.test('1', () => {});
await t.test('2', () => {});
});

test('t.beforeEach throws', async (t) => {
t.after(common.mustCall());
t.beforeEach(() => { throw new Error('beforeEach'); });
Expand Down Expand Up @@ -149,3 +159,5 @@ test('t.after() is called if test body throws', (t) => {
});
throw new Error('bye');
});

before((t) => t.diagnostic('before 2 called'));
71 changes: 58 additions & 13 deletions test/fixtures/test-runner/output/hooks.snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ not ok 2 - before throws
*
*
*
*
...
# Subtest: after throws
# Subtest: 1
Expand Down Expand Up @@ -307,6 +308,53 @@ ok 8 - test hooks
---
duration_ms: *
...
# Subtest: t.before throws
# Subtest: 1
not ok 1 - 1
---
duration_ms: *
failureType: 'hookFailed'
error: 'failed running before hook'
code: 'ERR_TEST_FAILURE'
stack: |-
*
*
*
*
*
*
*
*
*
*
...
# Subtest: 2
not ok 2 - 2
---
duration_ms: *
failureType: 'hookFailed'
error: 'failed running before hook'
code: 'ERR_TEST_FAILURE'
stack: |-
*
*
*
*
*
*
*
*
*
*
...
1..2
not ok 9 - t.before throws
---
duration_ms: *
failureType: 'subtestsFailed'
error: '2 subtests failed'
code: 'ERR_TEST_FAILURE'
...
# Subtest: t.beforeEach throws
# Subtest: 1
not ok 1 - 1
Expand Down Expand Up @@ -347,7 +395,7 @@ ok 8 - test hooks
*
...
1..2
not ok 9 - t.beforeEach throws
not ok 10 - t.beforeEach throws
---
duration_ms: *
failureType: 'subtestsFailed'
Expand Down Expand Up @@ -394,7 +442,7 @@ not ok 9 - t.beforeEach throws
*
...
1..2
not ok 10 - t.afterEach throws
not ok 11 - t.afterEach throws
---
duration_ms: *
failureType: 'subtestsFailed'
Expand All @@ -419,15 +467,14 @@ not ok 10 - t.afterEach throws
*
*
*
*
...
# Subtest: 2
ok 2 - 2
---
duration_ms: *
...
1..2
not ok 11 - afterEach when test fails
not ok 12 - afterEach when test fails
---
duration_ms: *
failureType: 'subtestsFailed'
Expand All @@ -452,7 +499,6 @@ not ok 11 - afterEach when test fails
*
*
*
*
...
# Subtest: 2
not ok 2 - 2
Expand All @@ -474,15 +520,15 @@ not ok 11 - afterEach when test fails
*
...
1..2
not ok 12 - afterEach throws and test fails
not ok 13 - afterEach throws and test fails
---
duration_ms: *
failureType: 'subtestsFailed'
error: '2 subtests failed'
code: 'ERR_TEST_FAILURE'
...
# Subtest: t.after() is called if test body throws
not ok 13 - t.after() is called if test body throws
not ok 14 - t.after() is called if test body throws
---
duration_ms: *
failureType: 'testCodeFailure'
Expand All @@ -493,16 +539,15 @@ not ok 13 - t.after() is called if test body throws
*
*
*
*
*
*
...
# - after() called
1..13
# tests 35
1..14
# before 1 called
# before 2 called
# tests 38
# suites 8
# pass 14
# fail 19
# fail 22
# cancelled 2
# skipped 0
# todo 0
Expand Down