Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: skip unloaded iframes for all apis #752

Merged
merged 6 commits into from
Jun 15, 2023
Merged

fix: skip unloaded iframes for all apis #752

merged 6 commits into from
Jun 15, 2023

Conversation

straker
Copy link
Contributor

@straker straker commented Jun 14, 2023

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

@straker straker requested a review from a team as a code owner June 14, 2023 21:55
Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple small points

.options({ runOnly: ['label', 'frame-tested'] })
.analyze();

const timeout = await client.getTimeouts();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need to reset this in an after call?

Copy link
Contributor Author

@straker straker Jun 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I shouldn't need to as the driver is recreated each test.

packages/webdriverio/src/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@stephenmathieson stephenmathieson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's reject promises rather than fulfilling them with an error.

packages/puppeteer/src/utils.ts Outdated Show resolved Hide resolved
packages/webdriverio/src/index.ts Outdated Show resolved Hide resolved
packages/webdriverio/src/utils.ts Outdated Show resolved Hide resolved
straker and others added 3 commits June 15, 2023 08:28
Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
Co-authored-by: Stephen Mathieson <571265+stephenmathieson@users.noreply.github.com>
@straker
Copy link
Contributor Author

straker commented Jun 15, 2023

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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't part of the point of making these changes that we do not throw an exception?

Copy link
Contributor Author

@straker straker Jun 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dylanb
dylanb previously requested changes Jun 15, 2023
Copy link
Contributor

@dylanb dylanb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the effect on an analyze call of throwing that exception? If it is going to cause the analyze call to throw, then I don't think that is the correct behavior

@stephenmathieson
Copy link
Member

LGTM but I'll wait to approve until Dylan's question is answered.

@axe-core axe-core mentioned this pull request Oct 12, 2023
@github-actions github-actions bot mentioned this pull request Oct 12, 2023
@dequejenn dequejenn mentioned this pull request Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Puppeteer hangs when page has lazy loaded iframe
4 participants