Keep temp files out of project dir and improve cleanup by EliahKagan · Pull Request #1825 · gitpython-developers/GitPython

added 4 commits

February 15, 2024 10:11
This fixes a test bug in TestTree.test_tree_modifier_ordering
where:

- A `tmp` subdirectory of the directory the tests are run from
  (almost always the root of the GitPython source tree) was
  created, instead of creating it in /tmp or elsewhere configured.

- That directory was never deleted afterward, creating new
  untracked files in GitPython's own working tree, and also making
  it so running the tests twice would always fail the second time,
  unless a manual intervening deletion was performed. (This had not
  broken CI, since CI checks clone the repository anew each time.)

- The directory was changed into to set up the test, but not
  deterministically changed back out of. It would typically be
  exited, but this was not guaranteed to occur if some subprocess
  commands were to fail unexpectedly.

In addition to fixing all three aspects of that bug, this also:

- Remains in the temporary directory only as long as necessary to
  execute the subprocesses in it that produce the expected value
  for the test (including not entering it until running them).

- Deletes the temporary directory immediately after exiting it,
  since it is only used to produce a file list to compare to.

- Avoids interleaving file creation and git subprocess commands,
  since doing so made it harder to understand what was being set up
  and since the difference is not relevant to the test.

- Does the work in that temporary directory before performing any
  operations with the code under test, because it is conceptually
  an "arrange" step, and because doing so makes its limited purpose
  clearer on reading the tests.

- Some other refactoring to support the fixes and accompanying
  changes, including extracting the temporary directory logic to a
  helper method.

This deliberately does not change the order in which any files are
created. It also keeps the approach of using subprocess functions
directly to operate on the temporary git repository (and changing
directory while doing so, keeping each of the commands the same),
since there might be good reasons to do this, such as to make very
clear that the file order being obtained to compare to is really
coming from git itself. Even if this is to be changed, it seems
outside the scope of this bugfix.

The test still fails if the fix to git/objects/tree.py from 365d44f
(gitpython-developers#1799) is absent. This was verified by running:

    git revert --no-commit 365d44f
    pytest --no-cov -vv test/test_tree.py
    git revert --abort
In Python 3.7, tempfile.TemporaryDirectory doesn't delete files on
cleanup whose permissions have to be changed to be deleted.

This uses `@with_rw_directory` instead, though that may be overkill
here.
This changes from `@with_rw_directory` back to TemporaryDirectory,
but calls git.util.rmtree on the repsitory's .git directory where
some read-only files otherwise cause TemporaryDirectory's cleanup
to raise PermissionError on Windows in Python 3.7.

This avoids the gc.collect that is known not to be necessary in
this speciifc situation, as well as the problem that, if operating
in the temporary directory did fail, then name of the helper would
be logged as the name of the test where the failure occurred. But
this has the disadvantage of making the helper more complex and
harder to understand. So this may not be the best approach either.
The situation with test_tree_modifier_ordering's helper, where it
makes sense to wrap the helper itself with `@with_rw_directory`
while still deleting the temporary directory as soon as the helper
itself finishes, in order to make clear that the test itself does
not use the temporary directory that the helper used, only occurs
in one place in GitPython's tests as far as I know.

In particular, all other places where `@with_rw_directory` was
actually in use are test methods, not helper methods, and the way
the decorator would have its wrapper log on failure documented the
decorated method as a test. Mainly for that reason, I had backed
away from using `@with_rw_directory` here.

But the test code seems much easier to understand when it is used
compared to other approaches. While it also has the disadvantage of
doing a gc.collect that is unnecessary here, this is not the only
place what that is done.

This commit brings back the test_tree_modifier_ordering helper
implementation that uses `@with_rw_directory`, while also modifying
the logging logic in `@with_rw_directory` slightly, so that when
the decorated method is not named as a test, it is referred to as a
"Helper" rather than a "Test" in the failure log message.

lettuce-bot bot referenced this pull request in lettuce-financial/github-bot-signed-commit

Feb 15, 2024

renovate bot referenced this pull request in allenporter/flux-local

Feb 16, 2024