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

[wdspec] Only resize and re-position window in session setup and teardown if needed #43853

Merged

Conversation

whimboo
Copy link
Contributor

@whimboo whimboo commented Jan 4, 2024

This is a workaround until the underlying bug in Chrome has been fixed. Potentially we can remove a lot of longer timeout requests as well. Lets see what tests results show us.

@whimboo whimboo force-pushed the chromedriver_window_bounds branch 3 times, most recently from 972af8e to a1802c1 Compare January 5, 2024 07:48
@whimboo
Copy link
Contributor Author

whimboo commented Jan 5, 2024

The Safari wpt.fyi results are somewhat busted because the webSocketURLis interpreted as 1instead of true:

webdriver.error.InvalidArgumentException: invalid argument (400): Unknown capability encountered: 'webSocketUrl' = '1'

This timeout only happens with my patch applied and I'm not able to reproduce it locally at all. Also the log output on Azure stops at that point, so maybe it's an issue with the provider?

@whimboo
Copy link
Contributor Author

whimboo commented Jan 5, 2024

Note that the runtime of the wpt-chrome-dev-results* jobs is now down from 56m 43s to just 27m 13s!

CC @OrKoN @jrandolf

@OrKoN
Copy link
Contributor

OrKoN commented Jan 5, 2024

@whimboo great, could it be the reason for the slowness we investigated previously?

@whimboo
Copy link
Contributor Author

whimboo commented Jan 5, 2024

@whimboo great, could it be the reason for the slowness we investigated previously?

Not sure which slowness you mean specifically but that is / was probably a longer term issue. But since recently when the prompt handling and network interception tests landed there are quite a few of extra Timeout results for BiDi and Chrome. Increasing the timeout here doesn't make sense given that the tests would still hang. So one of you might want to take a look at those.

@whimboo
Copy link
Contributor Author

whimboo commented Jan 5, 2024

The chrome stability job finished but looks like it missed synching and as such is still listed as running. I've checked the logs of the other jobs and everything looks fine.

@jgraham would you mind admin merge this PR? Thanks!

@jgraham jgraham merged commit 50c127e into web-platform-tests:master Jan 5, 2024
16 of 19 checks passed
@whimboo whimboo deleted the chromedriver_window_bounds branch January 5, 2024 17:15
@whimboo
Copy link
Contributor Author

whimboo commented May 21, 2024

@sadym-chromium and @nechaev-chromium I noticed this change in chromedriver from mid of March. Does that actually mean that we can remove the workaround as put in place with this PR?

@nechaev-chromium
Copy link
Contributor

Yes. The fix is supposed to eliminate the unconditional sleep on window bound change. There is no longer any performance benefit in checking the bounds before setting them in the tests.

@whimboo
Copy link
Contributor Author

whimboo commented May 21, 2024

@nechaev-chromium that is great to hear! Would you mind to provide a PR to get these changes reverted then? Thanks!

@whimboo
Copy link
Contributor Author

whimboo commented Jun 3, 2024

@nechaev-chromium that is great to hear! Would you mind to provide a PR to get these changes reverted then? Thanks!

I've created #46589

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants