-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Run rustdoc-gui tests in parallel #86692
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
Run rustdoc-gui tests in parallel #86692
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
I extracted this from #86594 with @GuillaumeGomez |
src/tools/rustdoc-gui/tester.js
Outdated
} | ||
function print_test_erroneous() { | ||
// Bold Red "F" Reset | ||
process.stdout.write("\033[1m\x1b[31mF\x1b[0m"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That might work well for linux, but not on windows at all. ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wished that JavsScript had #[cfg(unix)]
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's the only thing you wished js had, I guess it's fine. 😉
For now, just keep it simple and simply print text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found an easy way to detect linux at runtime and enable the colour, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's completely beyond scope. I'd rather keep it as simple as possible. Please simply print the text (on stderr if it's an error).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
src/tools/rustdoc-gui/tester.js
Outdated
} | ||
}).catch(err => { | ||
console.error(err); | ||
error_outputs += err + "\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of putting everything into one string, it would maybe be better to keep an array of the tests and have some info for each, like output, errors, etc...
I also think we should disable running checks in parallel when |
I implemented sequential execution if the However, I was unable to make it run with ./x.py test --stage 1 --jobs 8 --no-headless --debug rustdoc-gui
Updating only changed submodules
Submodules updated in 0.03 seconds
Finished dev [unoptimized + debuginfo] target(s) in 0.15s
Unrecognized option: 'no-headless'
Usage: x.py <subcommand> [options] [<paths>...]
... |
Regarding ordering the logs, I perallocate an Array now that the async tests fill with a result object. This can happen in order with the The output then remains sorted with the exception of execution errors, which are at the end because I wanted people working with them to see them at the end of the output where they should be simpler to spot. |
Look here how to pass arguments to the tester through |
d5b4f1b
to
74ed2b2
Compare
74ed2b2
to
dc61d6c
Compare
bff8e1d
to
fa0c7a5
Compare
Co-authored-by: Guillaume Gomez <[email protected]>
591be15
to
c17628b
Compare
src/tools/rustdoc-gui/tester.js
Outdated
await tests[i]; | ||
} | ||
} | ||
await Promise.all(tests); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be:
await Promise.all(tests); | |
if (!no_headless) { | |
await Promise.all(tests); | |
} |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic wise kind of yes. But you can await a future multiple times, the work will get done only once
src/tools/rustdoc-gui/tester.js
Outdated
} | ||
await Promise.all(tests); | ||
// final \n after the tests | ||
console.log("\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be:
console.log("\n"); | |
console.log(""); |
? Or do you specifically want an empty line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, maybe better to use two console.log("")
instead I think. No strong opinion though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted a bit more separation between the sections for readability so the progress bar is visually spaced from the outputs.
The output should be the same on all platforms because we don't write anything on the empty line but fully correct it should be two console.log("")
, will change that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced it with a better finish method that prints the final stats
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions to be answered but otherwise it looks really promising!
Other test-suites use 100 but I don't care. We can always update it later. Looks all good to me now, thanks a lot! @bors: r+ |
📌 Commit 7f2b52b has been approved by |
…sts, r=GuillaumeGomez Run rustdoc-gui tests in parallel I hid the passing tests and only show the failed ones in alphabetical order:  Also this PR cuts down the execution time from ~40 to ~9 seconds
…sts, r=GuillaumeGomez Run rustdoc-gui tests in parallel I hid the passing tests and only show the failed ones in alphabetical order:  Also this PR cuts down the execution time from ~40 to ~9 seconds
Is this respecting -j passed to x.py somehow? We should avoid running multiple processes if -j1 is passed, for example. |
☀️ Test successful - checks-actions |
@Mark-Simulacrum Good point. I'll open an issue for it. |
I hid the passing tests and only show the failed ones in alphabetical order:

Also this PR cuts down the execution time from ~40 to ~9 seconds