-
Notifications
You must be signed in to change notification settings - Fork 361
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
bump ui test crate #3008
bump ui test crate #3008
Conversation
huh how did I manage to break mir-opt-level 4 tests? |
That's a lot of dashes... why?^^ (Also it's
One reason we did the split was so that the pass tests can run even when cargo-miri is borked, so we might want to keep the split. |
We can keep the directory split, but we can run all tests in one big runner (without accidentally allowing dep tests in the non-dep folder). Maybe I should have said "merge the test runners" not "merge the test suites" |
You can then still filter by directory, and the dependencies won't get built at all if the dep folder is not part of the filter |
idk yet 😆 something with the test runner |
uh... those are broken for me locally on master, too |
63b608c
to
3ee9f49
Compare
Ah, fancy. I guess I'll see how that works. :) |
One set of Maybe we should use an env var scheme so that we can have |
because it means CI will not show anything (same reason we don't use |
Well CI can use a non-default config. But honestly I prefer the version without the dots. ;) So I'm fine with non-quiet being the default. I gave it quick shot locally. Some first feedback:
I don't know where one test ends and the next one begins. "Too many open files" sounds like maybe it went a bit overboard with the concurrency or so?
libtest prints the |
uh. I have 96 cores and I don't get this issue... weird. Not sure how to investigate this :/ |
Also filtering doesn't seem to work any more: On my next attempt I am getting some even wilder errors:
That file definitely exists. |
that's an internal check, not about the file. Uh... what XD I want your desktop's thread switching capabilities, they seem to be great at fuzzing |
oh yea, that's the |
You might be running into clap-rs/clap#5055. |
I don't know what OS yall are on, but MacOS has a default open files limit that is shockingly low. |
I really don't want to start figuring out the open file limit. Maybe a few quiet retries (bounded to a hard limit) with some sleeps will make it work out? |
I replicated compiletest-rs behaviour. I'll switch to mirroring libtest |
I can look into this later, or we could invite someone else to look into it. |
I'm on Linux. I've never seen "too many open files" before so retry to me files like it would paper over whatever is going on... Also, Linux error code are awfully unspecific. Can I get it to tell me which operation raised the error? It might also appear when spawning too many threads too quickly or whatever. |
it's probably during child process spawning and setting up pipes to it (from your panic line and message). |
I would/will figure this out by using |
Everything but the spurious failures and
has been fixed now. I got rid of |
Nice. :) I am still seeing errors, though new ones:
Also there are some glitches while tests are running. "at " or so? Text that very briefly appears in a few places. After Ctrl-C only tiny parts of it are left
|
there shouldn't be. These lines are from filtered backtraces from when miri interprets a panic. Any test that takes longer than 1s starts putting its last line from stderr after the test spinner
|
Uh, this thread issue is in miri not in ui_test. |
No, I have never seen this before. Only with a |
Yeah I've seen that. The reports I linked are this happening in rustc.
Threads are basically processes that share the address space on Linux, so I wouldn't be surprised if the process limit also includes threads. |
oh that makes sense. I guess it'll be fine then if ppl reduce the number of threads or increase their |
If the default limits are fine then I wouldn't worry about it. |
Yes, I would only worry about defaults on common dev systems. So like Ubuntu or whatever common Linux with Manually lowering the resource limits can break all kinds of software, including systemd. So let's handle users being creative if anyone reports a problem. Same deal with a 448-vCPU VM. |
Oh great I need a windows host to reproduce the i686-pc-windows-msvc failure. |
Found a windows host. CI passes, locally tests pass with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks very nice here. :) Please squash all the "bump ui-test" commits though.
tests/compiletest.rs
Outdated
std::env::var("MIRI_TEST_THREADS").map_or_else( | ||
|_| std::thread::available_parallelism().unwrap(), | ||
|threads| NonZeroUsize::new(threads.parse().unwrap()).unwrap(), | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please write this as a match
, I find map_or_else
much harder to read than a direct match
☔ The latest upstream changes (presumably #2972) made this pull request unmergeable. Please resolve the merge conflicts. |
684c291
to
80dd9a2
Compare
80dd9a2
to
78b4c6c
Compare
I squashed all the chained ui test bumps, but left the ones that had other changes in between. It also helps having individual commits to review the changes required by the bumps instead of skipping over too many versions at once |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this locally for a bit and it looks great, thanks a ton for all the work you put into this. :) Just got some nits here.
@@ -1,5 +1,5 @@ | |||
$DIR/backtrace-api-v0.rs:24:14 (func_d) | |||
$DIR/backtrace-api-v0.rs:20:5 (func_c) | |||
$DIR/backtrace-api-v0.rs:9:5 (func_b) | |||
$DIR/backtrace-api-v0.rs:9:5 (func_b::<u8>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fascinating, why does this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have absolutely no clue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I think I changed some default path filters and they may affect this but I don't know why)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like somehow this filter no longer applies?
//@normalize-stderr-test: "::<.*>" -> "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah wait, that's a stderr filter. Was there a bug previously where those applied to stdout as well?
EDIT: Yes, there was. So this is a bugfix in ui_test. Nice. :) Does the ui-test test suite have a test to ensure normalization is for stdout/stderr only?
@bors r+ |
☀️ Test successful - checks-actions |
The recommended way to run tests locally is
./miri bless -- -- --quiet
, which will showstderr
and the last line will be shown after the test name and updated every few hundred milliseconds.As a side effect this PR also fixes #2998 and only builds dependencies if any tests actually need them (this means that with the next ui_test update we'll be able to merge all our test suites).
Also fixes #3052.