Handle timeout when waiting for page readiness

[jugglinmike]

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.

[gnarf]

The promise logic makes sense to me, but is this just turning the error into "didn't run test setup" errors now? Like, it still won't click the button?

[gnarf]

Wondering if maybe await allSettled makes more sense? The "timeout" case being an error means we still want to click right?

[gnarf]

Also a thought, if my assumption that a timeout during waiting for the page load event was always supposed to still try to click the button if it was found:

      const loadedTimeout = timeout(AFTER_RUN_TEST_SETUP_BUTTON_DELAY);
      const loaded = this.webDriver.executeAsyncScript(function (callback) {
        new Promise(resolve => {
          window.addEventListener('load', () => resolve());
        })
          // Wait until after any microtasks registered by other 'load' event
          // handlers.
          .then(() => Promise.resolve())
          .then(callback);
      })
        // convert an error (script timeout) into our positive timeout.
        .catch(() => loadedTimeout);

      const runTestSetup = await this.webDriver.wait(
        until.elementLocated(By.className('button-run-test-setup')),
        RUN_TEST_SETUP_BUTTON_TIMEOUT
      );
      // TODO: Replace loaded and timeout race with a deterministic signal that
      // the page is ready. This likely needs a change in aria-at's process.
      await Promise.race([loaded, loadedTimeout]);
      await runTestSetup.click();

[jugglinmike]

The promise logic makes sense to me, but is this just turning the error into "didn't run test setup" errors now? Like, it still won't click the button?

You're right! Good call!

Also a thought, if my assumption that a timeout during waiting for the page load event was always supposed to still try to click the button if it was found:

That version retains the unhandled Promise. Your change makes it demonstrably benign, but I'd rather avoid it altogether because it represents a refactoring hazard. (By the way, the Promise.race isn't necessary since loaded subsumes loadedTimeout for the cases which matter.)

I just pushed up an alternate solution; what do you think?

[gnarf]

(By the way, the Promise.race isn't necessary since loaded subsumes loadedTimeout for the cases which matter.)

I'm also not entirely convinced this is correct, though it also looks good for me to land if you don't agree with this analysis:

Given my read of the original intent here, the timeout was there to short circuit via the race if loaded was taking longer than AFTER_RUN_TEST_SETUP_BUTTON_DELAY. Without the race, it's going to always fail down to the timeout internally of the webdriver. Given as is now, these two times seem close enough that it probably doesn't make a difference, but I think the race is slightly "more correct"

My understanding is that the intent was:

1. Watch for loaded + microtask event callback. loaded
2. Start looking for button we plan to push.
3. Wait for first of loaded success or AFTER_RUN_TEST_SETUP_BUTTON_DELAY timeout
4. Resolve the button and click it.

without the race - step 3 changes to "wait for loaded or failure (after waiting for both the webdriver and the DELAY timeout)"

[gnarf]

Given how tricky this logic is with multiple async/await refactoring traps awaiting us, does it even make sense to start looking for the button to press before resolving the loaded (race or not?)

[jugglinmike]

Okay, I see that. It seems unnecessary to .catch(() => loadedTimeout), though; wouldn't catch(() => {}) be more clear?

Regarding a more fundamental change: I've been very strongly tempted to do that from the start, but given the age of this code, I'm not sure we'll be able to find a definitive motivation for the current design. Given the availability of a far better solution, I'm still (even after finding all these intricacies) inclined to preserve the current behavior for the time being.

[gnarf]

I think my reason for directing to the internal timeout in catch was assuming any other type of error could happen, it might happen before our internal timeout. My understanding of the purpose here was to wait for successful load detect, or timeout. Catch to empty could result in unsuccessful load event without timeout

[jugglinmike]

Superseded by gh-30.