Fix the timeout logic in the waitForEvent integration test helper function
#18325
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…unction Debugging mozilla#17931, by printing all parts of the event lifecycle including timestamps, uncovered that some events for which a timeout was logged actually did get triggered correctly in the browser. Going over the code and discovering https://stackoverflow.com/questions/47107465/puppeteer-how-to-listen-to-object-events#comment117661238_65534026 showed what went wrong: if the event we wait for is triggered then `Promise.race` resolves, but that doesn't automatically cancel the timeout. The tests didn't fail on this because `Promise.race` resolved correctly, but slightly later once the timeout was reached we would see spurious log lines about timeouts for the already-triggered events. This commit fixes the issue by canceling the timeout if the event we're waiting for has triggered.
timvandermeij
changed the title
waitForEvent integration test helper f…waitForEvent integration test helper function
|
/botio integrationtest |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @timvandermeij received. Current queue size: 0 Live output at: http://54.193.163.58:8877/28d99334c8f3d5e/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @timvandermeij received. Current queue size: 0 Live output at: http://54.241.84.105:8877/d8e7a6b9abae7b1/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/d8e7a6b9abae7b1/output.txt Total script time: 7.76 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.193.163.58:8877/28d99334c8f3d5e/output.txt Total script time: 19.12 mins
|
timvandermeij
requested review from
calixteman and
Snuffleupagus
and removed request for
calixteman
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Debugging #17931, by printing all parts of the event lifecycle including timestamps, uncovered that some events for which a timeout was logged actually did get triggered correctly in the browser. Going over the code and discovering https://stackoverflow.com/questions/47107465/puppeteer-how-to-listen-to-object-events#comment117661238_65534026 showed what went wrong: if the event we wait for is triggered then
Promise.raceresolves, but that doesn't automatically cancel the timeout. The tests didn't fail on this becausePromise.raceresolved correctly, but slightly later once the timeout was reached we would see spurious log lines about timeouts for the already-triggered events.This commit fixes the issue by canceling the timeout if the event we're waiting for has triggered.
Note that this patch gets rid of the incorrect timeout logs, but this doesn't solve the problem of the correct timeout logs yet (but it's preparatory work for follow-up patches that address that problem).
Fixes a part of #17931.