Skip to content

Make timeout work for synchronous tests - #3495

Open
ericcornelissen wants to merge 2 commits into
avajs:mainfrom
ericcornelissen:sync-test-timeout
Open

ericcornelissen wants to merge 2 commits into
avajs:mainfrom
ericcornelissen:sync-test-timeout

Conversation

@ericcornelissen

Copy link
Copy Markdown
Contributor

Closes #3454

This changes the Test class to fail when its runtime exceeds the configured timeout value. This prevents tests with long runtimes but no asynchronicity from succeeding, as illustrated by the added test fixture.

Like with asynchronous timeouts, the synchronous timeout is reset in the refreshTimeout method. This makes the behavior consistent and was forced by the pre-existing test suite.

If you want I can spend a bit more time trying to reduce the duplication. Also, I'm unsure about the return on line 467 as the test suite passes regardless of whether or not it's there.

Fail a (synchronous) test when its runtime exceeds the configured
timeout value. This prevents tests with long runtimes but no
asynchronicity from succeeding.

The added test fixture demonstrates this. Before this change such a test
would pass but with this change it will fail because it exceeds the
configured timeout.
Extend the synchronous timeout behavior to, like the asynchronous
timeout behavior, reset the timer, through `refreshTimeout`, on (e.g.)
assertions.
@hafizahmedzia

Copy link
Copy Markdown

Is the change to reported teardown duration intentional in @ericcornelissen's synchronous-timeout fix? finish() now reuses the timestamp captured before await this.runTeardowns(), so a slow teardown is omitted even when the test never calls t.timeout():

import test from 'ava';
import {setTimeout as delay} from 'node:timers/promises';

test('slow cleanup', t => {
  t.teardown(() => delay(300));
  t.pass();
});

In the recorded Linux / Node v24.19.0 comparison, across three runs per ref, the real Test.run() result reported about 300 ms on 7502074074d0681a2fd395534e88f0673aabf05d and 0–1 ms on d9a461444403652e2bf7787062777b58896d7471; an independent monotonic clock measured about 300 ms through completed cleanup on both. The actual default CLI also changes from a line with a (301ms) suffix to no duration suffix. A 300 ms body-only control still reports about 300 ms; 150 ms body plus 300 ms teardown changes from about 451 ms to 151 ms.

Could the pre-teardown timestamp be kept for checking the timeout while measuring reported duration after teardown? The existing timeout scope can stay the same: a quick body with t.timeout(50) and 300 ms teardown passes on both refs.

The added synchronous fixture passes on the pre-PR baseline and correctly fails with a timeout on the head. Focused runs passed on both refs: tap test-tap/test.js (139 assertions each) and test-ava test/test-timeouts/test.js (2 baseline / 3 head). The full suite was not run.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Blocking tests don't cause tests with timeout to fail

2 participants