Fix revision support for UI tests. #49812

ehuss · 2018-04-09T18:48:42Z

ehuss · 2018-04-09T18:51:29Z

I tried to make the code a little more explicit and cleaner to avoid confusion, though I'm not sure I succeeded. :) Everything should work the same as before, except UI tests can now output TESTNAME.REVISION.stderr/stdout files.

In order to generate the output for a test with multiple revisions, you have to run compiletest separately for each revision because it stops immediately when a revision fails. There doesn't seem to be the equivalent of --no-fail-fast for compiletest. Thoughts?

nikomatsakis · 2018-04-09T19:52:12Z

r? @nikomatsakis

nikomatsakis · 2018-04-09T19:54:23Z

@ehuss

In order to generate the output for a test with multiple revisions, you have to run compiletest separately for each revision because it stops immediately when a revision fails. There doesn't seem to be the equivalent of --no-fail-fast for compiletest. Thoughts?

Hmm, one idea would be to fix #47604 -- @spastorino got most of the way there in #48023, but got entangled in some of the mess that you're sorting out in this PR. A side-effect of this change would be that each revision is its own test, so they would all run independently of a failure.

nikomatsakis · 2018-04-10T16:06:47Z

src/tools/tidy/src/ui_tests.rs

+                if !file_path.with_file_name(testname)
+                             .with_extension("rs")
+                             .exists() {
+                    println!("Stray file with UI testing output: {:?}", file_path);


cc @pnkfelix

Grabbing everything up to first dot to derive the source file name sounds like a reasonable start to me.

nikomatsakis

On second read, I'm perhaps confused.

nikomatsakis · 2018-04-10T16:07:54Z

src/tools/compiletest/src/runtest.rs

-            .with_extension(&self.config.stage_id)
+    }
+
+    fn output_base_name_stage(&self) -> PathBuf {


Maybe a comment?

I believe this returns foo/bar.stage1, right?

Stage also includes the platform triple (like foo/bar.stage1-x86_64-apple-darwin). That is...a little confusing. :)

ok -- in any case I'd like to see a comment =)

nikomatsakis · 2018-04-10T16:08:41Z

src/tools/compiletest/src/runtest.rs

@@ -2864,7 +2879,9 @@ impl<'test> TestCx<'test> {
            }
        }

-        let output_file = self.output_base_name().with_extension(kind);
+        let output_file = self.output_base_name()


Why do we not want the stage here?

Good question. What are the arguments in favor of including the stage? Is it to guard against using the wrong cached results if you run x.py test --stage 1 and x.py test --stage 2 separately?

One issue is that update-references.sh won't know which stage to use. We could add that as another argument, and print the correct stage in the usage instructions.

Some of the other tests that don't include the stage may need to be updated as well. I was reluctant to do so because I don't fully understand the need and consequences.

What are the arguments in favor of including the stage?

The way I always imagined it, we use the stage/target data to effectively "isolate" stage1 and stage2 tests from one another completely. To be honest, I think we should be using separate directories altogether (i.e, src/test/stage1). I'm not sure why we don't, I think for no particularly good reason.

But you're right that I don't know what can go wrong in particular.

One issue is that update-references.sh won't know which stage to use. We could add that as another argument, and print the correct stage in the usage instructions.

Seems fair. We can leave as is for now.

If we moved to ./x.py bless, then you could do ./x.py bless --stage 1 and so forth to specify the test suite that you ran. (Maybe it should be ./x.py test --bless? Anyway.)

bors · 2018-04-11T17:31:49Z

☔ The latest upstream changes (presumably #49861) made this pull request unmergeable. Please resolve the merge conflicts.

ehuss · 2018-04-11T20:26:18Z

@nikomatsakis I just wanted to confirm, what you'd like me to do in regards to the new NLL compare-mode changes. For this PR, I at least need to redo the tidy checks (cc @pnkfelix).

Long term do you want the output files to be $test.$rev.$mode.stderr (where rev and mode are optional)? The new tidy check might be a little aggressive since it can't handle $test.$rev.stderr.

AFAIK, compiletest currently doesn't handle generating output files with the $mode part (and update-references doesn't handle it either). I'm guessing updates to support that should be in another PR?

nikomatsakis · 2018-04-11T21:28:40Z

@ehuss that does need to be decided =)

I think though it might be most practical if compare-mode=nll used an nll revision if available but otherwise used a kind of "default" revision.

nikomatsakis · 2018-04-11T21:29:27Z

That is to say, I feel like 'compare-mode' might perhaps be better considered a kind of "automatic revision" that applies to every test which does not already have one. (And -- indeed -- perhaps revisions also want the same "failover" behavior for their stderr files? I guess that's the question.)

bors · 2018-04-19T13:45:20Z

☔ The latest upstream changes (presumably #49900) made this pull request unmergeable. Please resolve the merge conflicts.

ehuss · 2018-04-20T00:11:29Z

I think this is now working well with the latest NLL updates. With a sample test from compile-fail, it was able to successfully create mir, ast, and ast.nll variants, and everything seems happy.

bors · 2018-04-20T19:06:40Z

☔ The latest upstream changes (presumably #50119) made this pull request unmergeable. Please resolve the merge conflicts.

Fixes rust-lang#48878

nikomatsakis · 2018-04-23T20:18:16Z

I think this is now working well with the latest NLL updates. With a sample test from compile-fail, it was able to successfully create mir, ast, and ast.nll variants, and everything seems happy.

@ehuss can you say a bit more? What was the test you tried? I don't quite understand what "everything seems happy" means. =)

nikomatsakis · 2018-04-23T20:21:42Z

I believe the new behavior is as follows:

If a test has two revisions, say ast and mir, we expect their output in foo.ast.stderr and foo.mir.stderr respectively
If we then run with a compare-mode, say nll, we will look for the new output of each revision in foo.ast.nll.stderr and foo.mir.nll.stderr respectively, falling back to the original paths if those files do not exist

In other words, revisions and compare modes are orthogonal? This seems ok so long as the flags that nll compare-mode passes (e.g., -Zborrowck=mir) are "composable" with the custom flags in the mir revision (in particular, can be given more than once).

nikomatsakis · 2018-04-23T20:22:29Z

@bors r+

bors · 2018-04-23T20:22:30Z

📌 Commit c3af118 has been approved by nikomatsakis

ehuss · 2018-04-23T20:30:45Z

What was the test you tried?

I tried copying over some tests from compile-fail (without the stderr files). Example of going through this dance:

Run x.py test. Test fails.
Run update-references.sh to copy over xxx.ast.stderr.
Run x.py test. Test fails.
Run update-references.sh to copy over xxx.mir.stderr.
Run x.py test. Test fails.
Run update-references.sh to copy over xxx.ast.nll.stderr.
Run x.py test. Test succeeds! (The mir NLL is the same without NLL in this example, so no file is generated.)

And verified that the stderr files look reasonable.

…akis Fix revision support for UI tests. Fixes rust-lang#48878

Rollup of 11 pull requests Successful merges: - #49461 (std: Child::kill() returns error if process has already exited) - #49727 (Add Cell::update) - #49812 (Fix revision support for UI tests.) - #49829 (Add doc links to `std::os` extension traits) - #49906 (Stabilize `std::hint::unreachable_unchecked`.) - #49970 (Deprecate Read::chars and char::decode_utf8) - #49985 (don't see issue #0) - #50118 (fix search bar bug) - #50139 (encourage descriptive issue titles) - #50174 (Use FxHashMap in syntax_pos::symbol::Interner::intern.) - #50185 (core: Fix overflow in `int::mod_euc` when `self < 0 && rhs == MIN`) Failed merges:

rust-highfive assigned nikomatsakis Apr 9, 2018

nikomatsakis reviewed Apr 10, 2018

View reviewed changes

nikomatsakis approved these changes Apr 10, 2018

View reviewed changes

nikomatsakis requested changes Apr 10, 2018

View reviewed changes

pietroalbini added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 10, 2018

ehuss force-pushed the ui-test-revisions branch from 5cda53e to 55ccdf4 Compare April 12, 2018 00:16

pietroalbini added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 16, 2018

ehuss mentioned this pull request Apr 19, 2018

Add src/test/ui regression testing for NLL #49900

Merged

ehuss force-pushed the ui-test-revisions branch from 5b1f55d to bc56181 Compare April 19, 2018 22:13

ehuss added 2 commits April 20, 2018 12:43

Fix revision support for UI tests.

bf816a2

Fixes rust-lang#48878

Add doc for output_base_name_stage.

c3af118

ehuss force-pushed the ui-test-revisions branch from bc56181 to c3af118 Compare April 20, 2018 19:43

nikomatsakis approved these changes Apr 23, 2018

View reviewed changes

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 23, 2018

kennytm added a commit to kennytm/rust that referenced this pull request Apr 24, 2018

Rollup merge of rust-lang#49812 - ehuss:ui-test-revisions, r=nikomats…

b9dbf8e

…akis Fix revision support for UI tests. Fixes rust-lang#48878

kennytm mentioned this pull request Apr 24, 2018

Rollup of 11 pull requests #50191

Merged

bors merged commit c3af118 into rust-lang:master Apr 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix revision support for UI tests. #49812

Fix revision support for UI tests. #49812

ehuss commented Apr 9, 2018

ehuss commented Apr 9, 2018

nikomatsakis commented Apr 9, 2018

nikomatsakis commented Apr 9, 2018

nikomatsakis Apr 10, 2018

pnkfelix Apr 11, 2018

nikomatsakis left a comment

nikomatsakis Apr 10, 2018

nikomatsakis Apr 10, 2018

ehuss Apr 10, 2018

nikomatsakis Apr 11, 2018

nikomatsakis Apr 10, 2018

ehuss Apr 10, 2018

nikomatsakis Apr 11, 2018

nikomatsakis Apr 11, 2018

nikomatsakis Apr 11, 2018

bors commented Apr 11, 2018

ehuss commented Apr 11, 2018

nikomatsakis commented Apr 11, 2018

nikomatsakis commented Apr 11, 2018

bors commented Apr 19, 2018

ehuss commented Apr 20, 2018

bors commented Apr 20, 2018

nikomatsakis commented Apr 23, 2018

nikomatsakis commented Apr 23, 2018 •

edited

Loading

nikomatsakis commented Apr 23, 2018

bors commented Apr 23, 2018

ehuss commented Apr 23, 2018

Fix revision support for UI tests. #49812

Fix revision support for UI tests. #49812

Conversation

ehuss commented Apr 9, 2018

ehuss commented Apr 9, 2018

nikomatsakis commented Apr 9, 2018

nikomatsakis commented Apr 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Apr 11, 2018

ehuss commented Apr 11, 2018

nikomatsakis commented Apr 11, 2018

nikomatsakis commented Apr 11, 2018

bors commented Apr 19, 2018

ehuss commented Apr 20, 2018

bors commented Apr 20, 2018

nikomatsakis commented Apr 23, 2018

nikomatsakis commented Apr 23, 2018 • edited Loading

nikomatsakis commented Apr 23, 2018

bors commented Apr 23, 2018

ehuss commented Apr 23, 2018

nikomatsakis commented Apr 23, 2018 •

edited

Loading