add support for "test revisions" into ui tester

[nikomatsakis]

ui tests do not current support revisions. This blocks our goals to migrate entirely over to unit tests. We should fix it!

I imagine it working like this:

• For each test T with revisions R1..Rn, we would expect N stderr files:
  • T.Ri.stderr, where Ri is the current revision
• Comments on lines work as normal (//[Ri]~ ERROR)

However, it might be useful to permit tests to have a "fallback" T.stderr file, in which case the output is supposed to be the same for all revisions. I am of mixed minds about this though: I don't know how often it would be useful, and I fear "dead stderr files" that are not used. Probably best not to have it.

[nikomatsakis]

Mentoring instructions

Background material is covered in the rustc-guide:

• Revisions
• UI tests

The actual revision mechanism is orthogonal to the kind of test, and already exists. In particular, we will parse out the revisions, and when running tests, wind up here:

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

Lines 165 to 178 in fedce67

	if base_props.revisions.is_empty() {
	base_cx.run_revision()
	} else {
	for revision in &base_props.revisions {
	let revision_props = TestProps::from_file(&testpaths.file, Some(revision), &config);
	let rev_cx = TestCx {
	config: &config,
	props: &revision_props,
	testpaths,
	revision: Some(revision),
	};
	rev_cx.run_revision();
	}
	}

Note that the revision is stored in a field, revision:

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

Lines 185 to 190 in fedce67

	struct TestCx<'test> {
	config: &'test Config,
	props: &'test TestProps,
	testpaths: &'test TestPaths,
	revision: Option<&'test str>,
	}

The UI test functionality is implemented starting here:

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

Line 2492 in fedce67

fn run_ui_test(&self) {

You can see that it loads the expected stderr/stdout here:

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

Lines 2504 to 2508 in fedce67

	let expected_stderr_path = self.expected_output_path(UI_STDERR);
	let expected_stderr = self.load_expected_output(&expected_stderr_path);
	let expected_stdout_path = self.expected_output_path(UI_STDOUT);
	let expected_stdout = self.load_expected_output(&expected_stdout_path);

not complete

[nikomatsakis]

cc @petrochenkov @estebank

[nikomatsakis]

Huh, wait a minute. All the code kind of looks right -- are we sure they don't work? =)

[nikomatsakis]

OK, I dug a bit deeper. They don't work but they....sort of do. Actually the breakage is kind of deep in its own way. The function output_base_name is meant to compute a "base name" for a test that does not include an extension (see its comment?):

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

Lines 1839 to 1848 in 7222241

	/// 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)
	}

However, you can see it actually does add an extension! This is a problem, because some of its callers also invoke set_extension, which means that this information is lost. Also it doesn't include the revision. So we should change this to use - instead of ., basically, and then things would work better.

One problem though: the update_references.sh script would no longer because it guesses the wrong extensions. I'd like to remove that and make it Rust code anyway. I'll try to either set aside some time to fix this better or else mentor it.

[ehuss]

@nikomatsakis I can try to fix this if you'd like. If I understand it correctly, there are roughly two things to fix:

1. compare_output(...) needs to dump each revision of stdout/stderr with the correct filename.
2. update-references.sh needs to copy all revisions.

Is there a particular reason to convert that shell script to Rust? Is that something you'd like done now, or would a simple tweak to glob all the appropriate paths be good enough?

Anything else you'd like done with this?

[nikomatsakis]

@ehuss

Is there a particular reason to convert that shell script to Rust? Is that something you'd like done now, or would a simple tweak to glob all the appropriate paths be good enough?

A simple tweak would be enough. The reason I had planned to convert to Rust was so that it would work on windows etc. But that's a fairly orthogonal task, and maybe it'd be better to step back and see if we can rethink the workflow anyway.

For example, I doubt I would keep it as a separate script, now that I think about it, but rather I would roll it into x.py, so that you write something like

./x.py bless 

or

./x.py bless src/test/ui/foo.rs

[nikomatsakis]

@ehuss

I can try to fix this if you'd like

That'd be awesome, btw!

[ehuss]

but rather I would roll it into x.py

OK, I'd be happy to work on it if you have a clear direction you want to go. Perhaps you have ideas on other things it would cover?

I realize now I neglected to update the .sh script for ui-fulldeps. It seemed odd that it was just copied. Clippy also has its own copy of the script. None of the compile-fail-fulldeps currently rely on revisions, though.

[nikomatsakis]

@ehuss

OK, I'd be happy to work on it if you have a clear direction you want to go. Perhaps you have ideas on other things it would cover?

Maybe we should open a follow-up issue and discuss there.

[nikomatsakis]

@ehuss filed #49815, will add some more details shortly