Reorganize test_util and make xfail marks precise

[EliahKagan]

Fixes #1728

This makes xfail markings precise in the test suite, so that only tests that are expected to fail (and do fail on CI) are marked xfail. This addresses both the general problem that spurious XPASS statuses make the information from xfail-marked tests less informative, and the specific problem focused on in #1728.

In addition, this also refactors some other recently added tests in test_util.py, to achieve the goals described there. I may soon propose other changes that build on this in other ways, but that is not included here. Basically, this just:

• Reorganizes the tests in test_util.py, making some of them pure pytest tests.
• Uses @pytest.mark.parametrize with the marks= argument to apply @pytest.mark.xfail to exactly the cases that currently need it, and no others.
• Refactors some of the other tests' use of ad-hoc fixtures, removing some code reuse that I had added that was making the code harder rather than easier to maintain, and achieving useful code reuse more cleanly with a custom pytest fixture.

This includes two changes to the pytest plugins that are used. Since those affect development dependencies, I list them here with their rationales:

• Because the parametrized mark supports nesting, generating test cases for the Cartesian product of the specified parameters, I removed the use of subtests for now, and thus also the pytest-subtests development dependency. Subtests, and that plugin, could be brought back in the future if they prove useful for another reason, but right now there are no longer any subtests in the test suite. This is the change I had anticipated in #1700 (comment).
• I have added and used pytest-mock for some of the new pure pytest tests that had previously used nested with-statements. The pytest-mock plugin provides a mocker fixture that wraps the functionality of unittest.mock but automatically unpatches everything (and in the reverse order in which it was patched) after the test.

Since we do not yet have CI for native Windows, I have manually run the test_util.py tests on Windows (on Windows 10, with Python 3.12) to verify that they produce the expected statuses (including no FAIL statuses).

[Byron]

This looks so much cleaner, thanks so much!

Please note that my one comment merely is a note and nothing more, as I understand this code as always been like that and worked fine in single-threaded test-runners, which is all it has to be able to do.

[Byron]

I keep choking on this as it seems more fragile than it has to be given that one could also create a tempdir with a unique name as well.

[EliahKagan]

Can you tell me more about this concern? I'll be working on test_util.py again soon, so I'll have a good opportunity to make further improvements. However, tmp_path is already always unique: it's a Path object for a path like /tmp/hw9aphg (or a more systematically named path, but uniquely picked by pytest), not /tmp itself.

[Byron]

Ah, perfect, then there is no concern, I just wasn't aware and too overwhelmed/not very focussed at the time.

[EliahKagan]

then there is no concern

Ah, excellent.

Some of the code this PR modifies are the tests I introduced it in #1700. Both before and after the changes here, these tests may give the mistaken impression that variables like td refer to objects representing paths like /tmp/tmpdir that are always the same, rather than paths like /tmp/hw9aphg/tmpdir (or, with pytest, a more systematic scheme but still one that makes sure to use an initially nonexistent location).

After all, if tmp_path really corresponds to a unique and newly created directory, why not use it rather than creating a tmp_path / "testdir" subdirectory?

I do this for several reasons:

• I think of tmp_path as a resource the testing framework and/or custom fixtures manage: it is created before the test and (where appropriate and feasible) removed after the test. So for testing the behavior of git.util.rmtree in deleting a directory, I make a subdirectory for that, even if I wouldn't have to.
• Currently, I have configured pytest not to attempt to delete the temporary directory it creates afterwards, if the test failed. This allows the directory to be examined to troubleshoot a regression that causes a failure. But this would be confusing to actually do, if there is confusion between the pytest-managed temporary directory that may be deleted if the test succeeds, and the directory that the code under test attempts to delete.
• In at least one test, conceptually zero directories are created, and the purpose of the temporary directory is to ensure that it doesn't have an entry of a particular name in it, so that this nonexistent entry can itself be used, to test what happens when the code under test raises a FileNotFoundError.
• In some forthcoming tests I plan to propose soon (but that I've had in mind for a short while, including when working on this PR), two directories will be created. Making them subdirectories of the tmp_path directory will make the test code a bit simpler, as I won't have to use a more versatile but complex fixture to create multiple pytest-managed temporary directories. More importantly, it will allow the two directories being affected by git.util.rmtree to be distinguished from each other when examining the tests and their results, rather than having them both have meaningless names that change each time.

This may be insufficiently clear in reading the test code, and if so, I would be interested in improving the situation, perhaps by changing some variable names, adding/expanding some docstrings or comments, or both. But maybe that wouldn't help; I'm not sure.

(Edit: Regarding the broader issue of test-runner parallelism, I've posted a separate top-level comment below.)

[Byron]

Thanks for the explanation! Personally I think that it's OK to leave it as is but refactor the next time one stumbles over it. Please note that I didn't read the top-level comment yet, to which I will probably reply separately.

[EliahKagan]

(I've posted in the review thread above about the narrower and separate issue of why I use paths like tmp_path / "testdir" instead of just tmp_path.)

I'm curious if there are any areas where the test suite is written in a way that prevents parallelism that would otherwise be feasible and desirable. If so, I would be interested in trying to remedy that.

Some of the use of temporary files is done with insecure deprecated functions, which would best be replaced, as noted in gitpython-developers/smmap#41. That issue is in smmap, but GitPython and gitdb also contain such code. Although these are subject to a race condition, it is mostly an issue between the process that uses the insecure function and other processes running other code, rather than in the affected code performing multithreading or multiprocessing. The same temporary paths are not automatically reused, except by strange chance or deliberate arrangement; new pseudorandom names for temporary files and directories are still generated. The biggest reason it can be a problem is security-related.

The facility that was previously used in the specific test code changed in this PR was TemporaryDirectory from the Python standard library. Like pytest's tmp_path, this is free of that security weakness, and like all the techniques for getting usable temporary file and directory paths discussed here, it gives a new path and can be used in parallel (including, but not limited to, across threads of the same process).

I'm aware of two factors that limit GitPython itself in multithreaded use. The first is that it changes directory temporarily to do some operations, as reported in #1052. The second is that the fix I made for #1635 (in #1636, and improved in #1650 to fix #1646) creates a race condition on os.environ, as pointed out in #1650 (comment) (see also #1650 (comment)). So that the tests also change directory and patch os.environ doesn't seem like a problem.

The test suite does also do various monkey-patching that would cause problems if tests were run using multithreading, and this goes beyond any problem that I am aware of already existing in the code under test. It is possible to attempt to run tests using multithreading, and I think there are plugins for doing that, though I'm not familiar with the details of any of them. What I think is more common, and more useful for Python tests, is to run them using multiprocessing, such as with pytest-xdist. This retains isolation across tests, so it should work fine even when tests change directory, mutate os.environ, and monkey-patch GitPython and standard library facilities. Multiprocessing is also typically faster than multithreading in Python, except for code that is IO-bound or that delegates CPU-bound operations to native-code extension modules that release the GIL, though this situation will eventually improve.

If the same temporary directory/files are reused in a way that can clash, or if code is taking what amounts to a lock on rorepo's shared backing repository (currently the GitPython repository), etc., then this would get in the way of multiprocessing just as much as multithreading, thereby preventing test parallelism from being achieved through multiprocessing using pytest-xdist or other such tools. This is the sort of thing I'd be especially interested in becoming aware of and improving, though if there are reasons to specifically want multithreaded test runners to work, then I am of course interested.

[Byron]

Thanks for the analysis!

The reason I keep thinking about multi-threading is that it would force one to properly sandbox tests. Rust doest that by default as well.
This wasn't to say it's a goal for GitPython to be thread-safe, or to work in a multiprocressing powered environment nor that a test-runner which uses multiprocessing should be introduced - I'd leave everything as is as long as it works well enough.

I think what I try to say is that software that can deal with these situations typically correctly handles its state, which is always desirable. However, in Python I'd consider that out of scope (whereas in Rust, it's naturally happening). Overall, I think my relationship to Rust changes the way I think about everything else, even though achieving such a level of quality an diligence outside of Rust is usually close to impossible or at least incredibly frustrating.