-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
compiletest normalization: preserve non-JSON lines such as ICEs #59769
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
One run-pass test fails:
This does not seem very stable. I can normalize away these lines, but even then the normalized output is still a given number of empty lines, and that will likely change as well. Is there a way to disable output comparison entirely, or to also normalize away the redundant empty lines that get created here? I tried
but that does not seem to work. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Actually it does work, but there is the following line in the output:
That seems to come from compiletest itself, which does not want to capture output this big. However, the UI tests seem to be annoyingly non-deterministic. I had run them with |
d6bb5e2
to
be83bd5
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
r=me with passing tests |
I think I need help with the tests. This is basically blocked on #59774: rustc currently non-deterministically sometimes prints
and with this change, that now becomes a reason for test failures. |
Could that test perhaps be ignored with a FIXME pointing to the issue? |
It's a small handful of tests, not just one -- and there might be more, because this is so highly non-deterministic. Did you mean ignored as in |
This isn't a regression, right? It's just exposing already-existing behavior? And yeah I'd imagine using If you would prefer to not use |
Well it's a regression in terms of test coverage. I think normalizing is preferrable over ignoring the test entirely. I added normalization for all tests that I saw fail this way locally or on CI. But it seems very random, so I fear that this might affect future PRs. |
9f9dc76
to
8861232
Compare
@bors: r+ |
📌 Commit 8861232 has been approved by |
🌲 The tree is currently closed for pull requests below priority 15, this pull request will be tested once the tree is reopened |
… r=alexcrichton compiletest normalization: preserve non-JSON lines such as ICEs Currently, every non-JSON line from stderr gets normalized away when compiletest normalizes the output. In particular, ICEs get normalized to the empty output. That does not seem desirable, so this changes normalization to preserve non-JSON lines instead. Also see Manishearth/compiletest-rs#169: because of that bug, Miri currently *looks* green in the toolstate, but some tests ICE. That same bug is likely no longer present in latest compiletest because the error code gets checked separately, but it still seems like a good idea to also make sure that ICEs are considered stderr output: This change found an accidental user-visible `error!` in CTFE validation (fixed), and a non-deterministic panic when there are two `main` symbols (not fixed, no idea where this comes from). Both got missed before because non-JSON output got ignored.
Rollup of 9 pull requests Successful merges: - rust-lang#59655 (Use a proc macro to declare preallocated symbols) - rust-lang#59769 (compiletest normalization: preserve non-JSON lines such as ICEs) - rust-lang#59776 (Apply resource-suffix to search-index and source-files scripts as well) - rust-lang#59784 (Suggest importing macros from the crate root) - rust-lang#59812 (Exclude profiler-generated symbols from MSVC __imp_-symbol workaround.) - rust-lang#59856 (update polonius-engine) - rust-lang#59874 (Clean up handling of `-Z pgo-gen` commandline option.) - rust-lang#59890 (Don't generate empty json variables) - rust-lang#59911 (Revert "compile crates under test w/ -Zemit-stack-sizes") Failed merges: r? @ghost
📌 Commit 581c1ab has been approved by |
@bors p=2 r0llup planning. |
⌛ Testing commit 581c1ab with merge 3b20e3419976e09c1429a2b73e3f8792d76c6072... |
💔 Test failed - checks-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Interesting, seems like it's not Windows-only after all but also affects macOS. I'll just make it normalize on all platforms then I guess. |
@bors r=alexcrichton |
📌 Commit 28c4397 has been approved by |
…hton compiletest normalization: preserve non-JSON lines such as ICEs Currently, every non-JSON line from stderr gets normalized away when compiletest normalizes the output. In particular, ICEs get normalized to the empty output. That does not seem desirable, so this changes normalization to preserve non-JSON lines instead. Also see Manishearth/compiletest-rs#169: because of that bug, Miri currently *looks* green in the toolstate, but some tests ICE. That same bug is likely no longer present in latest compiletest because the error code gets checked separately, but it still seems like a good idea to also make sure that ICEs are considered stderr output: This change found an accidental user-visible `error!` in CTFE validation (fixed), and a non-deterministic panic when there are two `main` symbols (not fixed, no idea where this comes from). Both got missed before because non-JSON output got ignored.
☀️ Test successful - checks-travis, status-appveyor |
Tested on commit rust-lang/rust@3b27b4f. Direct link to PR: <rust-lang/rust#59769> 💔 rls on windows: test-pass → test-fail (cc @nrc @Xanewok, @rust-lang/infra).
Currently, every non-JSON line from stderr gets normalized away when compiletest normalizes the output. In particular, ICEs get normalized to the empty output. That does not seem desirable, so this changes normalization to preserve non-JSON lines instead.
Also see Manishearth/compiletest-rs#169: because of that bug, Miri currently looks green in the toolstate, but some tests ICE. That same bug is likely no longer present in latest compiletest because the error code gets checked separately, but it still seems like a good idea to also make sure that ICEs are considered stderr output:
This change found an accidental user-visible
error!
in CTFE validation (fixed), and a non-deterministic panic when there are twomain
symbols (not fixed, no idea where this comes from). Both got missed before because non-JSON output got ignored.