tests: Require run-fail ui tests to have an exit code (SIGABRT not ok)
#143002
+132
−79
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
rustbot has assigned @petrochenkov. Use |
rustbot
added
A-compiletest
|
Some changes occurred in src/tools/compiletest cc @jieyouxu |
Enselic
added
S-waiting-on-author
rustbot
added
the
T-infra
This comment has been minimized.
This comment has been minimized.
|
why not simply also would accept |
|
@bors2 delegate=try |
|
@Enselic can now perform try builds on this pull request |
Enselic
force-pushed
the
tests-ui-run-fail-exit-vs-signal
branch
from
6ad1973 to
17be091
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@bors2 try jobs=x86_64-msvc-1,x86_64-msvc-2 |
rust-bors bot
added a commit
that referenced
this pull request
Jun 25, 2025
…try> tests: Require `run-fail` ui tests to have an exit code (`SIGABRT` not ok) Normally a `run-fail` ui test shall not be terminated by a signal like `SIGABRT`. So begin having that as a hard requirement. Some of our current tests do terminate by a signal however. Introduce and use `run-fail-without-exit-code` for those tests. This adds further (cc #142304, #142886) protection against the regression in #123733 since that bug also manifested as `SIGABRT` in `tests/ui/panics/panic-main.rs` (shown as `Aborted (core dumped)` in the logs attached to that issue, and I have also been able to reproduce this locally). ### TODO - [ ] what about on Windows? - [ ] also update docs at https://rustc-dev-guide.rust-lang.org/tests/directives.html#controlling-outcome-expectations - [ ] clean up the code ### Zulip discussion See https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/compiletest.3A.20terminate.20by.20signal.20vs.20exit.20with.20error/with/525611235 try-job: x86_64-msvc-1 try-job: x86_64-msvc-2
This comment was marked as resolved.
This comment was marked as resolved.
Enselic
force-pushed
the
tests-ui-run-fail-exit-vs-signal
branch
from
17be091 to
ad4e082
Compare
|
An only-windows test that was run-fail now must be run-crash. Fixed now. Trying again: @bors2 try jobs=x86_64-msvc-1,x86_64-msvc-2 |
rust-bors bot
added a commit
that referenced
this pull request
Jun 25, 2025
…try> tests: Require `run-fail` ui tests to have an exit code (`SIGABRT` not ok) Normally a `run-fail` ui test shall not be terminated by a signal like `SIGABRT`. So begin having that as a hard requirement. Some of our current tests do terminate by a signal however. Introduce and use `run-fail-without-exit-code` for those tests. This adds further (cc #142304, #142886) protection against the regression in #123733 since that bug also manifested as `SIGABRT` in `tests/ui/panics/panic-main.rs` (shown as `Aborted (core dumped)` in the logs attached to that issue, and I have also been able to reproduce this locally). ### TODO - [ ] **Q:** what about on Windows? **A:** we'll treat any exit code outside of 1 - 127 as "crashed", and we'll do the same on unix. - [ ] also update docs at https://rustc-dev-guide.rust-lang.org/tests/directives.html#controlling-outcome-expectations - [ ] clean up the code - [ ] test all permutations of actual vs expected ### Zulip discussion See https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/compiletest.3A.20terminate.20by.20signal.20vs.20exit.20with.20error/with/525611235 try-job: x86_64-msvc-1 try-job: x86_64-msvc-2
This comment was marked as resolved.
This comment was marked as resolved.
Enselic
force-pushed
the
tests-ui-run-fail-exit-vs-signal
branch
from
ad4e082 to
2499dac
Compare
Enselic
force-pushed
the
tests-ui-run-fail-exit-vs-signal
branch
from
2c00e05 to
80e8db8
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
Hopefully we don't get the strange failure on Windows now after I rebased on latest master. @bors2 try jobs=x86_64-msvc-1,x86_64-msvc-2 |
rust-bors bot
added a commit
that referenced
this pull request
Jun 25, 2025
…try> tests: Require `run-fail` ui tests to have an exit code (`SIGABRT` not ok) Normally a `run-fail` ui test shall not be terminated by a signal like `SIGABRT`. So begin having that as a hard requirement. Some of our current tests do terminate by a signal however. Introduce and use `run-fail-without-exit-code` for those tests. This adds further (cc #142304, #142886) protection against the regression in #123733 since that bug also manifested as `SIGABRT` in `tests/ui/panics/panic-main.rs` (shown as `Aborted (core dumped)` in the logs attached to that issue, and I have also been able to reproduce this locally). ### TODO - [ ] **Q:** what about on Windows? **A:** we'll treat any exit code outside of 1 - 127 as "crashed", and we'll do the same on unix. - [x] test all permutations of actual vs expected **Done:** See #143002 (comment). ### Zulip discussion See https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/compiletest.3A.20terminate.20by.20signal.20vs.20exit.20with.20error/with/525611235 try-job: x86_64-msvc-1 try-job: x86_64-msvc-2
|
Now that tests pass on Windows I'd also like to make sure that failures also work on Windows by doing this on Windows. |
|
@bors2 try jobs=x86_64-msvc-1,x86_64-msvc-2 |
rust-bors bot
added a commit
that referenced
this pull request
Jun 25, 2025
…try> tests: Require `run-fail` ui tests to have an exit code (`SIGABRT` not ok) Normally a `run-fail` ui test shall not be terminated by a signal like `SIGABRT`. So begin having that as a hard requirement. Some of our current tests do terminate by a signal however. Introduce and use ~`run-fail-without-exit-code`~ `run-crash` for those tests. This adds further (cc #142304, #142886) protection against the regression in #123733 since that bug also manifested as `SIGABRT` in `tests/ui/panics/panic-main.rs` (shown as `Aborted (core dumped)` in the logs attached to that issue, and I have also been able to reproduce this locally). ### TODO - [ ] **Q:** what about on Windows? **A:** we'll treat any exit code outside of 1 - 127 as "crashed", and we'll do the same on unix. - [x] test all permutations of actual vs expected **Done:** See #143002 (comment). ### Zulip discussion See https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/compiletest.3A.20terminate.20by.20signal.20vs.20exit.20with.20error/with/525611235 try-job: x86_64-msvc-1 try-job: x86_64-msvc-2
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
Failures on Windows look good too. See below for the relevant log excerpt: Click to expand |
Enselic
force-pushed
the
tests-ui-run-fail-exit-vs-signal
branch
2 times, most recently
from
144409c to
3a52d94
Compare
This comment has been minimized.
This comment has been minimized.
…t ok) Normally a `run-fail` ui test shall not be terminated by a signal like `SIGABRT`. So begin having that as a hard requirement. Some of our current tests do terminate by a signal however. Introduce and use `run-crash` for those tests.
|
I decided to change the failures message in the to for clarity. I will now re-create these logs to be sure it still works. |
Enselic
force-pushed
the
tests-ui-run-fail-exit-vs-signal
branch
from
3a52d94 to
f7a54fe
Compare
This comment has been minimized.
This comment has been minimized.
|
Here is the updated version of #143002 (comment) using this commit. I tested what When When when when when when |
Enselic
force-pushed
the
tests-ui-run-fail-exit-vs-signal
branch
from
f7a54fe to
64c076e
Compare
|
I'm happy with the code and how it works so this is ready for review. Summary of where we stand:
@rustbot ready |
rustbot
added
S-waiting-on-review
| &proc_res, | ||
| ); | ||
| } | ||
| } else if self.should_exit_with_failure() { |
There was a problem hiding this comment.
Suggested change
| } else if self.should_exit_with_failure() { | |
| } else if self.props.fail_mode == Some(FailMode::Run(RunFailMode::ExitWithFailure)) { |
Probably don't need a separate method.
|
r=me with the cleanup from #143002 (comment). |
petrochenkov
added
S-waiting-on-author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Normally a
run-failui test shall not be terminated by a signal likeSIGABRT. So begin having that as a hard requirement.Some of our current tests do terminate by a signal however. Introduce and use
run-fail-without-exit-coderun-crashfor those tests.This adds further (cc #142304, #142886) protection against the regression in #123733 since that bug also manifested as
SIGABRTintests/ui/panics/panic-main.rs(shown asAborted (core dumped)in the logs attached to that issue, and I have also been able to reproduce this locally).TODO
A: we'll treat any exit code outside of 1 - 127 as "crashed", and we'll do the same on unix.
Done: See tests: Require
run-failui tests to have an exit code (SIGABRTnot ok) #143002 (comment).Zulip discussion
See https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/compiletest.3A.20terminate.20by.20signal.20vs.20exit.20with.20error/with/525611235