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

Reduce wait-for instructions for rustdoc GUI tests #95172

Merged
merged 2 commits into from
Mar 26, 2022

Conversation

GuillaumeGomez
Copy link
Member

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Mar 21, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 21, 2022
@notriddle
Copy link
Contributor

notriddle commented Mar 21, 2022

I’d really rather not mess with these numbers unless someone’s complaining about them becoming the long pole in the test suite. Making them shorter will increase the number of intermittent failures, when these tests run on a computer with a noisy neighbor.

Could these tests be tweaked to poll for some criteria to be fulfilled, instead of just waiting for a fixed amount of time?

For example, could the wait-for instruct be modified to take both a time and a selector?

@GuillaumeGomez
Copy link
Member Author

It already can. It's just that in some cases, waiting for a selector isn't working because the CSS changes associated to it haven't been computed by the browser yet. I only changed the biggest ones because the 1 second wait for example is just way too much.

@notriddle
Copy link
Contributor

Could assert-css be extended with a timeout parameter, so that instead of just failing the first time it observed a failing match, it can instead poll for a while until it either observed a match or times out and fails?

@GuillaumeGomez
Copy link
Member Author

That could. Not sure if it's a good idea though...

@notriddle
Copy link
Contributor

I've done browser UI tests (not with your framework, just Selenium-PHP), and wound up with a test suite that took hours to run while consuming less than 1% CPU. Redesigning it to use a function that pretty much does what "assert-css with a timeout param" would do improved it a lot, cutting runtime by more than half, while also pretty much eliminating intermittent test failures.

But, on the other hand, that project was strange in a lot of ways, so I'm not sure how reflective of rustdoc it'll be.

@GuillaumeGomez
Copy link
Member Author

I could add a command wait-for-css instead. That would make more sense.

@notriddle
Copy link
Contributor

Sure, that would be great. Thanks!

@GuillaumeGomez
Copy link
Member Author

I opened GuillaumeGomez/browser-UI-test#286. I'll try to do it in the next days.

@GuillaumeGomez
Copy link
Member Author

@notriddle Thanks to your great suggestion, almost all "random amount of time" waits have been removed! \o/

@notriddle
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 25, 2022

📌 Commit e0a697a has been approved by notriddle

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 25, 2022
@bors
Copy link
Contributor

bors commented Mar 25, 2022

⌛ Testing commit e0a697a with merge a2ebd5a...

@bors
Copy link
Contributor

bors commented Mar 26, 2022

☀️ Test successful - checks-actions
Approved by: notriddle
Pushing a2ebd5a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 26, 2022
@bors bors merged commit a2ebd5a into rust-lang:master Mar 26, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 26, 2022
@GuillaumeGomez GuillaumeGomez deleted the reduce-wait-for branch March 26, 2022 11:02
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a2ebd5a): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants