Make revision tests show up as test.rs#revision #47605

spastorino · 2018-01-19T23:54:51Z

Mark-Simulacrum · 2018-01-19T23:57:50Z

src/tools/compiletest/src/main.rs

@@ -597,7 +597,7 @@ pub fn is_test(file_name: &OsString) -> bool {
    !invalid_prefixes.iter().any(|p| file_name.starts_with(p))
 }

-pub fn make_test(config: &Config, testpaths: &TestPaths) -> test::TestDescAndFn {
+pub fn make_test_with_revisions(config: &Config, testpaths: &TestPaths) -> Vec<test::TestDescAndFn> {


I don't think we should rename this function. It makes the test with and without revisions. With the new name I kind of expect a "default" make_test function somewhere.

👍 will change it back

nikomatsakis · 2018-01-19T23:59:41Z

src/tools/compiletest/src/main.rs

+                    should_panic,
+                    allow_fail: false,
+                },
+                testfn: make_test_closure(config, testpaths),


I think we have to be threading the revision down into the test closure here...

nikomatsakis · 2018-01-20T00:00:05Z

src/tools/compiletest/src/runtest.rs

-            rev_cx.run_revision();
-        }
-    }
+    base_cx.run_revision();


....so that here we can be given the revision, and load up the revision_props and so forth like we used to do

nikomatsakis · 2018-01-20T01:09:49Z

tidy errors

shepmaster · 2018-01-20T18:15:12Z

[00:57:24] stderr:
[00:57:24] ------------------------------------------
[00:57:24] warning: unused `std::result::Result` which must be used
[00:57:24]    --> /checkout/src/test/run-pass/impl-trait/example-calendar.rs:315:13
[00:57:24]     |
[00:57:24] 315 |             write!(s, "{}", e);
[00:57:24]     |             ^^^^^^^^^^^^^^^^^^^
[00:57:24]     |
[00:57:24]     = note: #[warn(unused_must_use)] on by default
[00:57:24]     = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
[00:57:24] 
[00:57:24] warning: unused `std::result::Result` which must be used
[00:57:24]    --> /checkout/src/test/run-pass/impl-trait/example-calendar.rs:318:17
[00:57:24]     |
[00:57:24] 318 |                 write!(s, "{}", e);
[00:57:24]     |                 ^^^^^^^^^^^^^^^^^^^
[00:57:24]     |
[00:57:24]     = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
[00:57:24] 
[00:57:24] warning: unused `std::result::Result` which must be used
[00:57:24]    --> /checkout/src/test/run-pass/impl-trait/example-calendar.rs:542:13
[00:57:24]     |
[00:57:24] 542 |             write!(buf, " {:>2}", d.day());
[00:57:24]     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[00:57:24]     |
[00:57:24]     = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

nikomatsakis · 2018-01-22T18:39:47Z

So I think the problem is that both tests are outputting things into the same directory. That's kind of annoying. Still, it seems kind of like a bad feature of the existing test harness too!

We can probably fix this by tweaking the way we compute the output file names. For example, we might alter output_base_name to take the revision into account:

rust/src/tools/compiletest/src/runtest.rs

Lines 1825 to 1834 in fdc18b3

    
               /// Given a test path like `compile-fail/foo/bar.rs` Returns a name like 
        
               /// 
        
               ///     <output>/foo/bar-stage1 
        
               fn output_base_name(&self) -> PathBuf { 
        
                   let dir = self.config.build_base.join(&self.testpaths.relative_dir); 
        
                   // Note: The directory `dir` is created during `collect_tests_from_dir` 
        
                   dir.join(&self.output_testname(&self.testpaths.file)) 
        
                       .with_extension(&self.config.stage_id) 
        
               }

It looks kind of like get_mir_dump_dir would need to be updated too:

rust/src/tools/compiletest/src/runtest.rs

Lines 2739 to 2745 in fdc18b3

    
           fn get_mir_dump_dir(&self) -> PathBuf { 
        
               let mut mir_dump_dir = PathBuf::from(self.config.build_base.as_path()); 
        
               debug!("input_file: {:?}", self.testpaths.file); 
        
               mir_dump_dir.push(&self.testpaths.relative_dir); 
        
               mir_dump_dir.push(self.testpaths.file.file_stem().unwrap()); 
        
               mir_dump_dir 
        
           }

I guess I'd search around for other uses of config.build_base combined with relative dir too. (I did a quick search and didn't see any, but maybe I missed something.)

spastorino · 2018-01-22T21:44:24Z

@nikomatsakis last comment, you wanted to add a comment like

rust/src/tools/compiletest/src/main.rs

Lines 543 to 550 in 5965b79

    
           // If we find a test foo/bar.rs, we have to build the 
        
           // output directory `$build/foo` so we can write 
        
           // `$build/foo/bar` into it. We do this *now* in this 
        
           // sequential loop because otherwise, if we do it in the 
        
           // tests themselves, they race for the privilege of 
        
           // creating the directories and sometimes fail randomly. 
        
           let build_dir = config.build_base.join(&relative_dir_path); 
        
           fs::create_dir_all(&build_dir).unwrap();

let me know if everything is correct, where do you think is better to add that

nikomatsakis · 2018-01-23T13:44:39Z

src/tools/compiletest/src/main.rs

-            allow_fail: false,
-        },
-        testfn: make_test_closure(config, testpaths),
+    let dir = config.build_base.join(&testpaths.relative_dir)


we should add a comment here explaining (a) the directory layout that we are going to be creating (i.e., with some examples) and (b) why we are doing that here and not in runtest

nikomatsakis · 2018-01-23T13:46:23Z

src/tools/compiletest/src/runtest.rs

@@ -1830,7 +1828,8 @@ impl<'test> TestCx<'test> {

        // Note: The directory `dir` is created during `collect_tests_from_dir`
        dir.join(&self.output_testname(&self.testpaths.file))
-            .with_extension(&self.config.stage_id)
+            .join(self.revision.unwrap_or(""))


here also we should update the comment on the function, which doesn't talk about revisions; also, does join have no effect when its argument is an empty string? I feel like I expect this to rather be:

let mut dir = ...; dir = dir.join(...); if let Some(r) = self.revision { dir = dir.join(r); } dir = dir.join(&self.config.stage_id); dir

spastorino · 2018-01-22T21:41:00Z

src/tools/compiletest/src/main.rs

+            let dir = config.build_base.join(&testpaths.relative_dir)
+                .join(PathBuf::from(&testpaths.file.file_stem().unwrap()))
+                .join(revision);
+            fs::create_dir_all(&dir).unwrap();


@nikomatsakis This is building a directory structure like `/home/santiago/src/oss/rust1/build/x86_64-unknown-linux-gnu/test/run-pass/borrowck/two-phase-baseline/lxl/stage1-x86_64-unknown-linux-gnu". So this code creates the dir up to lxl and stage1-x86_64-unknown-linux-gnu is the file. Let me know if you prefer something different

This looks right to me

spastorino · 2018-01-22T21:42:18Z

src/tools/compiletest/src/runtest.rs

-            .with_extension(&self.config.stage_id)
+        let r = dir.join(&self.output_testname(&self.testpaths.file))
+            .join(self.revision.unwrap_or(""))
+            .join(&self.config.stage_id)


@nikomatsakis This logic is repeated, we'd probably like to have a function around that computes this.

spastorino · 2018-01-22T21:43:27Z

src/tools/compiletest/src/runtest.rs

@@ -2741,6 +2740,7 @@ impl<'test> TestCx<'test> {
        debug!("input_file: {:?}", self.testpaths.file);
        mir_dump_dir.push(&self.testpaths.relative_dir);
        mir_dump_dir.push(self.testpaths.file.file_stem().unwrap());
+        mir_dump_dir.push(self.revision.unwrap_or(""));


@nikomatsakis This also is kind of similar but using relative dirs. We may probably want two functions, one that computes the relative thing and one that computes the full thing and uses the relative one.

spastorino · 2018-01-22T21:51:13Z

src/tools/compiletest/src/runtest.rs

@@ -1830,7 +1828,8 @@ impl<'test> TestCx<'test> {

        // Note: The directory `dir` is created during `collect_tests_from_dir`
        dir.join(&self.output_testname(&self.testpaths.file))
-            .with_extension(&self.config.stage_id)
+            .join(self.revision.unwrap_or(""))
+            .join(&self.config.stage_id)


@nikomatsakis This logic is repeated, we'd probably like to have a function around that computes this.

spastorino · 2018-01-22T22:07:24Z

src/tools/compiletest/src/main.rs

+        }]
+    } else {
+        early_props.revisions.iter().map(|revision| {
+            fs::create_dir_all(dir.join(revision)).unwrap();


@nikomatsakis This is building a directory structure like `/home/santiago/src/oss/rust1/build/x86_64-unknown-linux-gnu/test/run-pass/borrowck/two-phase-baseline/lxl/stage1-x86_64-unknown-linux-gnu". So this code creates the dir up to lxl and stage1-x86_64-unknown-linux-gnu is the file. Let me know if you prefer something different

spastorino · 2018-01-24T13:47:23Z

Ok, this is not working. We talked with @nikomatsakis to put this aside for a while. We may come back to this later. If someone wants to take over from where it is, feel free to do so. Otherwise, I may revisit it at some point.

rust-highfive assigned nikomatsakis Jan 19, 2018

Mark-Simulacrum reviewed Jan 19, 2018

View reviewed changes

nikomatsakis reviewed Jan 19, 2018

View reviewed changes

nikomatsakis reviewed Jan 20, 2018

View reviewed changes

spastorino force-pushed the make_revision_tests_show_up branch from 6ec4956 to 6dd40b8 Compare January 20, 2018 00:54

nikomatsakis mentioned this pull request Jan 20, 2018

who tests the tester? #47606

Open

spastorino force-pushed the make_revision_tests_show_up branch from 6dd40b8 to ed12efa Compare January 20, 2018 01:23

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

spastorino force-pushed the make_revision_tests_show_up branch from ed12efa to 2c379cc Compare January 22, 2018 21:36

spastorino force-pushed the make_revision_tests_show_up branch 3 times, most recently from a4c85fb to c65b6b8 Compare January 22, 2018 22:06

nikomatsakis requested changes Jan 23, 2018

View reviewed changes

spastorino commented Jan 23, 2018

View reviewed changes

spastorino force-pushed the make_revision_tests_show_up branch 4 times, most recently from ddc7c72 to 83a36db Compare January 23, 2018 20:53

Make revision tests show up as test.rs#revision

9631e13

spastorino force-pushed the make_revision_tests_show_up branch from 83a36db to 9631e13 Compare January 23, 2018 21:57

nikomatsakis closed this Jan 24, 2018

This was referenced Jan 31, 2018

make revisions in the test harness show up as independent tests #47604

Closed

Make revision tests show up as test.rs#revision #48023

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make revision tests show up as test.rs#revision #47605

Make revision tests show up as test.rs#revision #47605

spastorino commented Jan 19, 2018

Mark-Simulacrum Jan 19, 2018

spastorino Jan 20, 2018

nikomatsakis Jan 19, 2018

nikomatsakis Jan 20, 2018

nikomatsakis commented Jan 20, 2018

shepmaster commented Jan 20, 2018

nikomatsakis commented Jan 22, 2018

spastorino commented Jan 22, 2018

nikomatsakis Jan 23, 2018

spastorino Jan 23, 2018

nikomatsakis Jan 23, 2018

spastorino Jan 23, 2018

spastorino Jan 22, 2018

nikomatsakis Jan 23, 2018

spastorino Jan 22, 2018

spastorino Jan 22, 2018

spastorino Jan 22, 2018

spastorino Jan 22, 2018

spastorino commented Jan 24, 2018

Make revision tests show up as test.rs#revision #47605

Make revision tests show up as test.rs#revision #47605

Conversation

spastorino commented Jan 19, 2018

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

nikomatsakis commented Jan 20, 2018

shepmaster commented Jan 20, 2018

nikomatsakis commented Jan 22, 2018

spastorino commented Jan 22, 2018

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

spastorino commented Jan 24, 2018