fix: skip unloaded iframes for all apis #752
Conversation
| .options({ runOnly: ['label', 'frame-tested'] }) | ||
| .analyze(); | ||
|
|
||
| const timeout = await client.getTimeouts(); |
There was a problem hiding this comment.
Do you need to reset this in an after call?
There was a problem hiding this comment.
I shouldn't need to as the driver is recreated each test.
Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
Co-authored-by: Stephen Mathieson <571265+stephenmathieson@users.noreply.github.com>
|
Ops, forgot I hadn't pushed before retro started. Cleaned up the util function to not need the extra variable |
| }); | ||
| await Promise.race([timeoutPromise, executePromise]); | ||
| } catch { | ||
| throw new Error('Page/Frame is not ready'); |
There was a problem hiding this comment.
Isn't part of the point of making these changes that we do not throw an exception?
There was a problem hiding this comment.
We decided that the quickest way to resolve the bug was to return these iframes as frame-tested issues. This timeout / throw behavior is already being handled by the code to return frame-tested results and actually lets the code complete now.
Here's how each driver handled unloaded iframes previously:
- Puppeteer - hung indefinitely
- Webdriverjs - waited for the default page/script timeouts (300s and 30s respectively) before responding with an error
- Webdriverio - had both issues of puppeteer and webdriverjs depending on protocol
|
LGTM but I'll wait to approve until Dylan's question is answered. |
Implements the webdriverjs fix into all apis. Playwright was the only api that could handle unloaded iframes without issues, so it didn't need to do anything special and thus can give different results than the other apis if there are unloaded iframes on the page.
This fix doesn't fix the issue for legacy mode. If we want to support legacy mode and unloaded iframes we should do it in a separate pr.
Closes #727