Do not count writes to stdout/stderr as non-idling activity for timeouts #3374
Conversation
| @@ -218,7 +218,7 @@ export default class Api extends Emittery { | |||
| } | |||
|
|
|||
| runStatus.on('stateChange', record => { | |||
| if (record.testFile && !timedOutWorkerFiles.has(record.testFile)) { | |||
| if (record.testFile && !timedOutWorkerFiles.has(record.testFile) && record.type !== 'worker-stderr' && record.type !== 'worker-stdout') { | |||
There was a problem hiding this comment.
I'd personally prefer to make this opt-in so only assertions/test state changes reset the timeout (so we hew closer to the documentation).
But:
a) I don't know the complete list of record type's here that would apply
b) it's a bigger change from current behavior than I'm comfortable making without feedback
There was a problem hiding this comment.
I think this change is correct. The timeout here is a guard against test workers that are no longer making progress. It's different from t.timeout() which limits how long an individual test can take.
Essentially with this change, we no longer count stdout/stderr as "making progress".
There was a problem hiding this comment.
This fix is correct, but for a different reason than you might expect. The timeout in the api.js file is best thought of as an "idle guard". I agree that stdout/stderr shouldn't count as non-idle activity.
In #3373 the test worker isn't exiting because it's still waiting on a timer. And the idle guard doesn't force it to exit. With this change it will, though only 10 seconds after the test itself has timed out.
| @@ -218,7 +218,7 @@ export default class Api extends Emittery { | |||
| } | |||
|
|
|||
| runStatus.on('stateChange', record => { | |||
| if (record.testFile && !timedOutWorkerFiles.has(record.testFile)) { | |||
| if (record.testFile && !timedOutWorkerFiles.has(record.testFile) && record.type !== 'worker-stderr' && record.type !== 'worker-stdout') { | |||
There was a problem hiding this comment.
I think this change is correct. The timeout here is a guard against test workers that are no longer making progress. It's different from t.timeout() which limits how long an individual test can take.
Essentially with this change, we no longer count stdout/stderr as "making progress".
test/test-timeouts/test.js
Outdated
| test('timeouts are not blocked by console output', async t => { | ||
| const result = await t.throwsAsync(fixture(['console-output.js'])); | ||
| const error = result.stats.getError(result.stats.failed[0]); | ||
| t.snapshot(error.message, 'timeout despite console output'); | ||
| }); |
There was a problem hiding this comment.
This test is in the wrong place. test-timeouts refers to t.timeout() but this is modifying the overall timeout behavior. A new idle-timeout/ folder would be more appropriate.
(Can't recall whether there's existing tests under the old test-tap but I'm happy not to add there.)
There was a problem hiding this comment.
Went ahead and moved the test, thanks.
Thanks for explaining that, some of the behavior I saw while fixing this makes more sense now that I see the test timeout in Test#timeout vs. the worker timeout here. |
* Set a short timeout so it finishes more quickly * Remove the snapshot; the assertion message was the same as the error message being snapshotted * Tweak delays within test to be clearer that it can run for a second, but then we keep writing to stdout/stderr * Use timers/promises rather than new Promise() * Alternate writing to stdout and stderr
novemberborn
changed the title
|
Thanks @mdouglass, this looks good. I've made some smaller tweaks, mostly in the test itself. Your version took 10.8 seconds to execute in CI which is a little unnecessary 😄 |
🤣 most definitely too long. Thanks 👍 |
fixes #3373