fix: possible fix for Intermittent Test Failures in GH Actions

[davidkna]

Description

The recent Test Failures in GH Actions (#886) might be caused be premature tempdir cleanup. This PR attempts to remidy that by closing the tempdirs either expcilitly with close or using into_path and cleaning them up with remove_dir_all.

Motivation and Context

Possible fix for #886

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Screenshots (if appropriate):

How Has This Been Tested?

• I have tested using MacOS
• I have tested using Linux
• I have tested using Windows

Checklist:

• I have updated the documentation accordingly.
• I have updated the tests accordingly.

[chipbuster]

The changes look good to me, thanks for taking the time to do all this! There's a lot of changes in here.

Unfortunately, since this going to affect stuff running on GitHub, there are some other questions I want to ask before we merge:

• How do we determine if this succeeded or not? Should we keep Intermittent Test Failures in GH Actions #886 open for a few weeks until we don't (or do) get failures of this type anymore?
• If this works, how do we document this requirement?
• What's causing this issue in the first place? Is it an issue with NLL causing tempdirs to be dropped too early? I've been puzzling over your comments on Intermittent Test Failures in GH Actions #886 and I don't understand how just adding a .close() can solve the issue if we're taking ownership and dropping early--it seems to me that if we took ownership of the structure and dropped it, we shouldn't be able to call close() on it afterwards, and yet you seem to be doing that just fine.

[davidkna]

How do we determine if this succeeded or not? Should we keep #886 open for a few weeks until we don't (or do) get failures of this type anymore?

Yes it'll need to be kept open for a while.

If this works, how do we document this requirement?

Since I added the change to use sync_all to tests to make them pass on windows this behaviour appears to have spread organically without any formal documentation, so I don't think it's a strict requirement to document this formally but it would be nice to have.
The best place to put these requirements would be CONTRIBUTING.md I guess?

What's causing this issue in the first place? Is it an issue with NLL causing tempdirs to be dropped too early? I've been puzzling over your comments on #886 and I don't understand how just adding a .close() can solve the issue if we're taking ownership and dropping early--it seems to me that if we took ownership of the structure and dropped it, we shouldn't be able to call close() on it afterwards, and yet you seem to be doing that just fine.

.close() causes the tempdir to be used thus keeps it alive and prevents any dropping from occuring earlier. I think drop() might work too here.

All borrows to the tempdir usually go out of scope somewhere inside render_module so I assume the drop handler sometimes gets placed in there for some reason after all explicit references are lost.

As for the git tests with create_fixture_repo() the tempdir goes out of scope by the end of that function before any related tests run which makes me wonder why tests using it ever worked.