Fix dogfood tests

[waynr]

@mati865 @flip1995 following up on the discussion in #3443.

I'm currently having trouble getting the dogfood test to fail consistently when there is a known lint failure in the clippy_lints subdirectory. The problem seems to be that if the dogfood test case is run before the dogfood_tests test case, the former will always pass whereas it fails correctly if it is second.

[flip1995]

You can add a test function that calls the two test functions in the right order:

#[test]
fn dogfood_runner() {
    dogfood_tests();
    dogfood();
}

and remove the #[test] attribute from the other two functions.

[ghost]

cargo run ensured that the executable was built. I'm not sure if this is a problem but won't this break if the tests are run in release mode without building in debug first i.e. cargo clean; cargo test --release;. Also will the / path separators work on Windows?

[waynr]

That's a good point, is there a good way to handle release vs debug mode in the binary itself?

[waynr]

I could be wrong, but I think std::path::Path will normalize path separators appropriately for the current operating system. Check this out in the docs: https://doc.rust-lang.org/1.26.0/std/path/struct.Path.html#examples which has a comment indicating that Path::new("./foo/bar.txt") will work on Windows; if that works then I imagine other methods that operate on Path would also.

[flip1995]

There is cfg!(debug_assertions).

[waynr]

@mikerite @flip1995 I think this is ready for another review pass

[waynr]

Fixed the merge conflicts.

[flip1995]

Is this really the right order? Shouldn't it be the other way around or did I misunderstood your comment on this?

[waynr]

It looks like I was mistaken that the order in which the tests are run makes a difference. I've been using the following shell snippet to validate that this dogfood_runner function fixes the indeterminism (regardless of the actual order the implementation functions are called):

for i in $(seq 1 10 ) ;do
echo "run $i"
cargo test --test dogfood
done

Whenever I use the the dogfood_runner function the dogfood case consistently fails when it should, but if I move the #[test] decoration from dogfood_runner to each of dogfood and dogfood_tests the dogfood case only fails when it should something roughly every 3 out of 10 runs, sometimes it just always passes. It's kind of baffling.

[waynr]

Okay, coming back to this after reading through the cargo docs I think the indeterminism stems not from an ordering issue, but from a parallelism issue:

When you run multiple tests, by default they run in parallel using threads. This means the tests will finish running faster so you can get feedback quicker on whether or not your code is working. Because the tests are running at the same time, make sure your tests don’t depend on each other or on any shared state, including a shared environment, such as the current working directory or environment variables.

Both test cases do set the current working directory on the test process itself, but should instead be setting the current working directory only on the child process they are executing. I'm about to push a change to fix this.

[flip1995]

Sorry I didn't get back to this for that long.

This LGTM now. But since it changes pretty much of our CI setup I would like to have a second opinion on this. cc @mikerite @phansch

(Also needs a rebase)

[phansch]

LGTM, too.

[waynr]

Okay, this has been rebased onto master branch @flip1995 @phansch

[phansch]

Thanks!