Skip to content
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

ui test with ICE does not fail #169

Closed
RalfJung opened this issue Apr 6, 2019 · 6 comments · Fixed by #172
Closed

ui test with ICE does not fail #169

RalfJung opened this issue Apr 6, 2019 · 6 comments · Fixed by #172

Comments

@RalfJung
Copy link
Contributor

RalfJung commented Apr 6, 2019

We are using this in Miri, and I just have the situation that one of our UI tests ICEs when I call it directly but the test is considered passing by compiletest. I have copy-pasted the command-line (as it gets printed when I make the test really fail, e.g. by introducing a syntax error) to make sure it is not some weird extra flag.

On a possibly related note, when I run the test binary directly (target/debug/deps/compiletest-edd6bd72083f7b79), all tests should fail because Miri complains about a missing shared library. Instead, only those tests with a non-empty stdout/stderr reference file fail, complaining that stdout/stderr was empty instead. This is particularly curious since stderr is not actually empty:

normalized stderr:


expected stderr:
a


diff of stderr:

-a
-

The actual stderr differed from the expected stderr.
Actual stderr saved to /tmp/compiletest9fDVl0/fn_box.stderr
To update references, run this command from build directory:
tests/run-pass/update-references.sh '/tmp/compiletest9fDVl0' 'fn_box.rs'

error: 1 errors occurred comparing output.
status: exit code: 127
command: "target/debug/miri" "tests/run-pass/fn_box.rs" "-L" "/tmp/compiletest9fDVl0" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-C" "prefer-dynamic" "-o" "/tmp/compiletest9fDVl0/fn_box.stage-id" "-Dwarnings" "-Dunused" "--edition" "2018" "-L" "/tmp/compiletest9fDVl0/fn_box.stage-id.aux" "-A" "unused"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
target/debug/miri: error while loading shared libraries: librustc_driver-3f5e8a7f565acbfe.so: cannot open shared object file: No such file or directory

Compare "normalized stderr" and "stderr".

I have no idea how this is possible.

@RalfJung
Copy link
Contributor Author

RalfJung commented Apr 7, 2019

Okay, so the reason that "normalized stderr" and "stderr" are so different is about something with JSON stuff... if there is no --error-format, it tries to do something smart with JSON, and that can have really surprising consequences. Namely it looks like when rustc ICEs, that is not printed in JSON format even with --error-format json and hence json::extract_rendered just returns the empty string.

So, when there is a test that expects empty stderr, and that test ICEs, compiletest normalizes the stderr to empty and considers the test passed.

Cc @gnzlbg @alexcrichton

@RalfJung
Copy link
Contributor Author

RalfJung commented Apr 7, 2019

@laumann looks like runtest.rs in rustc master has check_if_test_should_compile that would catch cases like this. Any chance you could update the copy in this repo?

I am still not sure though if the behavior around normalization and JSON is intended. This all looks terribly fragile...

@RalfJung
Copy link
Contributor Author

RalfJung commented Apr 7, 2019

FWIW, it is only due to this bug that Miri currently looks green in the toolstate tracking. Actually, some of the tests fail with an ICE since rust-lang/rust#59500 landed.

@phansch
Copy link
Contributor

phansch commented Apr 7, 2019

I'm not sure if that works for Miri, but in Clippy I'm currently using // run-pass as a workaround. A test that can't compile will not be runnable, so compiletest will fail at that point because it can't find the compiled file. But yes, it would be good to fix the core issue here.

@RalfJung
Copy link
Contributor Author

RalfJung commented Apr 7, 2019

I guess Miri could do that, but we don't actually want to run these things so that seems like a waste of test time.

See #172 / rust-lang/rust#59769 for a possible fix.

phansch added a commit to phansch/rust-clippy that referenced this issue Apr 8, 2019
Handling the integer parsing properly instead of just unwrapping.

Note that the test is not catching the ICE because plain UI tests
[currently hide ICEs][compiletest_issue]. Once that issue is fixed, this
test would fail properly again.

[compiletest_issue]: Manishearth/compiletest-rs#169
bors added a commit to rust-lang/rust-clippy that referenced this issue Apr 10, 2019
Fix ICE in decimal_literal_representation lint

Handling the integer parsing properly instead of just unwrapping.

Note that the test is not catching the ICE because plain UI tests
[currently hide ICEs][compiletest_issue]. Once that issue is fixed, this
test would fail properly again.

Fixes #3891

[compiletest_issue]: Manishearth/compiletest-rs#169
Centril added a commit to Centril/rust that referenced this issue Apr 13, 2019
… 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.
Centril added a commit to Centril/rust that referenced this issue Apr 13, 2019
… 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.
bors added a commit to rust-lang/rust that referenced this issue Apr 14, 2019
…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.
bors added a commit to rust-lang/rust that referenced this issue Apr 16, 2019
…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.
@RalfJung
Copy link
Contributor Author

Awesome. :) How much work would it be to make a release with this fix?

djrenren pushed a commit to djrenren/compiletest that referenced this issue Aug 26, 2019
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants