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

compiletest: Introduce // {check,build,run}-pass pass modes #61778

Merged
merged 4 commits into from
Jun 23, 2019

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Jun 12, 2019

Pass UI tests now have three modes

// check-pass
// build-pass
// run-pass

mirroring equivalent well-known cargo commands.

// check-pass will compile the test skipping codegen (which is expensive and isn't supposed to fail in most cases).
// build-pass will compile and link the test without running it.
// run-pass will compile, link and run the test.
Tests without a "pass" annotation are still considered "fail" tests.

Most UI tests would probably want to switch to check-pass.
Tests validating codegen would probably want to run the generated code as well and use run-pass.
build-pass should probably be rare (linking tests?).

#61755 will provide a way to run the tests with any mode, e.g. bump check-pass tests to run-pass to satisfy especially suspicious people, and be able to make sure that codegen doesn't breaks in some entirely unexpected way.
Tests marked with any mode are expected to pass with any other mode, if that's not the case for some legitimate reason, then the test should be made a "fail" test rather than a "pass" test.
Perhaps some secondary CI can verify this invariant, but that's not super urgent.

// compile-pass still works and is equivalent to build-pass.
Why is // compile-pass bad - 1) it gives an impression that the test is only compiled, but not linked, 2) it doesn't mirror a cargo command.
It can be removed some time in the future in a separate PR.

cc #61712

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 12, 2019
@petrochenkov
Copy link
Contributor Author

petrochenkov commented Jun 12, 2019

r? @Mark-Simulacrum @Centril
cc @rust-lang/compiler

@Centril
Copy link
Contributor

Centril commented Jun 12, 2019

Tests without a "pass" annotation are still considered "fail" tests.

I would eventually like to have mandatory // compile-fail annotations for easier grepping but that is a larger task.

@Mark-Simulacrum
Copy link
Member

It feels like in order for #61755 to work well we'd want to actually require that all tests are annotated with all three "modes" (check, build, run), since presumably you could have a test that checks successfully, builds successfully, but should fail when being run. There could be some heuristic that run-pass implies check-pass and build-pass (and build-pass implies check-pass). Otherwise we'd not be able to effectively switch all tests to --stop-at run or whatever flag we choose in #61755, as the test suite would likely fail.

Other than these concerns r=me on the actual implementation though.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Jun 12, 2019

@Centril

compile-fail annotations for easier grepping

grep -L?

It's nice to have tests without extra noise.
For example, most (non-run-pass) UI tests want #![allow(unused)] to suppress unused warnings irrelevant to the test's point, so #![allow(unused)] is moved into compiletest and enabled by default, so it doesn't have to be added to individual tests.
No annotation for compile-fail seems to be a good default too.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Jun 12, 2019

@Mark-Simulacrum

you could have a test that checks successfully, builds successfully, but should fail when being run.

Such tests will need to be moved to the run-fail test suit.
(Which could be eventually merged into ui as well.)
That's the general idea - "fails at any stage -> not a pass test".

(Tests failing with --stop-at run will need to be audited though, I'm pretty sure a number of them will fail for silly reasons not related to the test's purpose in any way.)

@Mark-Simulacrum
Copy link
Member

Ah, I thought we had already merged the run-fail suite in, okay.

@Centril
Copy link
Contributor

Centril commented Jun 12, 2019

grep -L?

It's nice to have tests without extra noise.
For example, most UI tests want #![allow(unused)] to suppress unused warnings irrelevant to the test's point, so #![allow(unused)] is moved into compiletest and enabled by default, so it doesn't have to be added to individual tests.
No annotation for compile-fail seems to be a good default too.

I suppose it's a question wrt. noise/signal. I think it's more robust wrt. change to do // compile-fail specifically so that tooling (e.g. wg-grammar stuff) doesn't need to suss out various implicit meanings of various compiletest attributes. It also makes the test suite more easy to understand for beginners.

I think fn main() {} is more annoying personally... :)

@Centril
Copy link
Contributor

Centril commented Jun 15, 2019

Would be good to land this so we can unblock --pass check... :)

@petrochenkov
Copy link
Contributor Author

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Jun 23, 2019

📌 Commit 0886bc4 has been approved by Mark-Simulacrum

@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 Jun 23, 2019
Centril added a commit to Centril/rust that referenced this pull request Jun 23, 2019
compiletest: Introduce `// {check,build,run}-pass` pass modes

Pass UI tests now have three modes
```
// check-pass
// build-pass
// run-pass
```
mirroring equivalent well-known `cargo` commands.

`// check-pass` will compile the test skipping codegen (which is expensive and isn't supposed to fail in most cases).
`// build-pass` will compile and link the test without running it.
`// run-pass` will compile, link and run the test.
Tests without a "pass" annotation are still considered "fail" tests.

Most UI tests would probably want to switch to `check-pass`.
Tests validating codegen would probably want to run the generated code as well and use `run-pass`.
`build-pass` should probably be rare (linking tests?).

rust-lang#61755 will provide a way to run the tests with any mode, e.g. bump `check-pass` tests to `run-pass` to satisfy especially suspicious people, and be able to make sure that codegen doesn't breaks in some entirely unexpected way.
Tests marked with any mode are expected to pass with any other mode, if that's not the case for some legitimate reason, then the test should be made a "fail" test rather than a "pass" test.
Perhaps some secondary CI can verify this invariant, but that's not super urgent.

`// compile-pass` still works and is equivalent to `build-pass`.
Why is `// compile-pass` bad - 1) it gives an impression that the test is only compiled, but not linked, 2) it doesn't mirror a cargo command.
It can be removed some time in the future in a separate PR.

cc rust-lang#61712
bors added a commit that referenced this pull request Jun 23, 2019
Rollup of 5 pull requests

Successful merges:

 - #61778 (compiletest: Introduce `// {check,build,run}-pass` pass modes)
 - #62037 (Speed up tidy)
 - #62052 (submodules: Update clippy from 5a11ed7 to c5d1ecd)
 - #62070 (Run rustfmt on some libsyntax files)
 - #62075 (Remove `ast::Guard`)

Failed merges:

r? @ghost
@bors
Copy link
Contributor

bors commented Jun 23, 2019

⌛ Testing commit 0886bc4 with merge 2cd5ed4...

bors added a commit that referenced this pull request Jun 23, 2019
compiletest: Introduce `// {check,build,run}-pass` pass modes

Pass UI tests now have three modes
```
// check-pass
// build-pass
// run-pass
```
mirroring equivalent well-known `cargo` commands.

`// check-pass` will compile the test skipping codegen (which is expensive and isn't supposed to fail in most cases).
`// build-pass` will compile and link the test without running it.
`// run-pass` will compile, link and run the test.
Tests without a "pass" annotation are still considered "fail" tests.

Most UI tests would probably want to switch to `check-pass`.
Tests validating codegen would probably want to run the generated code as well and use `run-pass`.
`build-pass` should probably be rare (linking tests?).

#61755 will provide a way to run the tests with any mode, e.g. bump `check-pass` tests to `run-pass` to satisfy especially suspicious people, and be able to make sure that codegen doesn't breaks in some entirely unexpected way.
Tests marked with any mode are expected to pass with any other mode, if that's not the case for some legitimate reason, then the test should be made a "fail" test rather than a "pass" test.
Perhaps some secondary CI can verify this invariant, but that's not super urgent.

`// compile-pass` still works and is equivalent to `build-pass`.
Why is `// compile-pass` bad - 1) it gives an impression that the test is only compiled, but not linked, 2) it doesn't mirror a cargo command.
It can be removed some time in the future in a separate PR.

cc #61712
@bors
Copy link
Contributor

bors commented Jun 23, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: Mark-Simulacrum
Pushing 2cd5ed4 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 23, 2019
@bors bors merged commit 0886bc4 into rust-lang:master Jun 23, 2019
Centril added a commit to Centril/rust that referenced this pull request Jun 28, 2019
…petrochenkov

Add `--pass $mode` to compiletest through `./x.py`

Adds a flag `--pass $mode` to compiletest, which is exposed through `./x.py`.

When `--pass $mode` is passed, `{check,build,compile,run}-pass` tests will be forced to run under the given `$mode` unless the directive `// ignore-pass` exists in the test file.

The modes are explained in rust-lang#61778:
- `check` has the same effect as `cargo check`
- `build` or `compile` have the same effect as `cargo build`
- `run` has the same effect as `cargo run`

On my machine, `./x.py -i test src/test/run-pass --stage 1 --pass check` takes 38 seconds whereas it takes 2 min 7 seconds without `--pass check`.

cc rust-lang#61712

r? @petrochenkov
Centril added a commit to Centril/rust that referenced this pull request Jun 28, 2019
…petrochenkov

Add `--pass $mode` to compiletest through `./x.py`

Adds a flag `--pass $mode` to compiletest, which is exposed through `./x.py`.

When `--pass $mode` is passed, `{check,build,compile,run}-pass` tests will be forced to run under the given `$mode` unless the directive `// ignore-pass` exists in the test file.

The modes are explained in rust-lang#61778:
- `check` has the same effect as `cargo check`
- `build` or `compile` have the same effect as `cargo build`
- `run` has the same effect as `cargo run`

On my machine, `./x.py -i test src/test/run-pass --stage 1 --pass check` takes 38 seconds whereas it takes 2 min 7 seconds without `--pass check`.

cc rust-lang#61712

r? @petrochenkov
Centril added a commit to Centril/rust that referenced this pull request Jun 28, 2019
…petrochenkov

Add `--pass $mode` to compiletest through `./x.py`

Adds a flag `--pass $mode` to compiletest, which is exposed through `./x.py`.

When `--pass $mode` is passed, `{check,build,compile,run}-pass` tests will be forced to run under the given `$mode` unless the directive `// ignore-pass` exists in the test file.

The modes are explained in rust-lang#61778:
- `check` has the same effect as `cargo check`
- `build` or `compile` have the same effect as `cargo build`
- `run` has the same effect as `cargo run`

On my machine, `./x.py -i test src/test/run-pass --stage 1 --pass check` takes 38 seconds whereas it takes 2 min 7 seconds without `--pass check`.

cc rust-lang#61712

r? @petrochenkov
Centril added a commit to Centril/rust that referenced this pull request Jun 29, 2019
…petrochenkov

Add `--pass $mode` to compiletest through `./x.py`

Adds a flag `--pass $mode` to compiletest, which is exposed through `./x.py`.

When `--pass $mode` is passed, `{check,build,compile,run}-pass` tests will be forced to run under the given `$mode` unless the directive `// ignore-pass` exists in the test file.

The modes are explained in rust-lang#61778:
- `check` has the same effect as `cargo check`
- `build` or `compile` have the same effect as `cargo build`
- `run` has the same effect as `cargo run`

On my machine, `./x.py -i test src/test/run-pass --stage 1 --pass check` takes 38 seconds whereas it takes 2 min 7 seconds without `--pass check`.

cc rust-lang#61712

r? @petrochenkov
@RalfJung
Copy link
Member

// compile-pass still works and is equivalent to build-pass.
Why is // compile-pass bad - 1) it gives an impression that the test is only compiled, but not linked, 2) it doesn't mirror a cargo command.
It can be removed some time in the future in a separate PR.

Is there an issue tracking deprecation/removal of compile-pass?

@Centril
Copy link
Contributor

Centril commented Jun 29, 2019

Is there an issue tracking deprecation/removal of compile-pass?

Not to my knowledge but it should be a rather trivial find/replace with // compile-pass to // build-pass.

@cramertj
Copy link
Member

cramertj commented Jul 1, 2019

@Centril I'd figure most examples would want to be // compile-pass -> // check-pass, not // build-pass.

@Centril
Copy link
Contributor

Centril commented Jul 1, 2019

@cramertj Yes, but it's a risky move to blanket replace // compile-pass with // check-pass since that does something different and may reduce test coverage.

@cramertj
Copy link
Member

cramertj commented Jul 1, 2019

yeah, just pointing out that we might not want to blanket search-and-replace to // build-pass, since we'd no longer have a way to tell which things were intentionally build-pass or check-pass vs. things that just hadn't been moved over yet. We could solve that by adding a note of some kind in the output, e.g. // build-pass (unreviewed, possibly check-pass)

@Centril
Copy link
Contributor

Centril commented Jul 1, 2019

We could solve that by adding a note of some kind in the output, e.g. // build-pass (unreviewed, possibly check-pass)

@cramertj This seems fine; want to file a C-cleanup issue?

@cramertj
Copy link
Member

cramertj commented Jul 1, 2019

Opened #62277.

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.

8 participants