Fix rustdoc gui tester #112962

GuillaumeGomez · 2023-06-23T13:50:50Z

Problem was that the main was always exiting with 0, whether or not there was an error. It led to failing GUI tests being ignored in the CI since no one saw them.

r? @klensy

rustbot · 2023-06-23T13:50:54Z

Failed to set assignee to klensy: invalid assignee

Note: Only org members, users with write permissions, or people who have commented on the PR may be assigned.

GuillaumeGomez · 2023-06-23T13:53:03Z

r? @ozkanonur

klensy · 2023-06-23T14:03:50Z

src/tools/rustdoc-gui-test/src/main.rs

-            try_run(&mut cargo, config.verbose);
+            if !try_run(&mut cargo, config.verbose) {


Looking at cause if this bug, i want to slap must_use over try_run and find all similar places.

That's a good idea. Doing that shortly.

onur-ozkan · 2023-06-23T14:09:37Z

src/tools/rustdoc-gui-test/src/main.rs

@@ -158,5 +161,5 @@ If you want to install the `browser-ui-test` dependency, run `npm install browse

    command.args(&config.test_args);

-    try_run(&mut command, config.verbose);
+    if try_run(&mut command, config.verbose) { Ok(()) } else { Err(()) }


What about using assert! with a simple error message on all try_run usages?

The error message would be less good because of the panic. I think try_run should return a Result instead of a boolean. Impossible to ignore it this way.

onur-ozkan · 2023-06-23T14:24:47Z

r=me with adding must_use on try_run

GuillaumeGomez · 2023-06-23T14:34:56Z

Like I said above, instead of relying on #[must_use], I instead made try_run return a Result<(), ()> so the type system itself prevents the returned value to be ignored.

onur-ozkan · 2023-06-23T14:48:02Z

Like I said above, instead of relying on #[must_use], I instead made try_run return a Result<(), ()> so the type system itself prevents the returned value to be ignored.

I thought you wouldn't want to do it in this PR. This is even better. Thanks a lot!

Will queue the PR once CI is green

onur-ozkan · 2023-06-23T15:46:09Z

@bors r+ rollup

bors · 2023-06-23T15:46:11Z

📌 Commit 7b55779 has been approved by ozkanonur

It is now in the queue for this repository.

…ter, r=ozkanonur Fix rustdoc gui tester Problem was that the `main` was always exiting with `0`, whether or not there was an error. It led to failing GUI tests being ignored in the CI since no one saw them. r? `@klensy`

…ter, r=ozkanonur Fix rustdoc gui tester Problem was that the `main` was always exiting with `0`, whether or not there was an error. It led to failing GUI tests being ignored in the CI since no one saw them. r? ``@klensy``

…ter, r=ozkanonur Fix rustdoc gui tester Problem was that the `main` was always exiting with `0`, whether or not there was an error. It led to failing GUI tests being ignored in the CI since no one saw them. r? ```@klensy```

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#112616 (Improve tests on targets without unwinding) - rust-lang#112643 (Always register sized obligation for argument) - rust-lang#112740 (Add link to rustdoc book search chapter in help popover) - rust-lang#112810 (Don't ICE on unnormalized struct tail in layout computation) - rust-lang#112870 (Migrate `item_bounds` to `ty::Clause`) - rust-lang#112925 (Stop hiding const eval limit in external macros) - rust-lang#112960 ([tests/rustdoc] Add `@files` command) - rust-lang#112962 (Fix rustdoc gui tester) r? `@ghost` `@rustbot` modify labels: rollup

jyn514 · 2023-06-28T19:26:05Z

src/bootstrap/run.rs

-fn try_run(builder: &Builder<'_>, cmd: &mut Command) -> bool {
+fn try_run(builder: &Builder<'_>, cmd: &mut Command) -> Result<(), ()> {
    if !builder.fail_fast {
-        if !builder.try_run(cmd) {
+        if let Err(e) = builder.try_run(cmd) {
            let mut failures = builder.delayed_failures.borrow_mut();
            failures.push(format!("{:?}", cmd));
-            return false;
+            return Err(e);
        }
    } else {
        builder.run(cmd);
    }
-    true
+    Ok(())


this function should not have changed. now fail_fast is broken because anything calling try_run will panic immediately instead of waiting for all other steps to finish running.

jyn514 · 2023-06-28T19:26:19Z

src/bootstrap/test.rs

+fn try_run(builder: &Builder<'_>, cmd: &mut Command) -> Result<(), ()> {
    if !builder.fail_fast {
-        if !builder.try_run(cmd) {
+        if let Err(e) = builder.try_run(cmd) {
            let mut failures = builder.delayed_failures.borrow_mut();
            failures.push(format!("{:?}", cmd));
-            return false;
+            return Err(e);
        }
    } else {
        builder.run(cmd);
    }
-    true
+    Ok(())


this also doesn't look right and no longer respects fail_fast.

…nur,jyn514 Don't fail early if `try_run` returns an error Fixes rust-lang#113208. Follow-up of rust-lang#112962. r? `@jyn514`

GuillaumeGomez added 2 commits June 23, 2023 15:50

Fix rustdoc-gui tester

616469a

Fix failing rustdoc GUI test

d70faf3

rustbot assigned onur-ozkan Jun 23, 2023

GuillaumeGomez mentioned this pull request Jun 23, 2023

rust-installer: migrate to clap 4.2, change to 2021 edition and fix few clippy lints #112884

Merged

klensy reviewed Jun 23, 2023

View reviewed changes

onur-ozkan reviewed Jun 23, 2023

View reviewed changes

GuillaumeGomez force-pushed the fix-rustdoc-gui-tester branch from 0fd01a4 to 9a78945 Compare June 23, 2023 14:43

This comment has been minimized.

Sign in to view

Make try_run return a Result<(), ()> instead of a boolean

7b55779

GuillaumeGomez force-pushed the fix-rustdoc-gui-tester branch from 9a78945 to 7b55779 Compare June 23, 2023 15:07

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 Jun 23, 2023

matthiaskrgr mentioned this pull request Jun 23, 2023

Rollup of 7 pull requests #112966

Closed

matthiaskrgr mentioned this pull request Jun 23, 2023

Rollup of 6 pull requests #112967

Closed

matthiaskrgr mentioned this pull request Jun 23, 2023

Rollup of 7 pull requests #112970

Closed

matthiaskrgr mentioned this pull request Jun 23, 2023

Rollup of 8 pull requests #112974

Merged

bors merged commit 9d7f297 into rust-lang:master Jun 23, 2023
11 checks passed

rustbot added this to the 1.72.0 milestone Jun 23, 2023

GuillaumeGomez deleted the fix-rustdoc-gui-tester branch June 24, 2023 10:29

jyn514 reviewed Jun 28, 2023

View reviewed changes

jyn514 mentioned this pull request Jun 30, 2023

bootstrap: --fail-fast doesn't work anymore #113208

Closed

GuillaumeGomez mentioned this pull request Jun 30, 2023

Don't fail early if try_run returns an error #113214

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix rustdoc gui tester #112962

Fix rustdoc gui tester #112962

GuillaumeGomez commented Jun 23, 2023 •

edited

Loading

rustbot commented Jun 23, 2023

GuillaumeGomez commented Jun 23, 2023

klensy Jun 23, 2023

GuillaumeGomez Jun 23, 2023

onur-ozkan Jun 23, 2023

GuillaumeGomez Jun 23, 2023

onur-ozkan commented Jun 23, 2023

GuillaumeGomez commented Jun 23, 2023

onur-ozkan commented Jun 23, 2023

This comment has been minimized.

onur-ozkan commented Jun 23, 2023

bors commented Jun 23, 2023

jyn514 Jun 28, 2023

jyn514 Jun 28, 2023

		try_run(&mut cargo, config.verbose);
		if !try_run(&mut cargo, config.verbose) {

Fix rustdoc gui tester #112962

Fix rustdoc gui tester #112962

Conversation

GuillaumeGomez commented Jun 23, 2023 • edited Loading

rustbot commented Jun 23, 2023

GuillaumeGomez commented Jun 23, 2023

klensy Jun 23, 2023

Choose a reason for hiding this comment

GuillaumeGomez Jun 23, 2023

Choose a reason for hiding this comment

onur-ozkan Jun 23, 2023

Choose a reason for hiding this comment

GuillaumeGomez Jun 23, 2023

Choose a reason for hiding this comment

onur-ozkan commented Jun 23, 2023

GuillaumeGomez commented Jun 23, 2023

onur-ozkan commented Jun 23, 2023

This comment has been minimized.

onur-ozkan commented Jun 23, 2023

bors commented Jun 23, 2023

jyn514 Jun 28, 2023

Choose a reason for hiding this comment

jyn514 Jun 28, 2023

Choose a reason for hiding this comment

GuillaumeGomez commented Jun 23, 2023 •

edited

Loading