Fix flaky test #81197
Closed
Fix flaky test #81197
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jyn514
added
A-testsuite
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
Oh boo, rustc isn't available on CI because the only compiler available is the one built from source, which isn't in PATH. Maybe we should just remove this test? |
|
A run-make test should be able to do provide the rustc path correctly. |
|
Thanks, done. I had to build rustdoc for run-make tests. |
|
@bors p=5 This is causing spurious failures. |
jyn514
added a commit
to jyn514/rust
that referenced
this pull request
Jan 21, 2021
See rust-lang#81197 for what's going on here; this is a temporary stopgap until someone has time to review the proper fix.
Merged
camelid
reviewed
Jan 21, 2021
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jan 21, 2021
Remove flaky test See rust-lang#81197 for what's going on here; this is a temporary stopgap until someone has time to review the proper fix. r? `@ghost`
Mark-Simulacrum
approved these changes
Jan 22, 2021
src/bootstrap/test.rs
Outdated
| @@ -1003,7 +1003,7 @@ note: if you're sure you want to do this, please open an issue as to why. In the | |||
|
|
|||
| // Avoid depending on rustdoc when we don't need it. | |||
| if mode == "rustdoc" | |||
| || (mode == "run-make" && suite.ends_with("fulldeps")) | |||
| || mode == "run-make" | |||
There was a problem hiding this comment.
I would prefer to avoid this and instead move the test to fulldeps.
There was a problem hiding this comment.
That seems a shame when this doesn't actually need a stage 2 build :/ that means you have to compiler rustc twice to run the test at all. Rustdoc only takes about a minute to build even with a clean cache.
There was a problem hiding this comment.
I don't want to make this change in this PR. We can consider it separately.
There was a problem hiding this comment.
Ok, I reverted the change. I do think it would be useful though - #81223 could also use it, and that test is much more likely to fail, so building the compiler twice is painful.
Mark-Simulacrum
added
S-waiting-on-author
jyn514
added
S-waiting-on-review
|
r=me with commits squashed |
Mark-Simulacrum
added
S-waiting-on-author
Rustdoc writes to a pipe, and if the program on the other side has exited, it will exit with SIGPIPE. To avoid that happening, call `rustc` instead of `true`, which will wait to exit until it sees EOF on stdin. - Use a run-make-fulldeps test so that rustc is available in CI This is not exactly right, because the test shouldn't require building rustc twice, but it avoids having to build rustdoc for run-make tests.
|
@bors r=Mark-Simulacrum rollup |
Mark-Simulacrum
added
S-waiting-on-author
AIUI the program we specify here gets passed rustc flags. So we need something that:
rustc seems like the obvious answer. Maybe we are getting the stage0 rustc and we need a stage1? |
|
Yeah I'm afraid I don't have the time to do a deep dive into why the rustc being passed isn't found; I would recommend starting by making sure it is getting passed by printing out the command rustdoc tries to run when it fails... |
JohnCSimon
added
S-waiting-on-author
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Mar 9, 2021
…rk-Simulacrum Build rustdoc for run-make tests, not just run-make-fulldeps Rustdoc almost never needs a full stage 2 compiler, and requiring rustdoc tests to be in run-make-fulldeps adds a lot of compile time for no reason. This is the same change from rust-lang#81197, but separated into its own PR. I ran into this again today while working on rust-lang/docs.rs#1302. r? `@Mark-Simulacrum`
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Mar 9, 2021
…rk-Simulacrum Build rustdoc for run-make tests, not just run-make-fulldeps Rustdoc almost never needs a full stage 2 compiler, and requiring rustdoc tests to be in run-make-fulldeps adds a lot of compile time for no reason. This is the same change from rust-lang#81197, but separated into its own PR. I ran into this again today while working on rust-lang/docs.rs#1302. r? ``@Mark-Simulacrum``
JohnTitor
added a commit
to JohnTitor/rust
that referenced
this pull request
Mar 9, 2021
…rk-Simulacrum Build rustdoc for run-make tests, not just run-make-fulldeps Rustdoc almost never needs a full stage 2 compiler, and requiring rustdoc tests to be in run-make-fulldeps adds a lot of compile time for no reason. This is the same change from rust-lang#81197, but separated into its own PR. I ran into this again today while working on rust-lang/docs.rs#1302. r? ```@Mark-Simulacrum```
JohnCSimon
added
S-waiting-on-author
rustbot
added
S-waiting-on-review
|
@JohnCSimon This fails when tested on windows: #81197 (comment) @rustbot label: +S-waiting-on-author -S-waiting-on-review |
rustbot
added
S-waiting-on-author
I've tried to do this for about 15 minutes now and I'm really stumped - is there a way to show the output when a run-make test passes? Here's what I've tried so far: diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs
index 86d940cd733..9a33d952ff1 100644
--- a/src/bootstrap/test.rs
+++ b/src/bootstrap/test.rs
@@ -1257,7 +1257,7 @@ fn run(self, builder: &Builder<'_>) {
cmd.arg("--verbose");
}
- if !builder.config.verbose_tests {
+ if !builder.config.verbose_tests && !builder.is_verbose() {
cmd.arg("--quiet");
}
@@ -1808,7 +1808,7 @@ fn run(self, builder: &Builder<'_>) {
cargo.arg("--");
cargo.args(&builder.config.cmd.test_args());
- if !builder.config.verbose_tests {
+ if !builder.config.verbose_tests && !builder.is_verbose() {
cargo.arg("--quiet");
}
@@ -1921,7 +1921,7 @@ fn run(self, builder: &Builder<'_>) {
dylib_path.insert(0, PathBuf::from(&*builder.sysroot_libdir(compiler, target)));
cargo.env(dylib_path_var(), env::join_paths(&dylib_path).unwrap());
- if !builder.config.verbose_tests {
+ if !builder.config.verbose_tests && !builder.is_verbose() {
cargo.arg("--quiet");
}
@@ -1992,7 +1992,7 @@ fn run(self, builder: &Builder<'_>) {
cargo.arg("'-Ctarget-feature=-crt-static'");
}
- if !builder.config.verbose_tests {
+ if !builder.config.verbose_tests && !builder.is_verbose() {
cargo.arg("--quiet");
}
|
|
Also, if there's a way to get CI to run tests for Windows instead of Linux on PR builds, that would be super helpful. |
crlf0710
added
S-waiting-on-author
I think it's possible. Maybe just edit the CI related configurations temporarily in this PR... and revert the changes after it's ready. |
|
I'm not planning to spend any more time on this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rustdoc writes to a pipe, and if the program on the other side has
exited, it will exit with SIGPIPE. To avoid that happening, call
rustcinstead of
true, which will wait to exit until it sees EOF on stdin.cc #79705 (comment), #80924 (comment)
r? @ijackson