Keep temp files out of project dir and improve cleanup #1825

EliahKagan · 2024-02-15T17:41:45Z

This fixes temporary directory creation and cleanup in test_tree_modifier_ordering, extracts its helper to a separate method, slightly extends @with_rw_directory so it can be used directly on helper methods while still logging accurate descriptions, and uses it to ensure cleanup of the temporary directory that is used by the helper to generate the expected value that the test will compare its results to.

Because that decorator was not previously used on any test helper methods, only on test methods, it is not obvious that my use of it on the helper, and my modification to it to properly support such use, is justified. The reason I think it is justified is that we really do want to complete cleanup, including deleting the temporary directory, before beginning to use the code under test, in order to make clear (when reading the code or when debugging tests or inspecting output) that the helper really is just arranging an expected value for the test.

There's a little more information in the commit messages, but it's mostly covered by the details in #1824 and this PR description.

One important question is whether the new test really still works as a regression test. The answer is yes, as shown by a temporary non-committed revert of 365d44f (the fix in #1799). This is from before the change in this PR:

(.venv) ek@Glub:~/repos-wsl/GitPython (main $=)$ git log -1 365d44f50a3d72d7ebfa063b142d2abd4082cfaa
commit 365d44f50a3d72d7ebfa063b142d2abd4082cfaa
Author: Ｅthan <[email protected]>
Date:   Mon Jan 15 14:50:43 2024 +0800

    fix: treeNotSorted issue
(.venv) ek@Glub:~/repos-wsl/GitPython (main $=)$ git revert --no-commit 365d44f50a3d72d7ebfa063b142d2abd4082cfaa
(.venv) ek@Glub:~/repos-wsl/GitPython (main +$|REVERTING=)$ pytest --no-cov -vv test/test_tree.py
Test session starts (platform: linux, Python 3.12.1, pytest 8.0.0, pytest-sugar 1.0.0)
cachedir: .pytest_cache
rootdir: /home/ek/repos-wsl/GitPython
configfile: pyproject.toml
plugins: mock-3.12.0, sugar-1.0.0, cov-4.1.0, instafail-0.5.0
collected 3 items

 test/test_tree.py::TestTree.test_serializable ✓                                                          33% ███▍
 test/test_tree.py::TestTree.test_traverse ✓                                                              67% ██████▋

――――――――――――――――――――――――――――――――――――――――― TestTree.test_tree_modifier_ordering ―――――――――――――――――――――――――――――――――――――――――

self = <test.test_tree.TestTree testMethod=test_tree_modifier_ordering>

    def test_tree_modifier_ordering(self):
        def setup_git_repository_and_get_ordered_files():
            os.mkdir("tmp")
            os.chdir("tmp")
            subprocess.run(["git", "init", "-q"], check=True)
            os.mkdir("file")
            for filename in [
                "bin",
                "bin.d",
                "file.to",
                "file.toml",
                "file.toml.bin",
                "file0",
                "file/a",
            ]:
                open(filename, "a").close()

            subprocess.run(["git", "add", "."], check=True)
            subprocess.run(["git", "commit", "-m", "c1"], check=True)
            tree_hash = subprocess.check_output(["git", "rev-parse", "HEAD^{tree}"]).decode().strip()
            cat_file_output = subprocess.check_output(["git", "cat-file", "-p", tree_hash]).decode()
            return [line.split()[-1] for line in cat_file_output.split("\n") if line]

        hexsha = "6c1faef799095f3990e9970bc2cb10aa0221cf9c"
        roottree = self.rorepo.tree(hexsha)
        blob_mode = Tree.blob_id << 12
        tree_mode = Tree.tree_id << 12

        files_in_desired_order = [
            (blob_mode, "bin"),
            (blob_mode, "bin.d"),
            (blob_mode, "file.to"),
            (blob_mode, "file.toml"),
            (blob_mode, "file.toml.bin"),
            (blob_mode, "file0"),
            (tree_mode, "file"),
        ]
        mod = roottree.cache
        for file_mode, file_name in files_in_desired_order:
            mod.add(hexsha, file_mode, file_name)
        # end for each file

        def file_names_in_order():
            return [t[1] for t in files_in_desired_order]

        def names_in_mod_cache():
            a = [t[2] for t in mod._cache]
            here = file_names_in_order()
            return [e for e in a if e in here]

        git_file_names_in_order = setup_git_repository_and_get_ordered_files()
        os.chdir("..")

        mod.set_done()
>       assert names_in_mod_cache() == git_file_names_in_order, "set_done() performs git-sorting"
E       AssertionError: set_done() performs git-sorting
E       assert ['bin', 'bin.d', 'file', 'file.to', 'file.toml', 'file.toml.bin', 'file0'] == ['bin', 'bin.d', 'file.to', 'file.toml', 'file.toml.bin', 'file', 'file0']
E
E         At index 2 diff: 'file' != 'file.to'
E
E         Full diff:
E           [
E               'bin',
E               'bin.d',
E         +     'file',
E               'file.to',
E               'file.toml',
E               'file.toml.bin',
E         -     'file',
E               'file0',
E           ]

test/test_tree.py:99: AssertionError
------------------------------------------------- Captured stdout call -------------------------------------------------
[main (root-commit) ef03b86] c1
 7 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 bin
 create mode 100644 bin.d
 create mode 100644 file.to
 create mode 100644 file.toml
 create mode 100644 file.toml.bin
 create mode 100644 file/a
 create mode 100644 file0

 test/test_tree.py::TestTree.test_tree_modifier_ordering ⨯                                               100% ██████████
=============================================== short test summary info ================================================
FAILED test/test_tree.py::TestTree::test_tree_modifier_ordering - AssertionError: set_done() performs git-sorting

Results (0.24s):
       2 passed
       1 failed
         - test/test_tree.py:45 TestTree.test_tree_modifier_ordering
(.venv) ek@Glub:~/repos-wsl/GitPython (main +$%|REVERTING=)$ rm -rf tmp
(.venv) ek@Glub:~/repos-wsl/GitPython (main +$|REVERTING=)$ git revert --abort
(.venv) ek@Glub:~/repos-wsl/GitPython (main $=)$ pytest --no-cov -vv test/test_tree.py
Test session starts (platform: linux, Python 3.12.1, pytest 8.0.0, pytest-sugar 1.0.0)
cachedir: .pytest_cache
rootdir: /home/ek/repos-wsl/GitPython
configfile: pyproject.toml
plugins: mock-3.12.0, sugar-1.0.0, cov-4.1.0, instafail-0.5.0
collected 3 items

 test/test_tree.py::TestTree.test_serializable ✓                                                          33% ███▍
 test/test_tree.py::TestTree.test_traverse ✓                                                              67% ██████▋
 test/test_tree.py::TestTree.test_tree_modifier_ordering ✓                                               100% ██████████

Results (0.29s):
       3 passed
(.venv) ek@Glub:~/repos-wsl/GitPython (main $%=)$ rm -rf tmp
(.venv) ek@Glub:~/repos-wsl/GitPython (main $=)$

And this is from after the changes in this PR:

(.venv) ek@Glub:~/repos-wsl/GitPython (tree-test $=)$ git status
On branch tree-test
Your branch is up to date with 'origin/tree-test'.

nothing to commit, working tree clean
(.venv) ek@Glub:~/repos-wsl/GitPython (tree-test $=)$ git revert --no-commit 365d44f50a3d72d7ebfa063b142d2abd4082cfaa
(.venv) ek@Glub:~/repos-wsl/GitPython (tree-test +$|REVERTING=)$ pytest --no-cov -vv test/test_tree.py
Test session starts (platform: linux, Python 3.12.1, pytest 8.0.0, pytest-sugar 1.0.0)
cachedir: .pytest_cache
rootdir: /home/ek/repos-wsl/GitPython
configfile: pyproject.toml
plugins: mock-3.12.0, sugar-1.0.0, cov-4.1.0, instafail-0.5.0
collected 3 items

 test/test_tree.py::TestTree.test_serializable ✓                                                          33% ███▍
 test/test_tree.py::TestTree.test_traverse ✓                                                              67% ██████▋

――――――――――――――――――――――――――――――――――――――――― TestTree.test_tree_modifier_ordering ―――――――――――――――――――――――――――――――――――――――――

self = <test.test_tree.TestTree testMethod=test_tree_modifier_ordering>

    def test_tree_modifier_ordering(self):
        """TreeModifier.set_done() sorts files in the same order git does."""
        git_file_names_in_order = self._get_git_ordered_files()

        hexsha = "6c1faef799095f3990e9970bc2cb10aa0221cf9c"
        roottree = self.rorepo.tree(hexsha)
        blob_mode = Tree.blob_id << 12
        tree_mode = Tree.tree_id << 12

        files_in_desired_order = [
            (blob_mode, "bin"),
            (blob_mode, "bin.d"),
            (blob_mode, "file.to"),
            (blob_mode, "file.toml"),
            (blob_mode, "file.toml.bin"),
            (blob_mode, "file0"),
            (tree_mode, "file"),
        ]
        mod = roottree.cache
        for file_mode, file_name in files_in_desired_order:
            mod.add(hexsha, file_mode, file_name)
        # end for each file

        def file_names_in_order():
            return [t[1] for t in files_in_desired_order]

        def names_in_mod_cache():
            a = [t[2] for t in mod._cache]
            here = file_names_in_order()
            return [e for e in a if e in here]

        mod.set_done()
>       assert names_in_mod_cache() == git_file_names_in_order, "set_done() performs git-sorting"
E       AssertionError: set_done() performs git-sorting
E       assert ['bin', 'bin.d', 'file', 'file.to', 'file.toml', 'file.toml.bin', 'file0'] == ['bin', 'bin.d', 'file.to', 'file.toml', 'file.toml.bin', 'file', 'file0']
E
E         At index 2 diff: 'file' != 'file.to'
E
E         Full diff:
E           [
E               'bin',
E               'bin.d',
E         +     'file',
E               'file.to',
E               'file.toml',
E               'file.toml.bin',
E         -     'file',
E               'file0',
E           ]

test/test_tree.py:107: AssertionError
------------------------------------------------- Captured stdout call -------------------------------------------------
[main (root-commit) 391489e] c1
 7 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 bin
 create mode 100644 bin.d
 create mode 100644 file.to
 create mode 100644 file.toml
 create mode 100644 file.toml.bin
 create mode 100644 file/a
 create mode 100644 file0

 test/test_tree.py::TestTree.test_tree_modifier_ordering ⨯                                               100% ██████████
=============================================== short test summary info ================================================
FAILED test/test_tree.py::TestTree::test_tree_modifier_ordering - AssertionError: set_done() performs git-sorting

Results (0.48s):
       2 passed
       1 failed
         - test/test_tree.py:75 TestTree.test_tree_modifier_ordering
(.venv) ek@Glub:~/repos-wsl/GitPython (tree-test +$|REVERTING=)$ git revert --abort
(.venv) ek@Glub:~/repos-wsl/GitPython (tree-test $=)$ pytest --no-cov -vv test/test_tree.py
Test session starts (platform: linux, Python 3.12.1, pytest 8.0.0, pytest-sugar 1.0.0)
cachedir: .pytest_cache
rootdir: /home/ek/repos-wsl/GitPython
configfile: pyproject.toml
plugins: mock-3.12.0, sugar-1.0.0, cov-4.1.0, instafail-0.5.0
collected 3 items

 test/test_tree.py::TestTree.test_serializable ✓                                                          33% ███▍
 test/test_tree.py::TestTree.test_traverse ✓                                                              67% ██████▋
 test/test_tree.py::TestTree.test_tree_modifier_ordering ✓                                               100% ██████████

Results (0.25s):
       3 passed
(.venv) ek@Glub:~/repos-wsl/GitPython (tree-test $=)$

Note that they fail the same way, as desired, when the original bug is temporarily brought back--in particular, see the file list order diff under "Full diff" in each run--and pass when the bugfix is applied.

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.

Byron · 2024-02-15T19:57:36Z

Thanks a lot for your help, and apologies for not catching this.

Detecting that the temporary directory isn't cleaned up should have been possible. But like you said, maybe it was done that way in the first place because cleaning up a .git directory isn't trivial on Windows.

EliahKagan added 4 commits February 15, 2024 10:11

Fix new PermissionError in Windows with Python 3.7

90cf4d7

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.

EliahKagan marked this pull request as ready for review February 15, 2024 17:49

EliahKagan mentioned this pull request Feb 15, 2024

Tests create tmp dir in GitPython dir and fail on second run #1824

Closed

EliahKagan force-pushed the tree-test branch from b780a8c to dd42e38 Compare February 15, 2024 17:55

Byron merged commit 9caf3ae into gitpython-developers:main Feb 15, 2024
49 checks passed

EliahKagan deleted the tree-test branch February 15, 2024 20:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep temp files out of project dir and improve cleanup #1825

Keep temp files out of project dir and improve cleanup #1825

EliahKagan commented Feb 15, 2024 •

edited

Loading

Byron commented Feb 15, 2024

Keep temp files out of project dir and improve cleanup #1825

Keep temp files out of project dir and improve cleanup #1825

Conversation

EliahKagan commented Feb 15, 2024 • edited Loading

Byron commented Feb 15, 2024

EliahKagan commented Feb 15, 2024 •

edited

Loading