test_runner: add timeout support to test plan #56765
Conversation
|
Review requested:
|
nodejs-github-bot
added
needs-ci
lib/internal/test_runner/test.js
Outdated
|
|
||
| // Wait for the race to finish | ||
| await SafePromiseRace(promises); | ||
|
|
||
| this[kShouldAbort](); | ||
| this.plan?.check(); |
There was a problem hiding this comment.
I think we could do this more simply, and in a way that is less leaky into the rest of the test runner. By this point, the test runner believes that the user's test code has finished executing, although there can still be pending asynchronous activity that the test code scheduled. If the user has provided a plan timeout, then we can race that directly against the existing stopPromise here.
I would also consider renaming timeout to wait, and let that option be a boolean or a number. If it's a boolean, true means wait indefinitely and false means do not wait at all. false could be the default for backwards compatibility purposes. If wait is a number, then the plan would wait for that many milliseconds. All of that logic could live in the TestPlan class, away from the rest of the test runner.
I'm also concerned that with the current implementation, the plan.timeoutPromise serves almost the exact same purpose as the test() timeout option. I think by waiting to arm the timer until this point, it becomes more distinct functionality (that's also partly why I think wait is a better name here).
If the plan has already been satisfied by this point, it also means that we don't have to arm a timer at all, which is more efficient.
| this.plan?.check(); | |
| if (this.plan !== null) { | |
| // All of the plan waiting/checking logic goes here so that | |
| // only tests that use a plan have the overhead. | |
| } |
There was a problem hiding this comment.
thanks @cjihrig, yes, it seems a great approach!
I'll apply your suggestions as soon as possible!
| } | ||
| } | ||
|
|
||
|
|
| @@ -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. | |||
| 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); |
There was a problem hiding this comment.
IMHO, this refactor could enhance readability even though it is no longer needed and could be reverted
This PR attempts to address #56758.
One major concern about this feature: to allow the timeout to interrupt the tests the
--test-force-exit flagis required.I'm opening this PR as a draft to validate the implementation and behaviour.
Documentation still needs to be updated!