Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # 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