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

Bump ui test to 0.26 #3866

Merged
merged 4 commits into from
Sep 17, 2024
Merged

Bump ui test to 0.26 #3866

merged 4 commits into from
Sep 17, 2024

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Sep 7, 2024

lots of issues fixed and various improvements

🤞 that it works this time around

@oli-obk oli-obk force-pushed the ui_test_26_1 branch 2 times, most recently from 39771ca to 1fdb363 Compare September 8, 2024 08:01
tests/ui.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented Sep 8, 2024 via email

tests/ui.rs Outdated Show resolved Hide resolved
@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 8, 2024

uh... can someone bless the native lib tests? I don't have a linux machine

@eduardosm
Copy link
Contributor

There you go: miri-bless.diff.zip

@RalfJung
Copy link
Member

RalfJung commented Sep 8, 2024 via email

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 8, 2024

There you go: miri-bless.diff.zip

Thanks!

macOS should also work for the native lib tests.

I don't have a unix machine and don't want to figure out wsl 😆

@RalfJung
Copy link
Member

RalfJung commented Sep 8, 2024 via email

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 8, 2024

Yea, they work great modulo bat files having a prompt when cancelled with Ctrl + C

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 8, 2024

Oh fun. Line ending failures I think

ci/ci.sh Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2024

Sadly github makes this PR basically unreviewable... I told it to hide all stderr differences but it still only loads the first half of the .rs file diffs, making it extremely tedious to look at the remaining .rs file diffs. Is there some git command to exclude some file name patterns from a commit diff?

EDIT: yes, there is

git show FETCH_HEAD -- ':(exclude)*.stderr'

Cargo.toml Outdated Show resolved Hide resolved
tests/ui.rs Outdated Show resolved Hide resolved
tests/ui.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2024

Oh fun. Line ending failures I think

This fails locally even when I pass --bless. In fact something seems quite wrong, since I am getting

error: actual output differed from expected

when --bless is set.

The --bless flag is turned into the RUSTC_BLESS env var, and that seems to be ignored now.

tests/ui.rs Outdated Show resolved Hide resolved
@RalfJung RalfJung changed the title Bump ui test to 0.26.1 Bump ui test to 0.26 Sep 9, 2024
@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2024

I can run the native tests with --bless and push that to this branch -- once --bless works again.

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 9, 2024

I'll fix that, tho you can use -- -- --bless until then

@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2024

Lolwhat?^^ Two -- makes no sense to me... ./miri does not eat a --, in fact it adds an extra -- before the flags you give and then passes that to cargo test.

But yeah, that actually worked. :)

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 9, 2024

Yea, we need to escape cargo test and then escape the normal test binaries, and finally the ui_test harness eats that flag

@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2024

A single -- should work then since ./miri test already escapes cargo test.

@RalfJung
Copy link
Member

RalfJung commented Sep 9, 2024

Seems like this test...

//@only-target: x86_64: uses x86 target features
//@ignore-target: x86_64-apple-darwin: that target actually has ssse3

... is now being run on x86_64-apple-darwin, which it should not be.

@oli-obk oli-obk force-pushed the ui_test_26_1 branch 5 times, most recently from c44aaff to 1faf38d Compare September 11, 2024 10:52
@RalfJung RalfJung added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Sep 11, 2024
Comment on lines 502 to 513
// For `--dep` we need to set the env var instead of passing the flag.
if !dep {
if let Some(target) = &target {
early_flags.push("--target".into());
early_flags.push(target.into());
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm now it's not setting the target anymore, before it was setting it twice. not sure when I'll get around to debugging this, probably not this week

ci/ci.sh Outdated
@@ -76,6 +76,9 @@ function run_tests {
time HYPERFINE="hyperfine -w0 -r1" ./miri bench $TARGET_FLAG
fi

## Test that `miri run` with dependencies works
./miri run $TARGET_FLAG --dep tests/pass-dep/getrandom.rs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to move this in its own PR, it shouldn't be blocked on the ui_test update.

@bors
Copy link
Contributor

bors commented Sep 13, 2024

☔ The latest upstream changes (presumably #3881) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk oli-obk force-pushed the ui_test_26_1 branch 2 times, most recently from cd1a769 to a137efb Compare September 16, 2024 10:29
@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 16, 2024

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Sep 16, 2024
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM apart from some nits.

miri-script/src/commands.rs Outdated Show resolved Hide resolved
tests/ui.rs Outdated Show resolved Hide resolved
tests/ui.rs Show resolved Hide resolved
tests/ui.rs Outdated Show resolved Hide resolved
tests/ui.rs Outdated Show resolved Hide resolved
tests/pass/shims/x86/intrinsics-sha.rs Outdated Show resolved Hide resolved
@RalfJung RalfJung removed the S-waiting-on-review Status: Waiting for a review to complete label Sep 16, 2024
@RalfJung
Copy link
Member

Looks great, thanks! r=me when CI is happy. :)

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 17, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Sep 17, 2024

📌 Commit 059dbb7 has been approved by oli-obk

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 17, 2024

⌛ Testing commit 059dbb7 with merge 22d9817...

@bors
Copy link
Contributor

bors commented Sep 17, 2024

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 22d9817 to master...

@bors bors merged commit 22d9817 into master Sep 17, 2024
8 checks passed
@bors bors deleted the ui_test_26_1 branch September 17, 2024 06:59
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 this pull request may close these issues.

5 participants