Test fixtures are not cleaned up after a test run and may be stale on subsequent runs

[inferiorhumanorgans]

Per #382 the default behavior with test fixtures is:

• to only regenerate them when the contents of the fixture script changes
• to not remove the fixtures after a test success

Potentially more desirable behavior is:

• automatically remove the generated fixture after a test completes
• a tunable (env var) to allow archiving the fixture before removal
• or always archive before removal

I'm thinking the default behavior should be to leverage TempDir with a check at the end to see if an environment variable is set. If the variable is set, archive the fixture directory before the TempDir object goes out of scope.

Alternatively the existing logic (hashing the contents of the fixture script and placing the output in a directory within the repo) could be retained.

With either implementation the question is whether it's more desirable to move towards putting the test logic into closures (for more automatic cleanup) or to add an explicit check at the end of each test.

Thoughts?

[Byron]

Thanks for sharing your thoughts!
I would love to get a better understanding of the problem changes should solve or a proposition.
Thanks again

[inferiorhumanorgans]

The problem I had specifically is that when doing A/B testing I'd toggle between two different versions of a fixture script (let's say CRC=1 and CRC=2). The problem is that the output is always preserved and because the CRC value never changed beyond 1 or 2 the output got reused for the next test run (creating either false success or false failure depending). My solution was to manually remove the generated-do-not-edit directory between runs, but this violates the policy of least surprise – IMO it's reasonable to assume that each test run will re-run the fixture scripts as necessary.

My proposal is to have git_testtools::scripted_fixture_repo_read_only return a TempDir object similar to what git_testtools::scripted_fixture_repo_writable already does. This solves the cleanup portion of the issue.

For the archival portion I think it's pretty well agreed that archiving the fixture output should be gated on an environment variable (e.g. GITOXIDE_ARCHIVE_TEST=1 cargo test …). The question I have is what implementation of this gate would fit best with gitoxide?

• Add an epilogue to each test that uses fixtures e.g. in git-repository

  #[test]
  fn test_with_fixture() -> Result {
    // prelude
    let repo = repo("fixture_script.sh").map(|r| r.to_thread_local())?;
  
    /* test logic */
  
    // epilogue
    archive_if_needed(repo.path(), "name-of-test-case");
  }

• Create a wrapper struct around TempDir whose Drop impl handles the archival logic. I have a mild preference for this because it should "just work" with few invasive changes .
• Create a function that moves test logic into a fixture e.g.:

  #[test]
  fn test_with_fixture() -> Result {
    // run_fixture will create a temp_dir, run the fixture script, run the closure,
    // and optionally create an archive after the closure is finished
    run_fixture("fixture_script.sh", |repo| {
      let repo = repo("fixture_script.sh").map(|r| r.to_thread_local())?;
      /* test logic */ 
    });
  }

[Byron]

IMO it's reasonable to assume that each test run will re-run the fixture scripts as necessary.

I agree, and don't understand how a script can change without changing its CRC. In other words, CRC=1 refers to a state and CRC=2 refers to another, there should be not collisions, and the state should always be the desired one.

Apparently I am still missing something. The only reason to remove the generated-do-not-edit folder was to recreate a clean state because I fiddled with the previous one by hand.

My proposal is to have git_testtools::scripted_fixture_repo_read_only return a TempDir object similar to what git_testtools::scripted_fixture_repo_writable already does.

This can't happen, it's paramount that tests run fast and no work should be done unless needed. The state produced by scripts is immutable. When state needs to be changed by a fixture, there is scripted_fixture_repo_writable(…) which copies from a read-only fixture and returns a tempdir.

For the archival portion I think it's pretty well agreed that archiving the fixture output should be gated on an environment variable (e.g. GITOXIDE_ARCHIVE_TEST=1 cargo test …).

Right now an archive is created each time a fixture is created as the script is run, and all archives currently checked in using git-lfs. These are used to speed up MacOS and Windows runs. The idea is that archives that shouldn't be used are placed in .gitignore and removed from git. Currently they will be created nonetheless, but I can imagine to read the gitignore information and avoid creating such ignored archives unnecessarily.

I hope you can bear with my while I try to understand - I see potential improvements are possible but fail to see the issue the system has, unfortunately.

[inferiorhumanorgans]

Ah, I was toggling between two different versions of a fixture script. Version A that had a CRC of 1 and version B that had a CRC of 2. There was no CRC collision, but because the output is not removed (on failure) when I went from version B to version A the script was not re-run. IMO this is more likely to be an issue when you're actively developing a fixture script.

I suggested including the fixture script's mtime as part of the CRC input because that would give an easy way to force a fixture script to be re-run. But in a more broad sense cleaning up after the fixture script seems like a good idea to me.

[Byron]

Aaah, now I see. In the failure case the folder remains, then the script is indeed not rerun if it does not change. My workflow there was to see the error in the failed test (it failed because the fixture couldn't be initialized), and then edit the script to hopefully fix the issue. Rerunning the test without a script then triggers an unrelated error, which is certainly surprising.

What about… placing a marker in the fixture directory which is only removed when it runs successfully. Then before using an existing directory, it would check if the 'failure marker' exists. If so, it removes the directory and tries again.

That would be fully automatic and needs no adjustments to how things work otherwise. What do you think?

[Byron]

Implemented in 3617ff4 .

[inferiorhumanorgans]

Yeah, the issue I had is that I was going back and forth between versions of the script I'd used before. What I'd like to see is the ability to re-run the script with no intervention. In this case leaving the output in place on failure creates the problem I've run into. Using the mtime of the script would alleviate that (a.k.a. run the script if it's not been run before or if the script's mtime is newer than that of the output directory).

[Byron]

Thanks! The way I see it is that indeed, rerunning a test should yield the same result, and previously this wasn't the case if a failure in the fixture generation occurred. The first run is failing with the correct error message, the second run may fail because the fixture is partial and/or incorrect.

This has been fixed roughly in 3617ff4 and was refined in 7f88c7f.