Handle timeout when waiting for page readiness #30
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
Prior to this commit, if the Promise stored in the binding named `loaded` rejected while the WebDriver "wait" command was being evaluated, that rejection would trigger a Node.js global "unhandled rejection" error and cause the process to crash. Track all Promise values so that a rejection in any of them at any time is handled by the caller. Recognize when the expected "load" event has already occurred and stop waiting immediately.
gnarf
approved these changes
Nov 29, 2023
|
Tested the following so far with no errors/extra weird results
|
|
Right on! Thanks, @gnarf! |
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.
Prior to this commit, if the Promise stored in the binding named
loadedrejected while the WebDriver "wait" command was being evaluated, that rejection would trigger a Node.js global "unhandled rejection" error and cause the process to crash.Track all Promise values so that a rejection in any of them at any time is handled by the caller. Recognize when the expected "load" event has already occurred and stop waiting immediately.
@gnarf I've had a change of heart from my comment yesterday. Since we are preparing to deploy this for the very first time, there's relatively little risk in changing old code. Now seems like a good opportunity to challenge assumptions because everyone involved expects the initial release to be unstable.
To that end, I've simplified this logic to wait for "document readiness" and only then begin waiting for the presence of the "setup" button. That's involved addressing the previously-overlooked race condition with the "ready" event (one which you and I haven't discussed); now, this new version of the code recognizes when the event occurs before the script injection via WebDriver.
Beyond improving maintainability and reducing the risk of further design flaws, this change has the potential to significantly reduce the amount of time necessary to run the tests (since the harness will no longer pause for five seconds when it missed the "load" event). Another benefit is the surfacing of legitimate errors (because the harness no longer ignores all "execute script" timeouts unilaterally).
The risk to this change is that it exposes a latent race condition. In other words, it's possible that the overlong timeouts before were coincidentally delaying the test execution beyond some other, as-yet-unidentified pre-condition. To guard against that, I'm running some tests to see if the results still look reasonable; would you mind doing the same? (Such informal evidence would not normally convince me, but again, because we're deploying a new system, I'm a little more willing to experiment for the sake of more deterministic code.)