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.

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

test_runner: add timeout support to test plan #56765

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
29 changes: 29 additions & 0 deletions doc/api/test.md
Original file line number Diff line number Diff line change
Expand Up @@ -3407,6 +3407,10 @@ added:
- v22.2.0
- v20.15.0
changes:
- version:
- REPLACEME
pr-url: https://github.com/nodejs/node/pull/56765
description: Add the `options` parameter.
- version:
- v23.4.0
- v22.13.0
Expand All @@ -3415,6 +3419,13 @@ changes:
-->

* `count` {number} The number of assertions and subtests that are expected to run.
* `options` {Object} _(Optional)_ Additional options for the plan.
* `wait` {boolean|number} The wait time for the plan:
* If `true`, the plan waits indefinitely for all assertions and subtests to run.
* If a number, it specifies the maximum wait time in milliseconds
before timing out while waiting for expected assertions and subtests to be matched.
If the timeout is reached, the test will fail.
**Default:** `false`.

This function is used to set the number of assertions and subtests that are expected to run
within the test. If the number of assertions and subtests that run does not match the
Expand Down Expand Up @@ -3453,6 +3464,24 @@ test('planning with streams', (t, done) => {
});
```

When using the `wait` option, you can control how long the test will wait for the expected assertions.
For example, setting a maximum wait time ensures that the test will wait for asynchronous assertions
to complete within the specified timeframe:

```js
test('plan with wait: 2000 waits for async assertions', (t) => {
t.plan(1, { wait: 2000 }); // Waits for up to 2 seconds for the assertion to complete.

const asyncActivity = () => {
setTimeout(() => {
t.assert.ok(true, 'Async assertion completed within the wait time');
}, 1000); // Completes after 1 second, within the 2-second wait time.
};

asyncActivity(); // The test will pass because the assertion is completed in time.
});
```

### `context.runOnly(shouldRunOnlyTests)`

<!-- YAML
Expand Down
144 changes: 128 additions & 16 deletions lib/internal/test_runner/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ function lazyAssertObject(harness) {
return assertObj;
}

function stopTest(timeout, signal) {
function stopTest(timeout, signal, reason) {
const deferred = PromiseWithResolvers();
const abortListener = addAbortListener(signal, deferred.resolve);
let timer;
Expand All @@ -141,7 +141,7 @@ function stopTest(timeout, signal) {
writable: true,
value: PromisePrototypeThen(deferred.promise, () => {
throw new ERR_TEST_FAILURE(
`test timed out after ${timeout}ms`,
reason || `test timed out after ${timeout}ms`,
Copy link
Member Author

Choose a reason for hiding this comment

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

This should not be needed anymore

kTestTimeoutFailure,
);
}),
Expand Down Expand Up @@ -176,22 +176,118 @@ function testMatchesPattern(test, patterns) {
}

class TestPlan {
constructor(count) {
validateUint32(count, 'count');
#waitIndefinitely = false;

constructor(count, options = kEmptyObject) {
this.#validateConstructorArgs(count, options);
this.expected = count;
this.actual = 0;
this.#setWaitOption(options);
}

#validateConstructorArgs(count, options) {
validateUint32(count, 'count');
validateObject(options, 'options');
}

#setWaitOption(options) {
switch (typeof options.wait) {
case 'boolean':
this.wait = options.wait;
this.#waitIndefinitely = options.wait;
break;
case 'number':
validateNumber(options.wait, 'options.wait', 0, TIMEOUT_MAX);
this.wait = options.wait;
break;
default:
if (options.wait !== undefined) {
throw new ERR_INVALID_ARG_TYPE('options.wait', ['boolean', 'number'], options.wait);
}
}
}

#planMet() {
return this.actual === this.expected;
}

#startPoller() {
const { promise, resolve, reject } = PromiseWithResolvers();
const noError = Symbol();
let pollerId;
let maxWaitingTimeId;
const interval = 50;

const done = (err, result) => {
clearTimeout(pollerId);
maxWaitingTimeId ?? clearTimeout(maxWaitingTimeId);

if (err === noError) {
resolve(result);
} else {
reject(err);
}
};

// If the plan has a maximum wait time, set a timeout.
// Otherwise, the plan will wait indefinitely.
if (this.#shouldSetMaxWaitingTime()) {
maxWaitingTimeId = this.#setMaxWaitingTime(done);
}

const poller = async () => {
if (this.#planMet()) {
done(noError);
} else {
pollerId = setTimeout(poller, interval);
}
};

poller();
return promise;
}

#setMaxWaitingTime(done) {
return setTimeout(() => {
const err = new ERR_TEST_FAILURE(
`plan timed out after ${this.wait}ms with ${this.actual} assertions when expecting ${this.expected}`,
kTestTimeoutFailure,
);
// The poller has timed out; the test should fail.
done(err);
}, this.wait);
}

check() {
if (this.actual !== this.expected) {
if (this.#planMet()) {
// If the plan has been met, there is no need to check for a timeout.
return;
}
// The plan has not been met.
if (!this.#shouldWait()) {
// If the plan doesn't have a wait time defined, the test should fail immediately.
throw new ERR_TEST_FAILURE(
`plan expected ${this.expected} assertions but received ${this.actual}`,
kTestCodeFailure,
);
}
return this.#startPoller();
}

count() {
this.actual++;
}

#shouldWait() {
return this.wait !== undefined && this.wait !== false;
}

#shouldSetMaxWaitingTime() {
return !this.#waitIndefinitely && this.wait !== false;
}
}


Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change

class TestContext {
#assert;
#test;
Expand Down Expand Up @@ -228,15 +324,15 @@ class TestContext {
this.#test.diagnostic(message);
}

plan(count) {
plan(count, options = kEmptyObject) {
if (this.#test.plan !== null) {
throw new ERR_TEST_FAILURE(
'cannot set plan more than once',
kTestCodeFailure,
);
}

this.#test.plan = new TestPlan(count);
this.#test.plan = new TestPlan(count, options);
}

get assert() {
Expand All @@ -249,7 +345,7 @@ class TestContext {
map.forEach((method, name) => {
assert[name] = (...args) => {
if (plan !== null) {
plan.actual++;
plan.count();
}
return ReflectApply(method, this, args);
};
Expand All @@ -260,7 +356,7 @@ class TestContext {
// stacktrace from the correct starting point.
function ok(...args) {
if (plan !== null) {
plan.actual++;
plan.count();
}
innerOk(ok, args.length, ...args);
}
Expand Down Expand Up @@ -296,7 +392,7 @@ class TestContext {

const { plan } = this.#test;
if (plan !== null) {
plan.actual++;
plan.count();
}

const subtest = this.#test.createSubtest(
Expand Down Expand Up @@ -436,6 +532,8 @@ class Test extends AsyncResource {
super('Test');

let { fn, name, parent } = options;
// TODO(pmarchini): The plan has additional options that a user can set via t.plan.
Copy link
Member Author

Choose a reason for hiding this comment

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

@cjihrig, wdyt?

// We should allow users to set these options via the options object for consistency.
const { concurrency, entryFile, loc, only, timeout, todo, skip, signal, plan } = options;

if (typeof fn !== 'function') {
Expand Down Expand Up @@ -959,30 +1057,44 @@ class Test extends AsyncResource {
const runArgs = ArrayPrototypeSlice(args);
ArrayPrototypeUnshift(runArgs, this.fn, ctx);

const promises = [];
if (this.fn.length === runArgs.length - 1) {
// This test is using legacy Node.js error first callbacks.
// This test is using legacy Node.js error-first callbacks.
const { promise, cb } = createDeferredCallback();

ArrayPrototypePush(runArgs, cb);

const ret = ReflectApply(this.runInAsyncScope, this, runArgs);

if (isPromise(ret)) {
this.fail(new ERR_TEST_FAILURE(
'passed a callback but also returned a Promise',
kCallbackAndPromisePresent,
));
await SafePromiseRace([ret, stopPromise]);
ArrayPrototypePush(promises, ret);
} else {
await SafePromiseRace([PromiseResolve(promise), stopPromise]);
ArrayPrototypePush(promises, PromiseResolve(promise));
}
} else {
// This test is synchronous or using Promises.
const promise = ReflectApply(this.runInAsyncScope, this, runArgs);
await SafePromiseRace([PromiseResolve(promise), stopPromise]);
ArrayPrototypePush(promises, PromiseResolve(promise));
}

ArrayPrototypePush(promises, stopPromise);

// Wait for the race to finish
await SafePromiseRace(promises);
Comment on lines +1073 to +1086
Copy link
Member Author

Choose a reason for hiding this comment

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

IMHO, this refactor could enhance readability even though it is no longer needed and could be reverted


this[kShouldAbort]();
this.plan?.check();
if (this.plan !== null) {
const checkPollPromise = this.plan?.check();
// If the plan returns a promise, then the plan is polling and we need to wait for it to finish
// or the test to stop
if (checkPollPromise) {
await SafePromiseRace([checkPollPromise, stopPromise]);
}
}

this.pass();
await afterEach();
await after();
Expand Down
77 changes: 77 additions & 0 deletions test/fixtures/test-runner/output/test-runner-plan-timeout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
'use strict';
const { describe, it } = require('node:test');
const { platformTimeout } = require('../../../common');

describe('planning with wait', () => {
it('planning with wait and passing', async (t) => {
t.plan(1, { wait: platformTimeout(5000) });

const asyncActivity = () => {
setTimeout(() => {
t.assert.ok(true);
}, platformTimeout(250));
};

asyncActivity();
});

it('planning with wait and failing', async (t) => {
t.plan(1, { wait: platformTimeout(5000) });

const asyncActivity = () => {
setTimeout(() => {
t.assert.ok(false);
}, platformTimeout(250));
};

asyncActivity();
});

it('planning wait time expires before plan is met', async (t) => {
t.plan(2, { wait: platformTimeout(500) });

const asyncActivity = () => {
setTimeout(() => {
t.assert.ok(true);
}, platformTimeout(50_000_000));
};

asyncActivity();
});

it(`planning with wait "options.wait : true" and passing`, async (t) => {
t.plan(1, { wait: true });

const asyncActivity = () => {
setTimeout(() => {
t.assert.ok(true);
}, platformTimeout(250));
};

asyncActivity();
});

it(`planning with wait "options.wait : true" and failing`, async (t) => {
t.plan(1, { wait: true });

const asyncActivity = () => {
setTimeout(() => {
t.assert.ok(false);
}, platformTimeout(250));
};

asyncActivity();
});

it(`planning with wait "options.wait : false" should not wait`, async (t) => {
t.plan(1, { wait: false });

const asyncActivity = () => {
setTimeout(() => {
t.assert.ok(true);
}, platformTimeout(500_000));
};

asyncActivity();
})
});
Loading
Loading