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

Revamp // run-fail wrt. --pass & support // build-fail & // check-fail #66932

Merged
merged 5 commits into from
Dec 22, 2019

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Dec 1, 2019

Revamp how // run-fail tests work internally by having a separate FailMode that does not interfere with PassMode. In particular, --pass check will now have no effect on // *-fail tests. Moreover, new test annotations // check-fail (the default) and // build-fail are added. The latter is useful to distinguish post-monomorphization failures from pre-monomorphization failures as seen throughout the PR. Finally, ensure that non-Ui tests do not listen to --pass check such that the flag can be used with e.g. ./x.py test --pass check directly.

Fixes #66929.
Fixes #67128.

r? @petrochenkov
cc @RalfJung @ninjasource

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 1, 2019
@Centril

This comment has been minimized.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 1, 2019
@ninjasource
Copy link

@Centril and @petrochenkov, there is a preexisting (and passing) run-fail unit test in ui called src/test/ui/test-panic-abort.rs which has the following header comments:

// compile-flags: --test -Cpanic=abort -Zpanic_abort_tests
// run-flags: --test-threads=1
// run-fail
// check-run-results
// exec-env:RUST_BACKTRACE=0

// ignore-wasm no panic or subprocess support
// ignore-emscripten no panic or subprocess support

It may be worthwhile adding the relevant flags above to the offending async run-fail tests. However, I'm not sure if this test gets any special treatment and I cannot reproduce the failures locally.

@Centril Centril force-pushed the pass-check-runfail branch 2 times, most recently from 5660aef to 68cce86 Compare December 13, 2019 23:08
@Centril Centril changed the title make --pass check work with // run-fail tests Revamp // run-fail wrt. --pass & support // build-fail & // check-fail Dec 14, 2019
@Centril Centril added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 14, 2019
@Centril
Copy link
Contributor Author

Centril commented Dec 14, 2019

@petrochenkov I've rewritten the approach from scratch to something more useful and principled. :)

@rust-highfive

This comment has been minimized.

src/tools/compiletest/src/header.rs Outdated Show resolved Hide resolved
src/tools/compiletest/src/runtest.rs Outdated Show resolved Hide resolved
src/tools/compiletest/src/runtest.rs Show resolved Hide resolved
@@ -1,3 +1,4 @@
// build-fail
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how this is checked.
Looks like if the test starts failing at the "check" phase it will still pass with the // build-fail annotation.

Why are such annotations useful then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, how the // build-fail test set was collected?
This can be done by introducing some kind of analogue of --pass check, but for failing tests, but I don't see it in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like if the test starts failing at the "check" phase it will still pass with the // build-fail annotation.

Ouch; Didn't think of that. To fix it, I think we'd need to compile the test twice, once as check-pass and once as build-fail.

Also, how the // build-fail test set was collected?

A set of tests started failing because they did not fail with --emit metadata.

This can be done by introducing some kind of analogue of --pass check, but for failing tests, but I don't see it in this PR.

Didn't feel necessary, but I can add --fail if you think it's useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

but I can add --fail if you think it's useful.

No, no, just running twice would probably be preferable, the number of such tests is not too large, but we need to measure.
This is material for another PR anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I fixed it, but I had to refactor things to get rid of some implicit state to make it work. I think things should be less brittle & more understandable now. Let's see what the CI builder says. :)

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 14, 2019
@Centril Centril added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 14, 2019
@Centril
Copy link
Contributor Author

Centril commented Dec 14, 2019

PR builder is happy at last. 🎉

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 14, 2019

📌 Commit fdedf4c has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 14, 2019
@bors

This comment has been minimized.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 15, 2019
@bors

This comment has been minimized.

@bors

This comment has been minimized.

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 16, 2019
@Centril

This comment has been minimized.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 16, 2019
@Centril
Copy link
Contributor Author

Centril commented Dec 20, 2019

@bors p=199

@bors

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@bors

This comment has been minimized.

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 21, 2019
@Centril
Copy link
Contributor Author

Centril commented Dec 21, 2019

Fixed the 32-bit-only tests wrt. build-fail.

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Dec 21, 2019

📌 Commit 73e6a21 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 21, 2019
@bors
Copy link
Contributor

bors commented Dec 22, 2019

⌛ Testing commit 73e6a21 with merge 6e9172f...

bors added a commit that referenced this pull request Dec 22, 2019
Revamp `// run-fail` wrt. `--pass` & support `// build-fail` & `// check-fail`

Revamp how `// run-fail` tests work internally by having a separate `FailMode` that does not interfere with `PassMode`. In particular, `--pass check` will now have no effect on `// *-fail` tests. Moreover, new test annotations `// check-fail` (the default) and `// build-fail` are added. The latter is useful to distinguish post-monomorphization failures from pre-monomorphization failures as seen throughout the PR. Finally, ensure that non-`Ui` tests do not listen to `--pass check` such that the flag can be used with e.g. `./x.py test --pass check` directly.

Fixes #66929.
Fixes #67128.

r? @petrochenkov
cc @RalfJung @ninjasource
@bors
Copy link
Contributor

bors commented Dec 22, 2019

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing 6e9172f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 22, 2019
@bors bors merged commit 73e6a21 into master Dec 22, 2019
@Centril Centril deleted the pass-check-runfail branch December 22, 2019 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustdoc-ui tests fail when x.py is run with --pass check ./x.py test --pass check fails
7 participants