Parallelize tests and implement CmdArgument for TempDir

[casey]

I did this because I thought the tests were slow because of --test-threads=1, but it turns out it's because doctests are just inherently slow, so this doesn't actually improve the speed.

It does allow tests to be run with cargo test (at least if the test helper binaries are built), which is nice.

I also changed it to avoid creating tempfiles in the project root, and removed the corresponding entries in .gitignore.

To do this, I added a CmdArgument for TempDir, which I think is super useful, so I made it an optional feature.

You can enable the tepdir feature to enable the CmdArgument for TempDir, which enables the optional dependency on tempfile.

[soenkehahn]

Hmm, I think I'd prefer not making this change. For the doctests: I think that the main concern should be what would be the best documentation for users. And introducing a tempdir is not really useful and might even be confusing. And by default it doesn't even work for users, since the tempdir feature is not enabled. And for the unit tests: I think introducing a tempdir as an argument to cmd! is not really nice, because it moves the tests slightly further away from the real use-cases that are tested. And personally I have nothing against how in_temporary_directory works.

I do agree that it'd be nice to solve the problems this PR solves: Allowing cargo test to work out of the box and avoiding to write test files into the root directory of the repo. But I think there are different solutions for these problems, that don't come with the above-mentioned drawbacks. For example we could wrap the doctests in in_temporary_directory and comment the lines where that happens, so that they don't show up in the docs. And to allow cargo test, we could have a test wrapper (similar to in_temporary_directory) that acquires a global lock.

But yeah, you also wrote that the initial intention for this PR was to make the tests faster. I agree that that would be great! At the same time I also think that the two solvable problems from above don't have a really high priority. So I would propose to close this PR unmerged. If you feel that either of the two problems are really important, of course feel free to open issues or PRs for them.

What do you think?

[soenkehahn]

I like the previous version better, since it is closer to what we actually want to test. And since this doesn't seem much faster, I wouldn't change this.

[soenkehahn]

(Same for the other tests in here.)

[casey]

Hmm, I think I'd prefer not making this change. For the doctests: I think that the main concern should be what would be the best documentation for users. And introducing a tempdir is not really useful and might even be confusing. And by default it doesn't even work for users, since the tempdir feature is not enabled. And for the unit tests: I think introducing a tempdir as an argument to cmd! is not really nice, because it moves the tests slightly further away from the real use-cases that are tested. And personally I have nothing against how in_temporary_directory works.

I do agree that it'd be nice to solve the problems this PR solves: Allowing cargo test to work out of the box and avoiding to write test files into the root directory of the repo. But I think there are different solutions for these problems, that don't come with the above-mentioned drawbacks. For example we could wrap the doctests in in_temporary_directory and comment the lines where that happens, so that they don't show up in the docs. And to allow cargo test, we could have a test wrapper (similar to in_temporary_directory) that acquires a global lock.

But yeah, you also wrote that the initial intention for this PR was to make the tests faster. I agree that that would be great! At the same time I also think that the two solvable problems from above don't have a really high priority. So I would propose to close this PR unmerged. If you feel that either of the two problems are really important, of course feel free to open issues or PRs for them.

What do you think?

I definitely agree with a lot of your points. The doctests should be as good as possible, and definitely shouldn't rely on an optional feature. That could be done with, instead of having a CmdArgument implementation for &Tempdir, just using CurrentDir:

let tmpdir = Tempdir::new().unwrap();

cmd_unit!(%"touch foo", CurrentDir(tmpdir.path()));

I think that I find in_temporary_directory to just be "weird", and removing it would make the tests clearer to me, and closer to how tests in usually work. I kind of suspect that other rust developers might feel similarly, i.e. they expect tests to just run concurrently, and not modify the global current dir.

One non-aesthetic downside of in_temporary_directory is that if we made it take a lock, then it would be a bug to call the std::env functions without acquiring that lock, since a concurrent in_temporary_directory call might be munging them.

[soenkehahn]

Closing in favor of solving these problems (parallelize tests, get rid of weird in_temporary_directory, not writing test files into the repo root) in subsequent PRs individually. (If we care enough.)