Testing bug: Temporary directories are not removed after tests

[mkpro118]

The Problem

The core command tests do not clean up the temporary directories they create in setup after tests are done. In particular, the following tests fail to remove the tempdirs,

• tests::core::test_cat_file
• tests::core::test_hash_object
• tests::core::test_init
• tests::core::test_ls_tree
• tests::utils::test_fnmatch
• mini_git::utils::path

Anticipated Cause

I believe the error is with TempDirs that are being used with static Mutexes, as drop is not called on them, no cleanup happens with the interior data.

Proposed Solution

So far the only solution I believe is guaranteed to work is:

1. Maintain a static count of all the tests
2. Call a cleanup function after each test. cleanup would decrement the count of tests
3. Call drop on the TempDir when the count reaches zero.

I am not a big fan of this since it requires manually setting the number of tests, which is harder to maintain and prone to error.

Essentially, the trivial fix would be something along the lines of

static mut TEST_COUNT: usize = /* number or tests*/;
static FS_MUTEX: Mutex<TempDir<()>> = /* FileSystem Mutex */;

#[test]
fn my_test() {
  // Run test
  cleanup();
}

fn cleanup() {
  unsafe { TEST_COUNT -= 1; }
  if TEST_COUNT == 0 {
    match FS_MUTEX.lock() {
      Ok(mut inner) if inner.is_some() => *inner = None, // This should call drop on the TempDir
      Ok(..) => {}
      Err(..) => panic!("Mutex failed!"),
    };
  }
}

[mkpro118]

Attempting to fix this in PR #46, but at least on windows, the std::fs::remove_dir_all fails to unlink the temporary directories.

The call to remove_dir_all fails with the error

Err(
  Os {
    code: 32,
    kind: Uncategorized,
    message: "The process cannot access the file because it is being used by another process." 
  }
)

This clearly suggests another process has an open handle to the temporary directory. Unfortunately I have no idea which process could possibly have an open handle. Is it cargo that has an open handle?

With some debug logging, as of commit 4dbc65a, running cargo test --tests test_cat_file -- --nocapture produces (obviously prone to race conditions) the following output ($TEMPDIR is the path to the system's tempdir)

running 3 tests
incr: 1
incr: 2
decr: 1
incr: 2
test core::test_cat_file::tests::test_cmd_cat_file_testfile ... ok
decr: 1
test core::test_cat_file::tests::test_cmd_cat_file_readme ... ok
decr: 0
Dropping MUTEX!!!!
"$TEMPDIR\\cmd_cat_file1721716735083089700"
CLEANING UP "$TEMPDIR\\cmd_cat_file1721716735083089700"
RESULT Err(Os { code: 32, kind: Uncategorized, message: "The process cannot access the file because it is being used by another process." })
Ok(/* directory where the test command was executed */)
"$TEMPDIR\\cmd_cat_file1721716735083089700" exists? true
test core::test_cat_file::tests::test_cmd_cat_file_bad_file ... ok

However the "Dropping MUTEX" is always after all the test are complete, so there really should not be any open handles to the files in the temporary directory. Additionally, all FS operations are done with mutexes, so it should not be possible for any handles to not be closed due to race conditions.

[mkpro118]

Okay this is super weird, but the deletion works when run with many other tests. I reduced some output verbosity in tests in commit b554361, and only print in TempDir::drop if remove_dirs_all returns Err.

Running cargo test --tests core::test_ -- --nocapture, I get the following output,

running 19 tests
test core::test_init::tests::test_cmd_init_extra_args ... ok

Deleting "$TEMPDIR\\cmd_init_no_args1721719759369800200"

test core::test_hash_object::tests::test_cmd_hash_object_testfile ... ok

incr: 1
incr: 2

test core::test_init::tests::test_cmd_init_path ... ok
test core::test_hash_object::tests::test_cmd_hash_object_readme_write ... ok
test core::test_ls_tree::tests::test_dir1_no_recursive ... ok

decr: 1
incr: 2

test core::test_cat_file::tests::test_cmd_cat_file_testfile ... ok

Deleting "$TEMPDIR\\cmd_init_no_args1721719759370316300"
Deleting "$TEMPDIR\\cmd_init_no_args1721719759370517600"
decr: 1

test core::test_ls_tree::tests::test_dir2_no_recursive ... ok
test core::test_hash_object::tests::test_cmd_hash_object_testfile_write ... ok
test core::test_hash_object::tests::test_cmd_hash_object_readme ... ok

decr: 0
Dropping MUTEX!!!!
Deleting "$TEMPDIR\\cmd_cat_file1721719759369280000"   /* Deletion somehow worked */

// A bunch of other test results omitted

However the output is unchanged on running tests with cargo test --tests test_cat_file -- --nocapture.

Running the full test suite with cargo test --tests -- --nocapture also fails to unlink the temporary directory. Very weird.