test_runner: support timeout for tests

[MoLow]

Ref: #43490

[benjamingr]

Wondering if we can't use AbortSignal.timeout here

[MoLow]

interesting. how would you suggest doing that? it can be useful to pass a signal into the test context

[MoLow]

And if we pass an abortSignal to the callback - it should probably also be aborted here

node/lib/internal/test_runner/test.js

Lines 267 to 272 in 0fa7730

	this.fail(
	new ERR_TEST_FAILURE(
	'test did not finish before its parent and was cancelled',
	kCancelledByParent
	)
	);

so AbortSignal.timeout won't cover that. WDYT?

[benjamingr]

Yeah, a signal parameter to the test and then cancelling it to enable deep cancellation is probably a good idea but can also be done in a follow-up PR

[tniessen]

IMHO timeouts are not a good mechanism for performance testing, and the only reason to have (default) timeouts is to detect when a test is truly stuck and will never succeed. That is a somewhat rare scenario though, so a large timeout rarely negatively impacts developer experience. On the other hand, timeouts can very negatively impact developer experience when tests pass on fast machines but fail on slower ones. If we really want a default timeout, I'd suggest making it a full minute or more.

[MoLow]

I think in bdd or similar approaches it is not a rare scenario at all, but I would love hearing more opinions about this

[cjihrig]

Left some minor comments, but mostly LGTM. No strong opinion on what the default timeout length should be.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/44777/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/44781/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/44785/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/44787/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/44806/

[tniessen]

As explained in #43505 (comment), I am not a fan of (short) default timeouts. How do popular test frameworks handle this?

IIRC:

• Java: JUnit does not have a default timeout.
• Python: neither unittest nor pytest support timeouts.
• PHP: PHPUnit does not have a default timeout.
• JS: mocha, jest, and jasmine all use extremely short default timeouts, around 5 seconds or so. This does commonly lead to problems of tests timing out on slow machines (for example, in our very own node-core-utils repository).

If I do not explicitly specify a timeout in my code, then I should be able to run it, without modifying the source code, on an arbitrarily slow machine.

[tniessen]

Does the timeout have any effect on synchronous tests? If the event loop is busy, will it ever terminate?

[MoLow]

nope

[tniessen]

That might be unexpected for users. Isn't the purpose of timeouts to prevent tests from getting stuck indefinitely? (I realize that the nature of JavaScript does not make it simple to have synchronous time limits, but other languages have faced the same issues.)

[MoLow]

interesting, how can such a thing be implemented?

[F3n67u]

AFAIK, Jest doesn't implement this if you keep the event loop busy. Jest won't timeout because it doesn't have a chance to run timeout logic when the event loop is busy.

I think this feature could be achieved by distributing tests to a child process or worker thread, but I think it will make things over complicated.

[MoLow]

As explained in #43505 (comment), I am not a fan of (short) default timeouts. How do popular test frameworks handle this?

IIRC:

• Java: JUnit does not have a default timeout.
• Python: neither unittest nor pytest support timeouts.
• PHP: PHPUnit does not have a default timeout.
• JS: mocha, jest, and jasmine all use extremely short default timeouts, around 5 seconds or so. This does commonly lead to problems of tests timing out on slow machines (for example, in our very own node-core-utils repository).

I do not think other languages are a good comparison due to javascript's async nature, and as you see js popular libraries do implement default timeouts.
I agree five sceonds is a bit low

If I do not explicitly specify a timeout in my code, then I should be able to run it, without modifying the source code, on an arbitrarily slow machine.

wont the default timeout of 30 seconds be enough for that?

[tniessen]

If I do not explicitly specify a timeout in my code, then I should be able to run it, without modifying the source code, on an arbitrarily slow machine.

wont the default timeout of 30 seconds be enough for that?

No. By definition, any finite timeout makes tests flaky if measured as wall-clock time on a non-real time operating system. I work with cryptographic libraries a lot; a slow CPU-bound test might complete in 15 seconds on a fast CPU and that's fine. But now someone might be trying to run the test on a Raspberry Pi or in GitHub Actions or something and it suddenly fails or becomes flaky because the processing time increases by a small factor.

[MoLow]

No. By definition, any finite timeout makes tests flaky if measured as wall-clock time on a non-real time operating system. I work with cryptographic libraries a lot; a slow CPU-bound test might complete in 15 seconds on a fast CPU and that's fine. But now someone might be trying to run the test on a Raspberry Pi or in GitHub Actions or something and it suddenly fails or becomes flaky because the processing time increases by a small factor.

I hear what you are saying, I suggest I will change it to have no default timeout but once you specify a timeout it will affect all nested tests? this will adress most of your comments I believe?

[benjamingr]

@tniessen @MoLow I think it's an interesting debate/discussion and you both raise points I haven't considered before - I do think an important use case is that a test timeout enables you to figure out which test timed out. In core our tests currently enable this through "one test (hopefully) per file" which would be preserved without a timeout (I guess).

I'm wondering if a good alternative to "no default timeout" might be (an additional feature) of when telling you which test is currently running when the process is killed. (So defining a test adds an exit handler which writes to stderr what test was running maybe?).

I'm also not sure what TAP specifies so it's probably also important to make sure we're compatible.

Can we agree on:

• A timeout and in general anything based on ms is inherently flaky, which may not be a good tradeoff (or may be a reasonable tradeoff in certain cases).
• It is important to get feedback, preferably quickly when a test is stuck (which we obviously can't tell ahead of time because of the halting problem and all that).

If we can - I think the user getting feedback on process exit is a reasonable tradeoff. Otherwise I prefer the heuristic (big timeout) over the problematic DX for the common case (no timeout) even if it's not correct for the general rare case (running the test on a very slow system).

[MoLow]

If we can - I think the user getting feedback on process exit is a reasonable tradeoff. Otherwise I prefer the heuristic (big timeout) over the problematic DX for the common case (no timeout) even if it's not correct for the general rare case (running the test on a very slow system).

Sounds like the correct solution to me. I Will implement that.
@benjamingr @tniessen do you think such a solution makes this PR redundant? I am not sure about that

[MoLow]

Sounds like the correct solution to me. I Will implement that. @benjamingr @tniessen do you think such a solution makes this PR redundant? I am not sure about that

I think that instead of accepting a timeout in options, we can accept a signal, then this PR along with #43549 will provide a much better api and it will be very easy to implement timeout using AbortSignal.timeout

[benjamingr]

Users would still want to be able to provide timeouts I think?

[MoLow]

Users would still want to be able to provide timeouts I think?

that can be achieved this way:

test('loacker', { signal: AbortSignal.timeout(500) }, () => {
});

doesn't seem too different in terms of DX

test('loacker', { timeout: 500 }, () => {
});

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/45393/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/45407/

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/43505
✔  Done loading data for nodejs/node/pull/43505
----------------------------------- PR info ------------------------------------
Title      test_runner: support timeout for tests (#43505)
Author     Moshe Atlow  (@MoLow)
Branch     MoLow:test-runner-support-timeout -> nodejs:main
Labels     needs-ci, test_runner
Commits    13
 - test_runner: support timeout for tests
 - lintr
 - CR
 - linter
 - more tests
 - cr
 - lint
 - whoops
 - CR
 - timeout this as well
 - this is probably wrong since timeouts inherit
 - tests
 - fix rebase
Committers 1
 - Moshe Atlow 
PR-URL: https://github.com/nodejs/node/pull/43505
Refs: https://github.com/nodejs/node/issues/43490
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Colin Ihrig 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/43505
Refs: https://github.com/nodejs/node/issues/43490
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Colin Ihrig 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - test_runner: support timeout for tests
   ⚠  - lintr
   ⚠  - CR
   ⚠  - linter
   ⚠  - more tests
   ⚠  - cr
   ⚠  - lint
   ⚠  - whoops
   ⚠  - CR
   ⚠  - timeout this as well
   ⚠  - this is probably wrong since timeouts inherit
   ⚠  - tests
   ⚠  - fix rebase
   ℹ  This PR was created on Mon, 20 Jun 2022 09:55:35 GMT
   ✔  Approvals: 2
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/43505#pullrequestreview-1012196209
   ✔  - Colin Ihrig (@cjihrig) (TSC): https://github.com/nodejs/node/pull/43505#pullrequestreview-1014378753
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-07-13T17:08:34Z: https://ci.nodejs.org/job/node-test-pull-request/45407/
- Querying data for job/node-test-pull-request/45407/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/2668767180

[nodejs-github-bot]

Landed in c1d6595