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

Simplify --no-headless option for rustdoc-gui tester #91391

Merged
merged 1 commit into from
Dec 6, 2021

Conversation

GuillaumeGomez
Copy link
Member

It adds a message stating the change for the concurrency and also remove the extra condition when running the tests.

r? @camelid

@GuillaumeGomez GuillaumeGomez added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Nov 30, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 30, 2021
@camelid
Copy link
Member

camelid commented Nov 30, 2021

Sorry, I'm not familiar with the GUI tester.

r? @jsha

@rust-highfive rust-highfive assigned jsha and unassigned camelid Nov 30, 2021
@GuillaumeGomez
Copy link
Member Author

r? @notriddle

@notriddle
Copy link
Contributor

I'm not very familiar with how this tool works, either.

@notriddle
Copy link
Contributor

r? @jsha

@GuillaumeGomez
Copy link
Member Author

I'm not sure if @jsha is either. Oh well, we'll see. :)

@notriddle
Copy link
Contributor

BTW, this does seem like a reasonable change to me. If you're using a "headed" browser, running more than one test at a time can create window-manager-related bugs, and nobody wants to deal with that.

@GuillaumeGomez
Copy link
Member Author

It was actually already the case, I simply added some log about it and unified the code a bit more.

@jsha
Copy link
Contributor

jsha commented Dec 1, 2021

I'm okay reviewing this. I've looked a little bit at the GUI tester and have done some Node before.

@GuillaumeGomez
Copy link
Member Author

Don't hesitate if you have questions then!

@jsha
Copy link
Contributor

jsha commented Dec 5, 2021

@bors r+ rollup

Looks good! One comment on variable naming, which doesn't block merging:

} else if (!no_headless) {

When I see a statement like this I read it as "not no headless", which is a double negative and kind of confusing. Instead you can name the variable headless and invert the sense:

let headless = true;
...
        if (opts["no_headless"]) {
            args.push("--no-headless");
            headless = false;
        }

@bors
Copy link
Contributor

bors commented Dec 5, 2021

📌 Commit 1feeda8 has been approved by jsha

@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 Dec 5, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 6, 2021
…askrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#91367 (Fix ICE in `check_must_not_suspend_ty()`)
 - rust-lang#91391 (Simplify --no-headless option for rustdoc-gui tester)
 - rust-lang#91537 (compiler/rustc_target: make m68k-unknown-linux-gnu use the gnu base)
 - rust-lang#91554 (Update doc about code block edition attributes)
 - rust-lang#91563 (Bump download-ci-llvm-stamp for LLD inclusion)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9c777fc into rust-lang:master Dec 6, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 6, 2021
@GuillaumeGomez GuillaumeGomez deleted the simplify-no-headless branch December 6, 2021 09:19
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 15, 2021
…tester-code, r=jsha

Improve code for rustdoc-gui tester

Following advice given in rust-lang#91391.

It nicely improves the code readability. :)

r? `@jsha`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

7 participants