-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Panic (attempt to subtract with overflow
) when calculating the number of passed_tests
in utils\render_tests.rs
#136607
Comments
/cc @ranger-ross from #135355 |
It looks like there are multiple issues here. One is that the stdout is getting corrupted because multiple threads are writing to it at once. You can see this here:
bootstrap misses that JSON message because it got mixed with some other output. I don't know what is printing all that The other issue is that Because it missed one of the messages, it did not reach 100% in the percentage. When it runs the builtin-clone test, the |
The tests that are writing to the output are There's still a bunch of other tests that are printing panic messages from spawned threads (like |
This comment has been minimized.
This comment has been minimized.
Rollup merge of rust-lang#136630 - jieyouxu:render_tests, r=ChrisDenton Change two std process tests to not output to std{out,err}, and fix test suite stat reset in bootstrap CI test rendering I don't really know how to test if this unbreaks CI (since rust-lang#136607 reported that this breaks the CI test rendering *sometimes*). Fixes rust-lang#136607. r? `@ChrisDenton` (two Windows process tests, but feel free to reroll)
I'm going to reopen because there are two issues here. The other is that the counters never get reset between test suites, which means they can show incorrect results and possibly break in the same way depending on how the tests are organized. |
@ehuss AFAIK they should be reset if we receive a new test suite start message now: rust/src/bootstrap/src/utils/render_tests.rs Lines 309 to 319 in 942db67
Did I miss anything? Or is there another example I can use to test? |
Oh, I'm sorry! I did not look at the PR, and I think the title got truncated on my thing and I just assumed it was just updating the tests. My bad! |
Change two std process tests to not output to std{out,err}, and fix test suite stat reset in bootstrap CI test rendering I don't really know how to test if this unbreaks CI (since rust-lang#136607 reported that this breaks the CI test rendering *sometimes*). Fixes rust-lang#136607. r? `@ChrisDenton` (two Windows process tests, but feel free to reroll)
I apologize for not following the bug template + for not having a consistent repro. I hope that opening this issue will still help (writing down and sharing what we know, looping in the author of the line that panics, etc.).
It seems that when running
rust
tests on Windows, the test binary sometimes (flakily, inconsistently) panics saying (based on two cases when it happened recently - see here and here:So far this has only happened on Windows (and not on Linux). And it seems to only be happening when running tests from
library/std/tests/builtin-clone.rs
(which contain only a single testcase).It seems that the line that panics has been introduced relatively recently (around a month ago) in dc11857#diff-218273cbe7a566fb9d2f12187307eb7fd0e4664abd7c375f0f4cd30183ea78c9R230:
I guess the panic indicates that
self.executed_tests
is smaller than the sum ofself.failures.len()
andself.ignored_tests
. This seems rather unexpected.FWIW This issue is tracked in Chromium's issue tracker as https://crbug.com/392978169.
The text was updated successfully, but these errors were encountered: