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

kennytm · 2017-05-13T10:17:21Z

~~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 · 2017-05-13T10:17:32Z

r? @nikomatsakis

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

petrochenkov · 2017-05-13T10:54:50Z

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 · 2017-05-13T12:40:43Z

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 · 2017-05-14T14:13:59Z

src/tools/compiletest/src/runtest.rs

-            self.check_expected_errors(expected_errors, &proc_res);
-        }
+        assert!(expected_errors.is_empty(),
+                "run-pass tests with expected warnings should be moved to ui-run/");


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.

@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)

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

@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?

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.

@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.

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 · 2017-05-15T18:03:18Z

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

nikomatsakis · 2017-05-15T18:11:21Z

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 · 2017-05-18T20:24:53Z

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

cengiz-io · 2017-05-18T20:57:35Z

Hello,

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

kennytm · 2017-05-18T22:48:57Z

@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 · 2017-05-22T17:00:23Z

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

nikomatsakis · 2017-05-22T17:59:01Z

@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 adds ui-run. I don't think that we need any particular sign-off for just this change by the team at large.

nikomatsakis · 2017-05-22T18:03:22Z

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 · 2017-05-22T18:20:26Z

@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 · 2017-05-23T13:42:45Z

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

petrochenkov · 2017-05-23T14:07:19Z

@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 · 2017-05-23T22:06:12Z

@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 · 2017-05-23T22:09:23Z

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 · 2017-05-24T06:43:27Z

@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 · 2017-05-24T06:52:55Z

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 · 2017-05-24T14:46:28Z

@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 · 2017-05-30T21:09:27Z

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 · 2017-05-30T22:48:14Z

OK

nikomatsakis · 2017-06-01T19:33:04Z

@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 · 2017-06-01T19:33:39Z

@bors r+

bors · 2017-06-01T19:33:39Z

📌 Commit 92d1f4f has been approved by nikomatsakis

bors · 2017-06-02T10:17:40Z

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

bors · 2017-06-02T12:14:11Z

💔 Test failed - status-travis

kennytm · 2017-06-02T12:19:44Z

looks like emscripten is also 32-bit.

kennytm · 2017-06-02T12:25:39Z

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

Mark-Simulacrum · 2017-06-02T14:13:58Z

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:

…ust-lang#36516. The 'run-pass' header cause a 'ui' test to execute the result. It is used to test the lint output, at the same time ensure those lints won't cause the source code to become compile-fail. 12 run-pass/run-pass-fulldeps tests gained the header and are moved to ui/ui-fulldeps. 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 rust-lang#36516.

…ent. Refactored some related code to take advantage of this change.

kennytm · 2017-06-02T15:29:37Z

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

Mark-Simulacrum · 2017-06-04T01:34:58Z

@bors r=nikomatsakis

bors · 2017-06-04T01:34:59Z

📌 Commit 3e6c83d has been approved by nikomatsakis

bors · 2017-06-04T03:23:45Z

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

… r=nikomatsakis Introduce 'run-pass' header to 'ui' tests in compiletest. Fix issue #36516. <del>`ui-run` test is a combination of `ui` test and `run-pass` test. It is used to test lint output.</del> 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.

bors · 2017-06-04T05:44:34Z

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

@nikomatsakis

… r=nikomatsakis compilertest (UI test): Support custom normalization. Closes #42434. Adds this header for UI tests: ```rust // normalize-stderr-32bit: "fn() (32 bits)" -> "fn() ($PTR bits)" ``` It will normalize the `stderr` output on 32-bit platforms, by replacing all instances of `fn() (32 bits)` by `fn() ($PTR bits)`. Extends the UI tests in #42304 and #41968 to 32-bit targets. r? @nikomatsakis

@nikomatsakis

… r=nikomatsakis compilertest (UI test): Support custom normalization. Closes #42434. Adds this header for UI tests: ```rust // normalize-stderr-32bit: "fn() (32 bits)" -> "fn() ($PTR bits)" ``` It will normalize the `stderr` output on 32-bit platforms, by replacing all instances of `fn() (32 bits)` by `fn() ($PTR bits)`. Extends the UI tests in #42304 and #41968 to 32-bit targets. r? @nikomatsakis

Eh2406 · 2017-09-03T20:29:24Z

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?

rust-highfive assigned nikomatsakis May 13, 2017

kennytm force-pushed the fix-unreadable-json-test-output-36516 branch from 04728f8 to f8b85c8 Compare May 13, 2017 12:22

Mark-Simulacrum reviewed May 14, 2017

View reviewed changes

Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 14, 2017

kennytm force-pushed the fix-unreadable-json-test-output-36516 branch from f8b85c8 to fa62a0c Compare May 23, 2017 13:38

kennytm changed the title ~~Introduce 'ui-run' test to compiletest. Fix issue #36516.~~ Introduce 'run-pass' header to 'ui' tests in compiletest. Fix issue #36516. May 23, 2017

kennytm force-pushed the fix-unreadable-json-test-output-36516 branch from 6f4068d to 15edf4f Compare May 24, 2017 09:28

Mark-Simulacrum 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 May 28, 2017

kennytm force-pushed the fix-unreadable-json-test-output-36516 branch from 462fe26 to 92d1f4f Compare June 1, 2017 11:30

kennytm mentioned this pull request Jun 2, 2017

compiletest failures are close to unreadable because of JSON #36516

Closed

kennytm force-pushed the fix-unreadable-json-test-output-36516 branch from 92d1f4f to e703fa7 Compare June 2, 2017 12:24

kennytm added 2 commits June 2, 2017 23:28

compiletest: Force directive to be first complete word in header comm…

3e6c83d

…ent. Refactored some related code to take advantage of this change.

kennytm force-pushed the fix-unreadable-json-test-output-36516 branch from e703fa7 to 3e6c83d Compare June 2, 2017 15:29

bors merged commit 3e6c83d into rust-lang:master Jun 4, 2017

kennytm deleted the fix-unreadable-json-test-output-36516 branch June 7, 2017 04:31

kennytm mentioned this pull request Jul 6, 2017

compilertest (UI test): Support custom normalization. #43083

Merged

oli-obk mentioned this pull request Sep 6, 2017

Meta issue for FIXMEs that reference closed issues #44366

Open

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

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

Conversation

kennytm commented May 13, 2017 • edited Loading

rust-highfive commented May 13, 2017

petrochenkov commented May 13, 2017

Mark-Simulacrum commented May 13, 2017

Mark-Simulacrum May 14, 2017

Choose a reason for hiding this comment

kennytm May 14, 2017

Choose a reason for hiding this comment

Mark-Simulacrum May 14, 2017

Choose a reason for hiding this comment

kennytm May 14, 2017

Choose a reason for hiding this comment

Mark-Simulacrum May 14, 2017

Choose a reason for hiding this comment

kennytm May 14, 2017

Choose a reason for hiding this comment

Mark-Simulacrum May 14, 2017

Choose a reason for hiding this comment

nikomatsakis commented May 15, 2017

nikomatsakis commented May 15, 2017

alexcrichton commented May 18, 2017

cengiz-io commented May 18, 2017

kennytm commented May 18, 2017

carols10cents commented May 22, 2017

nikomatsakis commented May 22, 2017

nikomatsakis commented May 22, 2017

kennytm commented May 22, 2017

kennytm commented May 23, 2017

petrochenkov commented May 23, 2017

nikomatsakis commented May 23, 2017

nikomatsakis commented May 23, 2017

petrochenkov commented May 24, 2017

kennytm commented May 24, 2017 • edited Loading

nikomatsakis commented May 24, 2017

nikomatsakis commented May 30, 2017

petrochenkov commented May 30, 2017

nikomatsakis commented Jun 1, 2017

nikomatsakis commented Jun 1, 2017

bors commented Jun 1, 2017

bors commented Jun 2, 2017

bors commented Jun 2, 2017

kennytm commented Jun 2, 2017

kennytm commented Jun 2, 2017 • edited Loading

Mark-Simulacrum commented Jun 2, 2017

kennytm commented Jun 2, 2017

Mark-Simulacrum commented Jun 4, 2017

bors commented Jun 4, 2017

bors commented Jun 4, 2017

bors commented Jun 4, 2017

Eh2406 commented Sep 3, 2017

kennytm commented May 13, 2017 •

edited

Loading

kennytm commented May 24, 2017 •

edited

Loading

kennytm commented Jun 2, 2017 •

edited

Loading