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

Miri UI tests aren't run on CI (or locally) #110102

Closed
CraftSpider opened this issue Apr 9, 2023 · 8 comments · Fixed by #110177
Closed

Miri UI tests aren't run on CI (or locally) #110102

CraftSpider opened this issue Apr 9, 2023 · 8 comments · Fixed by #110177
Labels
A-miri Area: The miri tool A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Comments

@CraftSpider
Copy link
Contributor

Issue

As the title says - Miri UI tests aren't run when invoked through x.py, or on CI:
https://github.com/rust-lang-ci/rust/actions/runs/4636594388/jobs/8204646214#step:26:13316

The actual issue appears to be that compiletest.rs in the miri directory expects no non-filter args to be passed to it when being run, but the args -Z unstable-options --format json are getting passed.

Discovery

I was trying to test miri locally, and I ran the command x.py test src/tools/miri --stage 2. I expected the miri UI tests to get run, but instead, they were all always getting filtered out, despite not passing a filter. After looking into it a bit, I discovered the filter vector was always ["-Z", "unstable-options", "--format", "json"], and decided to check whether bors was running them.

@CraftSpider CraftSpider added the C-bug Category: This is a bug. label Apr 9, 2023
@jyn514 jyn514 added A-testsuite Area: The testsuite used to check the correctness of rustc A-miri Area: The miri tool T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Apr 9, 2023
@jyn514
Copy link
Member

jyn514 commented Apr 9, 2023

This seems ... pretty unfortunate 😓

cc @rust-lang/release @rust-lang/miri, I know miri is nightly only but it makes me pretty uncomfortable to be shipping tools that aren't running tests, even if rust-lang/miri is still running the tests.

@saethlin
Copy link
Member

saethlin commented Apr 9, 2023

If my shell history is anything to go by, this is recent breakage, and x test miri used to run all of Miri's tests.

@jyn514 jyn514 added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc labels Apr 9, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Apr 9, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Apr 9, 2023

Yea, this is not breakage on the miri side afaict, but must be a change on the cargo or bootstrap side.

I'm fine fixing this on our side tho. I can replicate the -- behaviour of the default test runner and adjust the wrappers in the miri repo to insert that

@CraftSpider
Copy link
Contributor Author

For making it follow normal -- behavior, I'm just replacing the skip(1).filter(...) in compiletest with skip_while(|arg| arg != "--") for now. I'd be willing to PR this change as a standalone if desired.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 10, 2023

For making it follow normal -- behavior, I'm just replacing the skip(1).filter(...) in compiletest with skip_while(|arg| arg != "--") for now. I'd be willing to PR this change as a standalone if desired.

Thanks! That would be great

saethlin added a commit to saethlin/miri that referenced this issue Apr 10, 2023
The panic test is now counted as an error test; we encounter a Terminate
terminator, and emit an interpreter error, as opposed to just
terminating due to a panic. So this test should have broken with
rust-lang/rust#102906 but wasn't because the Miri
test suite is currently broken in rust-lang/rust:
 rust-lang/rust#110102
@RalfJung
Copy link
Member

RalfJung commented Apr 10, 2023

Yeah I am pretty sure I tested this when we changed Miri to a subtree, and it worked back then.

How does looking for -- help? Looks to me like we'd have to know exactly which flags the regular test runner understands and parse them all the same way. E.g. -Zx filter should apply the filer but -Z filter should not. Which is of course ridiculous.

Can we stop bootstrap from passing wrong flags to test runners?

saethlin added a commit to saethlin/rust that referenced this issue Apr 11, 2023
The panic test is now counted as an error test; we encounter a Terminate
terminator, and emit an interpreter error, as opposed to just
terminating due to a panic. So this test should have broken with
rust-lang#102906 but wasn't because the Miri
test suite is currently broken in rust-lang/rust:
 rust-lang#110102
@RalfJung
Copy link
Member

I suggest that we change the test runner argument parsing to error out if there is any filter starting with - -- except after a --. So, something like:

// Arguments before the first '--' are filters, except if they start with `-` which are probably
// meant to be flags, and we don't support any flags.
for arg in args.as_ref().take_while(|arg| arg != "--") {
  if arg.starts_with("-") {
    eprintln!("Unknown command-line flag {arg}");
  }
  filters.push(arg);
}
// Append remaining arguments as filters
filters.extend(args);

@RalfJung
Copy link
Member

RalfJung commented Apr 11, 2023

The source of the regression is #108659.

Meanwhile #110177 should fix bootstrap invoking the Miri test suite.

bors added a commit to rust-lang/miri that referenced this issue Apr 13, 2023
compiletest: complain about unknown flags

This would have avoided rust-lang/rust#110102
bors added a commit to rust-lang/miri that referenced this issue Apr 13, 2023
compiletest: complain about unknown flags

This would have avoided rust-lang/rust#110102
@bors bors closed this as completed in b0884a3 Apr 14, 2023
@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Apr 14, 2023
oli-obk pushed a commit to oli-obk/miri that referenced this issue Apr 17, 2023
fix running Miri tests

This partially reverts rust-lang/rust#108659 to fix rust-lang/rust#110102: the Miri test runner does not support any flags, they are interpreted as filters instead which leads to no tests being run.

I have not checked any of the other test runners for whether they are having any trouble with these flags.

Cc `@pietroalbini` `@Mark-Simulacrum` `@jyn514`
RalfJung pushed a commit to RalfJung/rust that referenced this issue Apr 28, 2023
compiletest: complain about unknown flags

This would have avoided rust-lang#110102
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-miri Area: The miri tool A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants