Introduce 'run-pass' header to 'ui' tests in compiletest. Fix issue #36516.

[kennytm]

ui-run test is a combination of ui test and run-pass test. It is used to test lint output.

Added support of // run-pass header to ui tests.

The compiler message of each test must match the corresponding *.stderr file like the traditional ui tests. Additionally, the compiled output must be executed successfully like the run-pass test.

12 run-pass/run-pass-fulldeps tests are moved to ui/ui-fulldeps plus the headers. After this move, no run-pass/run-pass-fulldeps tests should rely on the compiler's JSON message. This allows us to stop passing --error-format json in run-pass tests, thus fixing #36516.

[rust-highfive]

r? @nikomatsakis

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

[petrochenkov]

BTW, I have a branch in progress unifying ui and compile-fail tests, so I'd like to bikeshed naming and the larger picture a bit.
I wanted to leave one test group instead of these two groups doing both UI checking and //~ ERROR checking, and to name it just compile. Tests in this group are not required to fail compilation, so it's not "compile-fail" but they are also not run, so there's no "run". There's also no "ui" because all these tests are UI.
There are also compile-fulldeps, because fulldeps is a completely orthogonal issue. parse-fail tests are renamed to parse, and not required to fail, and made UI tests.

I always wondered why run-pass and run-fail are two different test groups. Can't the difference be served by a test attribute (// should-fail or something)? Is it ever useful to run only one of these test groups but not another?
Maybe they can be merged into one group named just run (as opposed to compile (full compilation, no run) and parse (partial compilation, no run)), plus of course run-fulldeps?
Does it make sense to perform UI checking for all run tests? Then "ui" prefix/suffix can be removed from run tests as well because all these tests are UI.

(I'm not touching specialized tests like rustdoc or debuginfo here.)

[Mark-Simulacrum]

Yeah, it seems to me like UI checking is "good" but also probably adds a small overhead to each test, which may not be a good idea -- our cycle times are already long.

[Mark-Simulacrum]

I don't think this should be removed after this is merged. People in the future may still add run-pass tests, and if we want the separation, then we should enforce it.

[kennytm]

@Mark-Simulacrum Well this check does add a tiny overhead to the test 😄 (running load_errors will scan the whole file again for //~).

(This can be moved to tidy but it will spend the same amount of Travis time)

[Mark-Simulacrum]

Presumably we already load the errors for the test somewhere else, since we need them -- could we assert at that location?

[kennytm]

@Mark-Simulacrum The test runner loads the header information (// ignore-stage0 stuff) twice (EarlyProps and TestProps), but both do not look for the //~ errors. Or did I misunderstand what you mean?

[Mark-Simulacrum]

I thought that we probably loaded //~ errors at some point to verify that they're correct; and we could then assert there if the test has none.

[kennytm]

@Mark-Simulacrum The //~ are loaded in errors::load_errors, and checked against the output in check_expected_errors. So they were verified at this line exactly.

[Mark-Simulacrum]

Oh, yeah, I see what you mean. So I guess this is a little extra work, but hopefully not much. I think I'm more or less fine with removing it then.

[nikomatsakis]

cc @cengizio -- who was also looking into this general area :)

[nikomatsakis]

I like the idea of unifying run-pass, run-fail, and compile-fail into one category. I would prefer to be able to group tests by what they are testing, basically, not whether they are expected to generate a compilation error or not.

[alexcrichton]

@nikomatsakis to clarify, just for tagging purposes, is this waiting on a decision to happen elsewhere?

[cengiz-io]

Hello,

I'm (slowly) adding inline ERROR assertions to ui tests. Not sure if this intersects with it.

[kennytm]

@cengizio there should be no conflict with the scope of this PR itself, which just affects the original run-pass tests to get rid of the JSON output. The rest of the discussion (unifying test folders) seems to be the same direction as what outlined in the internals thread, I hope the actual unification, when a plan is decided, will be done in a different PR a later :).

[carols10cents]

ping @nikomatsakis for an answer to @alexcrichton's question :)

[nikomatsakis]

@alexcrichton@carols10cents

to clarify, just for tagging purposes, is this waiting on a decision to happen elsewhere?

It's not really clear. I started this internals thread and, as of yet, there's not been any opposition. We normally land changes to the test infrastructure kind of willy-nilly without a lot of fuss, but merging compile-fail and ui (which is part of what I would like to do) feels like a fairly fundamental change that I would like @rust-lang/compiler and others to know about and be on board with.

That said, this particular PR does something much more modest -- it addsui-run. I don't think that we need any particular sign-off for just this change by the team at large.

[nikomatsakis]

Now, that said, I think I'd prefer not to have a new direction (ui-run) and instead to extend the existing ui test concept to support tests that do not expect to have errors. @kennytm, what do you think about making it possible to add a // run-pass header into ui tests which causes them to not expect compilation failure (and to execute when done)?

(I think I would also like to support a "reference file" showing the output onto stdout/stderr, which might be good for run-fail tests, and also for checking that the execution behavior has not changed, but we could do that later.)

Finally, I think I agree that we should scan for //~ WARNING and prohibit it from run-pass tests. Eventually, I'd actually like to be checking that //~ WARNING and //~ ERROR are respected in ui tests, but this is what @cengizio (and maybe @petrochenkov) were doing, from what I can tell?

[kennytm]

@nikomatsakis

what do you think about making it possible to add a // run-pass header into ui tests which causes them to not expect compilation failure (and to execute when done)?

Sounds good to me

[kennytm]

@nikomatsakis Changed to use // run-pass headers.

[petrochenkov]

@nikomatsakis

add a // run-pass header into ui tests which causes them to not expect compilation failure (and to execute when done)?

I'd really like to avoid this for practical reasons.
run-{pass,fail} tests requires more time to run and it's important to use them only when some runtime behavior actually needs to be tested, not for "compile-pass" style testing of compiler's "frontend"/"middle-end" a-la "verify that something type-checks" (everything except for trans, basically).
This is especially important given that number of tests is constantly growing and it's becoming harder to run the full test suite.
This is exactly why "parse-fail" were moved into a separate category, that's not even type-checked and therefore runs super-quick.
When I work on "frontend"/"middle-end" I want to have a well-defined group of tests, that can be run as quickly as possible, not spending maybe +100% of time on running produced executables.
I don't want run tests to accidentally get into that group, and I think this PR should be rolled back to the previous version.

[nikomatsakis]

@petrochenkov

I'd really like to avoid this for practical reasons.

I don't understand. It sounds to me like you are arguing in favor of a // compile-pass header but not against// run-pass -- that is, sometimes we would want to test run-pass, and sometimes we don't feel the need. Right?

[nikomatsakis]

Oh, I see, you're saying you'd prefer to have them separated into their own directory? If so, I disagree. The problem is that I don't like having the tests that target a common area of the code strewn about in various directories; I'd like to see them all in one place, grouped by what they test, not by their "mode". (i.e., I would like all borrowck tests together).

What about if we compromised, by adding a flag to compile test to let it configure which "modes" it will execute? My personal preference would be to have tags in tests, so that we can have e.g. a "smoketest" tag, and then be able to say "run just the smoketests". This may include some run-pass tests but also some compile-pass and compile-fail tests. Things like run-pass or compile-fail could be automatic tags based on execution mode.

[petrochenkov]

@nikomatsakis

What about if we compromised, by adding a flag to compile test to let it configure which "modes" it will execute?

Yes, it's not necessary for "fast" and "slow" tests to be in different directories literally, the important thing is ability to run the "fast" group only somehow, maybe through compiletest flags and annotations in test files.

[kennytm]

Test failed due to comments like these...

// This test used to be part of a run-pass test, but revised outlives
// rule means that it no longer compiles.

Update: Added 15edf4f to require directives to be the first complete word of the line.

[nikomatsakis]

@petrochenkov

Yes, it's not necessary for "fast" and "slow" tests to be in different directories literally, the important thing is ability to run the "fast" group only somehow, maybe through compiletest flags and annotations in test files.

Great! I agree that this is a useful thing. What do you think of the "tags" idea as a way of specifying what "sets" we want to run? I feel though that this should be a distinct PR. Maybe we should open an issue to write up a "spec"?

I imagine this:

// tags foo bar baz

and adding a flag like --tags 'foo,bar' meaning "run things with foo and bar". Probably we could go overboard and support things like --tags 'foo;bar' (foo or bar) as well, or even crazier by adding some parentheses. =)

This flag also needs to work from ./x.py, so that you can do ./x.py test --tags foo,bar --stage 1 or whatever.

[nikomatsakis]

I'm inclined to r+ this specific PR, then, and try to address @petrochenkov's concern in a follow-up via some kind of tags scheme. Everybody OK with that?

[petrochenkov]

OK

[nikomatsakis]

@kennytm

The number of bytes here (32 vs 16) is target dependent, the error message is correct on x86_64. I guess this is due to 32- vs 64-bit, so this specific test is ignored on 32-bit platforms (x86 and arm).

Sigh. This seems like it would be a good use-case for either having multiple references, or adding the ability to add "wildcards" into the reference output. I'd hoped to avoid the latter so that references can still be updated automatically though.

I guess configuring the test for specific platforms suffices though -- at worst, if we really wanted to test multiple platforms, we could have two copies of the test.

[nikomatsakis]

@bors r+

[bors]

📌 Commit 92d1f4f has been approved by nikomatsakis

[bors]

⌛ Testing commit 92d1f4f with merge 65251ec...

[bors]

💔 Test failed - status-travis

[kennytm]

looks like emscripten is also 32-bit.

[kennytm]

@nikomatsakis Ignored emscripten as well in 2f68cfd (HEAD = e703fa7).

[Mark-Simulacrum]

Looks like a UI test needs updating.

[01:15:32] failures:
[01:15:32] [01:15:32] ---- [ui] ui/enum-size-variance.rs stdout ----
[01:15:32] ui: /checkout/src/test/ui/enum-size-variance.rs
[01:15:32] normalized stderr:
[01:15:32] warning: enum variant is more than three times larger (32 bytes) than the next largest
[01:15:32] --> $DIR/enum-size-variance.rs:32:5
[01:15:32] |
[01:15:32] 32 | L(isize, isize, isize, isize), //~ WARNING three times larger
[01:15:32] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[01:15:32] |
[01:15:32] note: lint level defined here
[01:15:32] --> $DIR/enum-size-variance.rs:17:9
[01:15:32] |
[01:15:32] 17 | #![warn(variant_size_differences)]
[01:15:32] | ^^^^^^^^^^^^^^^^^^^^^^^^
[01:15:32] [01:15:32] [01:15:32] [01:15:32] expected stderr:
[01:15:32] warning: enum variant is more than three times larger (32 bytes) than the next largest
[01:15:32] --> $DIR/enum-size-variance.rs:31:5
[01:15:32] |
[01:15:32] 31 | L(isize, isize, isize, isize), //~ WARNING three times larger
[01:15:32] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[01:15:32] |
[01:15:32] note: lint level defined here
[01:15:32] --> $DIR/enum-size-variance.rs:16:9
[01:15:32] |
[01:15:32] 16 | #![warn(variant_size_differences)]
[01:15:32] | ^^^^^^^^^^^^^^^^^^^^^^^^
[01:15:32] [01:15:32] [01:15:32] [01:15:32] diff of stderr:
[01:15:32] [01:15:32] warning: enum variant is more than three times larger (32 bytes) than the next largest
[01:15:32] - --> $DIR/enum-size-variance.rs:31:5
[01:15:32] + --> $DIR/enum-size-variance.rs:32:5
[01:15:32] |
[01:15:32] -31 | L(isize, isize, isize, isize), //~ WARNING three times larger
[01:15:32] +32 | L(isize, isize, isize, isize), //~ WARNING three times larger
[01:15:32] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[01:15:32] |
[01:15:32] note: lint level defined here
[01:15:32] - --> $DIR/enum-size-variance.rs:16:9
[01:15:32] + --> $DIR/enum-size-variance.rs:17:9
[01:15:32] |
[01:15:32] -16 | #![warn(variant_size_differences)]
[01:15:32] +17 | #![warn(variant_size_differences)]
[01:15:32] | ^^^^^^^^^^^^^^^^^^^^^^^^
[01:15:32] [01:15:32] [01:15:32] The actual stderr differed from the expected stderr.
[01:15:32] Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/enum-size-variance.stderr
[01:15:32] To update references, run this command from build directory:
[01:15:32] /checkout/src/test/ui/update-references.sh '/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui' 'enum-size-variance.rs'
[01:15:32] [01:15:32] error: 1 errors occurred comparing output.
[01:15:32] status: exit code: 0
[01:15:32] command: /checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc /checkout/src/test/ui/enum-size-variance.rs -L /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui --target=x86_64-unknown-linux-gnu -L /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/enum-size-variance.stage2-x86_64-unknown-linux-gnu.ui.libaux -C prefer-dynamic -o /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/enum-size-variance.stage2-x86_64-unknown-linux-gnu -Crpath -O -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers
[01:15:32] stdout:
[01:15:32] ------------------------------------------
[01:15:32] [01:15:32] ------------------------------------------
[01:15:32] stderr:
[01:15:32] ------------------------------------------
[01:15:32] warning: enum variant is more than three times larger (32 bytes) than the next largest
[01:15:32] --> /checkout/src/test/ui/enum-size-variance.rs:32:5
[01:15:32] |
[01:15:32] 32 | L(isize, isize, isize, isize), //~ WARNING three times larger
[01:15:32] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[01:15:32] |
[01:15:32] note: lint level defined here
[01:15:32] --> /checkout/src/test/ui/enum-size-variance.rs:17:9
[01:15:32] |
[01:15:32] 17 | #![warn(variant_size_differences)]
[01:15:32] | ^^^^^^^^^^^^^^^^^^^^^^^^
[01:15:32] [01:15:32] [01:15:32] ------------------------------------------
[01:15:32] [01:15:32] thread '[ui] ui/enum-size-variance.rs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:2480
[01:15:32] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:15:32] [01:15:32] [01:15:32] failures:

[kennytm]

@Mark-Simulacrum Thanks. Gah. Fixed in 3e6c83d.

[Mark-Simulacrum]

@bors r=nikomatsakis

[bors]

📌 Commit 3e6c83d has been approved by nikomatsakis

[bors]

⌛ Testing commit 3e6c83d with merge 1b5a923...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 1b5a923 to master...

[Eh2406]

There is a FIXME related to this issue,
https://github.com/rust-lang/rust/blob/master/src/tools/compiletest/src/runtest.rs#L229
Now that the issue is closed can the FIXME be fix, or made more specific?