make revisions in the test harness show up as independent tests

[nikomatsakis]

So @spastorino recently hacked up a test to include two revisions, one of which enables NLL, by adding:

// revisions: lexical nll
#![cfg_attr(nll, feature(nll))]

This generates the following output:

running 1 test
test [run-pass] run-pass/generator/yield-subtype.rs ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 2883 filtered out

at which point the question is -- did both of those revisions really run successfully?

Currently, if a test has N revisions, we run them all as part of its "main line". It'd be more reassuring if we made the revision part of the test name, so that you get output like:

running 2 tests
test [run-pass] run-pass/generator/yield-subtype.rs#nonlexical ... ok
test [run-pass] run-pass/generator/yield-subtype.rs#nll ... ok

The "early properties" of a test (which are gathered while we are assembling the list of tests) already contains the list of revisions:

rust/src/tools/compiletest/src/header.rs

Line 29 in 5965b79

pub revisions: Vec<String>,

Therefore, it should be a relatively simple thing to extend the make_test function to return not one test, but potentially many tests (one for each revision):

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

Line 600 in 5965b79

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

We would of course have to modify the test description to include which revision to run, and modify the run function, since that is the one that currently iterates over the list of revisions:

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

Lines 165 to 178 in 5965b79

	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();
	}
	}

But this shouldn't be a big change, I wouldn't think.

[nikomatsakis]

Tagging as E-mentor. I gave a few pointers in the description above, but of course happy to go into more detail.

[totorigolo]

Can I take this one?

[spastorino]

@totorigolo I'd say yes, go ahead. In any case check with @nikomatsakis. This #47605 is a very close attempt to implement the thing. Some tests were failing and I gave up because I didn't have any time. It's probably very close, I couldn't even check what the error is about, hehe.

[totorigolo]

Can you explain which tests were failing? I applied your changes and everything seems to be working.

[spastorino]

Run RUST_BACKTRACE=full ./x.py test --incremental --stage 1 src/test/{ui,run-pass,compile-fail}

[totorigolo]

I ran RUST_BACKTRACE=full ./x.py test --incremental --stage 1 src/test/{ui,run-pass} two times: with my branch up to date with 'origin/master' (6c15dff) and with your changes. The same tests failed in both cases:

• [ui] ui/explain.rs
• [ui] ui/lint/unused_parens_json_suggestion.rs
• [ui] ui/lint/use_suggestion_json.rs
• [run-pass] run-pass/out-of-stack.rs
• [run-pass] run-pass/stack-probes-lto.rs
• [run-pass] run-pass/stack-probes.rs

I don't understand why they fail, but they all seem unrelated to this issue.

Here is the result of rustc --version --verbose:

rustc 1.25.0-dev
binary: rustc
commit-hash: unknown
commit-date: unknown
host: x86_64-unknown-linux-gnu
release: 1.25.0-dev
LLVM version: 4.0

[spastorino]

I remember seeing failures in CI, hmmm. Could it be that this PR is actually ok?.
@nikomatsakis should I open a new PR with the same changes against master and wait for the CI to run?.

[nikomatsakis]

@spastorino worth a try =)

[nikomatsakis]

@totorigolo did you ever give this a try?

[nikomatsakis]

cc @ehuss -- I think after your changes, we may be able to get this done! Want to give it a try?

[ehuss]

@nikomatsakis yea, although it looks a little tricky. IIUC, incremental revisions are designed to be run in sequence, reusing the same directory. Supporting that is resulting in a lot of special-case code (directory names, init_incremental_test, generating tests, etc.). What do you think about changing incremental tests to use a new header called "steps"? I think that would simplify things a lot.

[ehuss]

@nikomatsakis I tried a slightly different approach with just a few special cases for Incremental. It works pretty well, however, I'm running into path-length problems on Windows. I tried sending \\?\ paths to rustc, but the Visual Studio linker still seemed to barf on it. Do you know if it is possible to use long paths on Windows? Otherwise, I'll try to think of ways to simplify and abbreviate...

[ehuss]

I put the paths on a diet and things seem to pass on Windows (at least on my system). I have a patch here: ehuss@b558fa3 The commit comment should explain generally what I did. Let me know if you like the idea and you'd like me to post it as a PR. I also had a few questions:

• Are there any tools (besides update-references) that depend on the directory layout of the test output?
• How do I enable wasm32-unknown-unknown? How do I run the tests in test/run-make?
• Why was canonicalize() used in stamp()? Was it for long Windows paths? Why was it not used elsewhere?

[nikomatsakis]

@ehuss

Sorry for dropping off the face of the earth. Trying to catch up on notifications!

IIUC, incremental revisions are designed to be run in sequence, reusing the same directory.

ah true, I forgot about that

What do you think about changing incremental tests to use a new header called "steps"?

👍 seems good

I have a patch here

will take a look!

[nikomatsakis]

@ehuss at first glance, that looks good to me. Maybe open the PR and i'll parse it more carefully.