Description
Since 08a819c (#1799), running the test suite a second time locally causes a test to fail the second time that passed the first time. This test is TestTree.test_tree_modifier_ordering, and the problem is that:
-
It creates its
tmpdirectory directly in the current working directory of the test runner, which in practice is the top of the GitPython source tree, rather than in/tmpor whatever other location is configured to hold temporary files:Lines 45 to 48 in 7ba3fd2
-
While it typically manages to change directory back (though this is not guarded by
try-finallyor equivalent, it never attempts to delete this directory:Lines 95 to 96 in 7ba3fd2
This does not break CI, because every CI check uses a new clone of the repository. Manually deleting the directory works as a workaround. The bug can be observed in local testing as follows:
(.venv) ek@Glub:~/repos-wsl/GitPython (main $=)$ git status
On branch main
Your branch is up to date with 'origin/main'.
nothing to commit, working tree clean
(.venv) ek@Glub:~/repos-wsl/GitPython (main $=)$ pytest --no-cov -vvk test_tree_modifier_ordering
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
testpaths: test
plugins: mock-3.12.0, sugar-1.0.0, cov-4.1.0, instafail-0.5.0
collected 601 items / 600 deselected / 1 selected
test/test_tree.py::TestTree.test_tree_modifier_ordering ✓ 100% ██████████
Results (0.46s):
1 passed
600 deselected
(.venv) ek@Glub:~/repos-wsl/GitPython (main $%=)$ git status
On branch main
Your branch is up to date with 'origin/main'.
Untracked files:
(use "git add <file>..." to include in what will be committed)
tmp/
nothing added to commit but untracked files present (use "git add" to track)
(.venv) ek@Glub:~/repos-wsl/GitPython (main $%=)$ pytest --no-cov -vvk test_tree_modifier_ordering
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
testpaths: test
plugins: mock-3.12.0, sugar-1.0.0, cov-4.1.0, instafail-0.5.0
collected 601 items / 600 deselected / 1 selected
――――――――――――――――――――――――――――――――――――――――― 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()
test/test_tree.py:95:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
def setup_git_repository_and_get_ordered_files():
> os.mkdir("tmp")
E FileExistsError: [Errno 17] File exists: 'tmp'
test/test_tree.py:47: FileExistsError
test/test_tree.py::TestTree.test_tree_modifier_ordering ⨯ 100% ██████████
=============================================== short test summary info ================================================
FAILED test/test_tree.py::TestTree::test_tree_modifier_ordering - FileExistsError: [Errno 17] File exists: 'tmp'
Results (0.36s):
1 failed
- test/test_tree.py:45 TestTree.test_tree_modifier_ordering
600 deselected
(.venv) ek@Glub:~/repos-wsl/GitPython (main $%=)$ rm -rf tmp
(.venv) ek@Glub:~/repos-wsl/GitPython (main $=)$ pytest --no-cov -vvk test_tree_modifier_ordering
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
testpaths: test
plugins: mock-3.12.0, sugar-1.0.0, cov-4.1.0, instafail-0.5.0
collected 601 items / 600 deselected / 1 selected
test/test_tree.py::TestTree.test_tree_modifier_ordering ✓ 100% ██████████
Results (0.35s):
1 passed
600 deselected
This is a test-only bug, and I believe the test does correctly verify the fix in 365d44f for #369. This test bug only causes the test to wrongly fail (when run a second time with no intervening deletion), not to wrongly pass. It can be fixed pretty straightforwardly by using context managers and/or decorators from the standard library and/or GitPython's test helpers to ensure that cleanup, including deletion of the directory, is deterministically performed.
Although fixing this test bug is fairly straightforward, the simple approach of using TemporaryDirectory to manage the directory is not quite sufficient, because in Python 3.7 on Windows it doesn't succeed at deleting files whose permissions need to be changed for them to be deleted, which includes some read-only files in a .git directory. Since GitPython already has git.util.rmtree to take care of this, which can be used either directly or indirectly through @with_rw_directory, the question of how best to do it mostly comes down to what is to be considered clearest. I've proposed a fix in #1825.