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

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

Merged
merged 8 commits into from
Jun 29, 2019

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Jun 11, 2019

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 #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 #61712

r? @petrochenkov

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 11, 2019
src/bootstrap/flags.rs Outdated Show resolved Hide resolved
src/bootstrap/flags.rs Outdated Show resolved Hide resolved
src/test/ui/print_type_sizes/packed.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

petrochenkov commented Jun 12, 2019

It's an open question what exactly "no codegen" option should be used for // check-pass and --force check.
In case of --emit metadata I'm not sure whether

  • metadata files hit the "file name is too long" cases (Skip codegen for one UI test with long file path #60648) when emitted.
    UPDATE: Turns out // skip-codegen already used --emit metadata, so it doesn't have issues with long file paths.
  • we probably don't really need the metadata files emitted for the check mode (unlike cargo), but on the other hand it would be nice to specify check/build/run as doing exactly the same thing as cargo.
    Need to measure how expensive the metadata emission is.

@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 Jun 12, 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 Jun 12, 2019
@Centril Centril changed the title Add --force check to compiletest through ./x.py Add --pass check to compiletest through ./x.py Jun 12, 2019
@Centril
Copy link
Contributor Author

Centril commented Jun 12, 2019

@petrochenkov Fixed the comments :)

@rust-highfive

This comment has been minimized.

@petrochenkov
Copy link
Contributor

This should probably wait for #61778.
Otherwise I'll have to revert the compiletest part of this PR to rebase #61778.
(Or let's squash and land, then reverting will be easy.)

@Centril Centril added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 12, 2019
@Centril
Copy link
Contributor Author

Centril commented Jun 12, 2019

OK Let's wait for #61778 to land first.

@Centril Centril force-pushed the compiletest-force-check branch from 22c4a9e to 57b6a20 Compare June 12, 2019 20:56
@Centril
Copy link
Contributor Author

Centril commented Jun 12, 2019

Squashed the commits a bit.

@bors

This comment has been minimized.

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
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
@Centril Centril added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jun 24, 2019
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

This does not, however, give me the option to "filter" by mode, right? Say I wanted to only run non-pass tests, or only run *-pass tests?

@Centril
Copy link
Contributor Author

Centril commented Jun 29, 2019

@RalfJung That would be #61719.

@petrochenkov
Copy link
Contributor

@RalfJung
Filtering was my first idea too, but after some thinking about testing scenarios (why I run tests and what I want from it) --pass check appeared to be a better approach.
With pass modes available I personally expect to never use filtering.

@Centril
Copy link
Contributor Author

Centril commented Jun 29, 2019

I do agree --pass check is what you want for regular development; but from a language team perspective I find the ability to do e.g. --filter-mode pass --test-args async to be useful if I want to extract all the relevant pass tests for async await (that assumes good organization for the async/await test, which is probably a great assumption but it's serviceable as a blunt instrument). That's not something I'd do every day, but it's still useful on occasion.

bors added a commit that referenced this pull request Jun 29, 2019
Rollup of 7 pull requests

Successful merges:

 - #61199 (Revert "Set test flag when rustdoc is running with --test option" )
 - #61755 (Add `--pass $mode` to compiletest through `./x.py`)
 - #61818 (Issue #60709 test)
 - #62023 (publish_toolstate: don't use 'new' from inside the loop)
 - #62104 (Inform the query system about properties of queries at compile time)
 - #62163 (Avoid mem::uninitialized() in std::sys::unix)
 - #62204 (doc(libcore) Fix CS)

Failed merges:

r? @ghost
@bors bors merged commit 93077f3 into rust-lang:master Jun 29, 2019
@Centril Centril deleted the compiletest-force-check branch June 29, 2019 15:32
bors added a commit that referenced this pull request Jul 21, 2019
tests: Require run-pass tests without annotations to run successfully again

Fixes #62775 (regression from #61755)
bors added a commit that referenced this pull request Jul 21, 2019
tests: Require run-pass tests without annotations to run successfully again

Fixes #62775 (regression from #61755)
bors added a commit that referenced this pull request Jul 21, 2019
tests: Require run-pass tests without annotations to run successfully again

Fixes #62775 (regression from #61755)
bors added a commit that referenced this pull request Jul 22, 2019
tests: Require run-pass tests without annotations to run successfully again

Fixes #62775 (regression from #61755)
bors added a commit that referenced this pull request Jul 27, 2019
Move run-pass tests to ui

This is the second attempt at doing #53994 (which was previously reverted in #54530).

The issue with inability to run the test suite in a faster way (#54047) that motivated the revert was recently addressed by #61755.

r? @Centril
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

7 participants