Fix bugs affecting exception wrapping in rmtree callback #1700

EliahKagan · 2023-10-09T11:22:08Z

Fixes #1698
Fixes #1699
May close #1571

This takes the approaches I suggested in #1698 and #1699 for those two issues. It also adds tests for both the already-working behavior and the new behavior. For the tests of new behavior, I have manually verified they failed in the expected way, and now pass, on native Windows. (Other results can be seen on CI.)

For interpreting the HIDE_WINDOWS_KNOWN_ERRORS and HIDE_WINDOWS_FREEZE_ERRORS environment variables:

I have kept True as the default (though we may be able to change that in the near future, even before fully addressing Remove unittest code from non-test code #790).
0, false, and no, as well as the empty string, are treated as False. This is case-insensitive and ignoring leading and trailing whitespace.
While other values are currently treated as True, values other than possibly padded and possibly case-varying 1, yes, or true show a warning about being unrecognized. This warning is in addition to, not instead of, the warning that is shown from the environment variable being present at all.
The attributes from those variables are always bool, now, too. They are never strings.

When limiting the exceptions the rmtree callback catches and reraises as unittest.SkipTest to PermissionError, I made a couple other changes as well:

I eliminated the duplicated logic, which technically is an additional behavioral change, because in some cases a PermissionError was wrapped in a SkipTest that was itself wrapped in another SkipTest. But I don't think this will cause any problems because even if someone relied on that, the only thing they would likely do would have been to follow the chain to the first non-SkipTest.
As noted in Failing (or warning?) with Python 3.12 alpha 7 #1571, passing a callback as the onerror parameter to shutil.rmtree is deprecated starting in Python 3.12, and the documentation indicates that it is planned for removal in 3.14. So I have used the new onexc parameter on 3.12 (and later). Because this is not available on 3.11 and earlier, it continues to use onerror in those versions. This was easy to implement, because in this case the same callback function naturally works for both, as it does not inspect the error information it receives.

#1571 may or may not be resolved by this, which I think is to a large extent subjective. That issue does two things. It documents a problem from 3.12.0.alpha7, which I believe was fixed, as I commented there. It also notes the deprecated status of shutil.rmtree's onerror argument and advocates that onexc be used instead. This was originally with the idea that using onerror caused or contributed to the observed test failures, but even though that was probably not the case, the issue might have been taken to stand for that as well.

I also fixed the type hints on the callback, renamed the callback itself as handler so it is no longer named after one of those two keyword arguments but not the other (which I think could cause confusing), and renamed its parameters to more closely match the documentation. As it is a local function that is not returned, this does not raise any compatibility issues. In a similar vein, I've included a number of low-stakes refactorings, mostly just to imports, in modules I was already modifying heavily, mainly in test_util.

This also adds a missing xfail and updates commented references to the callback-based skipping logic that already existed. Most tests that get skipped in this way, of which there are about eight, do not have comments like this, and I have not added them. The tests that did are those that had previously been marked with @skipIf decorators. Finally, I've updated the git.util.rmtree docstring, which will affect generated documentation.

This relates to, but only somewhat mitigates and definitely does not solve, #790.

One of the tests that was commented as being skipped as a result of SkipTest rasied in git.util.rmtree or one of the functions that calls it, test_git_submodule_compatibility, was not skipped in that way and was actually failing on Windows with PermissionError. It appears that the cause of failure changed over time, so that it once involved rmtree but no longer does. This removes the outdated comment and adds an xfail mark instead, specific to PermissionError and with a message identifying where in the test case's logic the PermissionError is currently triggered.

The remaining "ACTUALLY skipped by" comments in the test suite were for tests that are actually skipped by SkipTest exceptions raised from the code under test. But the information provided about where in the code they were skipped was out of date, and also not detailed enough because references to line numbers become stale when code is added or removed in the referenced module before the referenced code. This updates them and also provides more detailed information about the referenced code doing the skipping. The error messages are the same as before, and the paths are the same in relevant details, so this doesn't modify those parts of the comments.

In git.util.rmtree, exceptions are caught and conditionally (depending on the value of HIDE_WINDOWS_KNOWN_ERRORS) reraised wrapped in a unittest.SkipTest exception. Although this logic is part of git.util.rmtree itself, two of the calls to that rmtree function contain this same logic. This is not quite a refactoring: because SkipTest derives from Exception, and Exception rather than PermissionError is being caught including in the duplicated logic, duplicated logic where git.util.rmtree was called added another layer of indirection in the chained exceptions leading back to the original that was raised in an unsuccessful attempt to delete a file or directory in rmtree. However, that appeared unintended and may be considered a minor bug. The new behavior, differing only subtly, is preferable.

This reorders them lexicographically within each group, makes spacing/formatting more consistent, and removes the old comment about needing a dict to set .name, which had originally been on what later became the BytesIO import but had become separate from it. (In Python 2, there was a cStringIO type, which could provide a speed advantage over StringIO, but its instances, not having instance dictionaries, didn't support the dynamic creation of new attributes. This was changed to StringIO in 00ce31a to allow .name to be added. It was changed to BytesIO in bc8c912 to work with bytes on both Python 2 and Python 3. The comment about needing a dict later ended up on the preceding line in 0210e39, at which point its meaning was unclear. Because Python 2 is no longer supported and Python 3 has no cStringIO type, the comment is no longer needed, and this commit removes it.)

One of the new test cases fails, showing the bug where git.util.rmtree wraps any exception in SkipTest when HIDE_WINDOWS_KNOWN_ERRORS is true, even though the message it uses (and its purpose) is specific to PermissionError. The other new cases pass, because wrapping exceptions in SkipTest rightly does not occur when HIDE_WINDOWS_KNOWN_ERRORS is false.

staticmethod objects are descriptors that cause the same-named attribute on a class or its instances to be callable (and the first argument, representing a class or instance, not to be passed). But the actual staticmethod objects themselves are only callable starting in Python 3.10. This reorgnizes the just-added test code so it no longer wrongly relies on being able to call such objects.

The onerror function is still called on, and tries to resolve, any exception. But now, when it re-calls the file deletion function passed as func, the only exception it catches to conditionally convert to SkipTest is PermissionError (or derived exceptions). The old behavior of catching Exception was overly broad, and inconsistent with the hard-coded prefix of "FIXME: fails with: PermissionError" used to build the SkipTest exception messages. This commit also changes the message to use an f-string (which was one of the styles in the equivalent but differently coded duplicate logic eliminated in 5039df3, and seems clearer in this case). That change is a pure refactoring, not affecting generated messages.

The onerror parameter of shutil.rmtree receives a 3-tuple like what sys.exc_info() gives, not a string. Since we are not using that parameter anyway, I've fixed it in the onerror function defined in git.util.rmtree by changing it to Any rather than hinting it narrowly. I've also renamed the parameters so the names are based on those that are documented in the shutil.rmtree documentation. The names are not part of the function's interface (this follows both from the documentation and the typeshed hints) but using those names may make it easier to understand the function. - func is renamed to function. - path remains path. - exc_info is renamed to _excinfo. This parameter is documented as excinfo, but I've prepended an underscore to signifity that it is unused. These changes are to a local function and non-breaking. Finally, although not all type checkers will flag this as an error automatically, the git.util.rmtree function, like the shutil.rmtree function it calls, is conceptually void, so it should not have any return statements with operands. Because the return statement appeared at the end, I've removed "return".

The shutil.rmtree callback defined as a local function in git.util.rmtree was already capable of being used as both the old onerror parameter and the new onexc parameter--introduced in Python 3.12, which also deprecates onerror--because they differ only in the meaning of their third parameter (excinfo), which the callback defined in git.util.rmtree does not use. This modifies git.util.rmtree to pass it as onexc on 3.12 and later, while still passing it as onerror on 3.11 and earlier. Because the default value of ignore_errors is False, this makes the varying logic clearer by omitting that argument and using a keyword argument both when passing onexc (which is keyword-only) and when passing onerror (which is not keyword-only but can only be passed positionally if ignore_errors is passed explicitly).

- Slightly improve import sorting, grouping, and formatting. - Move the cygpath pairs parameters into the test class, so they can be immediately above the tests that use them. This was almost the case in the past, but stopped being the case when helpers for some new tests above those were introduced (and those helpers can't be moved inside the class without extra complexity). - Rename TestIterableMember to _Member, so it is no longer named as a test class. The unittest framework wouldn't consider it one, since it doesn't derive from unittest.TestCase, but the pytest runner, which we're actually using, does. More importanly (since it has no test methods anyway), this makes clear to humans that it is a helper class for tests, rather than a class of tests. - Improve the style of _Member, and have its __repr__ show the actual class of the instance, so if future tests ever use a derived class of it--or if its name ever changes again--the type name in the repr will be correct. - Remove the setup method (of TestUtils). It looks like this may at one time have been intended as a setUp method (note the case difference), but it is unused and there doesn't seem to be any attempt to use the instance attribute it was setting. - Use R"" instead of r"" for raw strings representing Windows paths, so that some editors (at least VS Code) refrain from highlighting their contents as regular expressions. - Other very minor reformatting and slight comment rewording.

The new test method just verifies the current behavior of the HIDE_WINDOWS_KNOWN_ERRORS and HIDE_WINDOWS_FREEZE_ERRORS environment variables. This is so there is a test to modify when changing that behavior. The purpose of these tests is *not* to claim that the behavior of either variable is something code that uses GitPython can (or has ever been able to) rely on. This also adds pytest-subtests as a dependency, so multiple failures from the subtests can be seen in the same test run.

This warns if the HIDE_WINDOWS_KNOWN_ERRORS or HIDE_WINDOWS_FREEZE_ERRORS environment variables are set. These behave unexpectedly, including (and especially) in their effect on the same-named git.util module attributes, and neither their effects nor those of those attributes are documented in a way that would have supported code outside the project relying on their specific semantics. The new warning message characterizes their status as deprecated. - This is now the case for HIDE_WINDOWS_KNOWN_ERRORS, and almost so for the same-named attribute, whose existence (though not its meaning) can technically be relied on due to inclusion in `__all__` (which this does *not* change). - But the HIDE_WINDOWS_FREEZE_ERRORS attribute was never guaranteed even to exist, so technically neither it nor the same-named environment variable are not *even* deprecated. The attribute's presence has never been reflected in the public interface of any GitPython component in any way. However, these attributes are still used by the tests. Furthermore, in the case of HIDE_WINDOWS_KNOWN_ERRORS, setting it is the only way to disable the behavior of converting errors from some file deletion operations into SkipTest exceptions on Windows. Since that behavior has not yet changed, but is unlikely to be desired outside of testing, no *attributes* are deprecated at this time, and no effort to warn from accessing or using attributes is attempted.

For now, this doesn't change how the correspondng environment variables are interpreted, in terms of truth and falsehood. But it does *convert* them to bool, so that the values of the HIDE_WINDOWS_KNOWN_ERRORS and HIDE_WINDOWS_FREEZE_ERRORS attributes are always bools. It also updates the tests accordingly, to validate this behavior.

This changes how HIDE_WINDOWS_KNOWN_ERRORS and HIDE_WINDOWS_FREEZE_ERRORS environment variables, if present, are interpreted, so that values that strongly seem intuitivley to represent falsehood now do. Before, only the empty string was treated as false. Now: - "0", "false", "no", and their case variants, as well as the empty string, are treated as false. - The presence of leading and trailing whitespace in the value now longer changes the truth value it represents. For example, all-whitespace (e.g., a space) is treated as false. - Values other than the above false values, and the recognized true values "1", "true", "yes", and their variants, are still treated as true, but issue a warning about how they are unrecognied.

Now that the expected truth values are intuitive, it is no longer necessary to group them by result and include messages that acknowldge the unintuitive cases. This reorders them so that pairs (like "yes" and "no") appear together, removes the messages that are no longer necessary, and reduces test code duplication. This also adds cases to test leading/trailing whitespace in otherwise nonempty strings, so it is clearer what the test is asserting overall, and so a bug where lstrip or rstrip (or equivalent with a regex) were used instead strip would be caught.

The main change here is to move the tests of how the HIDE_* environment variables are treated up near the rmtree tests, since they although the behaviors being tested are separate, they are conceptually related, and also not entirely independent in that they both involve the HIDE_WINDOWS_KNOWN_ERRORS attribute. Other changes are mostly to formatting.

git/util.py

EliahKagan · 2023-10-09T11:41:47Z

test-requirements.txt

@@ -7,4 +7,5 @@ pre-commit
 pytest
 pytest-cov
 pytest-instafail
+pytest-subtests


I've used the subTest method of unittest.TestCase in test_env_vars_for_windows_tests. This pytest plugin gives pytest full support for that mechanism. It lets pytest continue with the other subtests even after one fails, as the unittest test runner would, and show separate passes and failures for the individual subtests. Separate results are shown only when at least one fails, so when everything passes the report remains uncluttered. (It also provides a subtests pytest fixture, though since this is not an autouse fixture it is not usable from within a method of a class that inherits from unittest.TestCase.)

I think this is useful to have going forward, since we have many test cases that are large with many separate assertions of separate facts about the system under test, and as they are updated, some of them could be improved by having their separate claims divided into subtests so they can be individually described and so failures don't unnecessarily block later subtests.

However, if you'd rather this plugin be used, it can be removed. test_env_vars_for_windows_tests could retain subtests--they will still each run if none fails, just like multiple assertions in a test case without subtests. Or I could replace the subtests with more @ddt parameterization, or manually, etc.

I all sounds reasonable and in particular, since you are the one doing the work it seems fair that you can use the tooling you see as best fit. I also have no experience here and no preferences, and believe that anything that improves the tests in any way is very welcome. Thank you!

Sounds good. For the conceptually unrelated reason that it would facilitate more fine-grained xfail marking--so we don't habitually carry XPASSing cases that aren't actually expected to have failed--I think the test class involved here, test.test_util.TestUtils, should be split, so that the cygpath-related tests can be pure pytest tests. The reason to split it is:

@pytest.mark.parametrize supports fine-grained marking of individual test cases, including as skip or xfail, but that form of parameterization cannot be applied to test methods in unittest.TestCase subclasses. So at least those tests would currently (i.e., until the underlying causes of some cases failing are addressed) benefit from being pure pytest tests.

Some of the test methods, not for cygpath, use the rorepo fixture provided by test.lib.helper.TestBase, which inherits from unittest.TestCase. Although this could be converted to a pytest fixture, I'd rather wait to do that until after more operating systems, at least Windows, are tested on CI, and also until I have more insight into whether it makes sense to do that at all, rather than replacing rorepo and other fixtures with new corresponding fixtures that use isolated repositories (#914, #1693 (review)). So at least those tests should currently remain in a unittest.TestCase subclass.

So long as it's acceptable to have multiple test classes in the same test module, this could be done at any time, and it may facilitate some other simplifications. I mention it here because I think it might lead to the elimination of subtests in this particular module, either by using @pytest.mark.parametrize for this too or for other reasons.

If that happens, I might remove the pytest-subtests test dependency, even though it might be re-added later, because the alternative of pytest-check may be preferable in some of the large test methods if they can also be converted to be pure pytest tests (because pytest-check supports a more compact syntax).

The wording was ambiguous before, because fixing a PermissionError could mean addressing it successfully at runtime (which is the intended meaning in that docstring) but it could also mean fixing a bug or test case related to such an error (not intended).

Byron

Thanks again for your amazing work! GitPython is being pulled out of a deep tech-debt hole thanks to it!

#1571 may or may not be resolved by this, which I think is to a large extent subjective.

Let's pro-actively assume it works and let them reply if they don't think so to possibly reopen the issue.

I am super-excited to see the progress towards native-windows CI :).

Please note that the coming weeks I try to handle merge 1 PR per day due to limited bandwidth. Me being slow isn't a sign of my (maybe perceived lack of) appreciation or care for your work. Thanks for your understanding.

Byron · 2023-10-10T07:34:00Z

test-requirements.txt

@@ -7,4 +7,5 @@ pre-commit
 pytest
 pytest-cov
 pytest-instafail
+pytest-subtests


I all sounds reasonable and in particular, since you are the one doing the work it seems fair that you can use the tooling you see as best fit. I also have no experience here and no preferences, and believe that anything that improves the tests in any way is very welcome. Thank you!

git/util.py

EliahKagan · 2023-10-10T23:35:31Z

Please note that the coming weeks I try to handle merge 1 PR per day due to limited bandwidth. Me being slow isn't a sign of my (maybe perceived lack of) appreciation or care for your work. Thanks for your understanding.

No problem!!

I'm unsure if you mean you expect to review one PR in GitPython daily (which would be a higher rate than I am currently managing to open them and I would likely notice no difference), or across all repositories (in which case I might notice some difference, since this would mean longer turnaround times for GitPython). But either way that is absolutely no problem! Extremely short turnaround times for reviewing PRs is something I've actually never seen, in any project, until contributing to GitPython, and I definitely do not expect that to always happen!

(Furthermore, in most cases, having one PR open would not prevent me from opening another PR to make a mostly unrelated change, nor when I have multiple PRs open is there any need for them to be reviewed or merged on the same day, week, etc., as each other, nor necessarily in the order in which they were opened.)

Byron · 2023-10-11T06:57:19Z

I'm unsure if you mean you expect to review one PR in GitPython daily (which would be a higher rate than I am currently managing to open them and I would likely notice no difference), or across all repositories (in which case I might notice some difference, since this would mean longer turnaround times for GitPython

Hehe, I only meant 1 PR per day in GitPython - it's also currently the most active repository thanks to you so most of my time goes here. If you are ever interested how much that it, I do publish timesheets of my public work on a monthly cadence.

EliahKagan · 2023-10-11T07:28:24Z

Thanks! I was aware of the timesheets' existence, yet I had somehow assumed, without checking, that most of your public work, time-wise, went into gitoxide.

Byron · 2023-10-12T07:26:17Z

[..] thanks to you so most of my time goes here.

Right - most of my maintenance time goes here, apologies for the lack of specificity. But even gitoxide takes a back-seat sometimes.

EliahKagan · 2023-10-12T07:55:36Z

Right - most of my maintenance time goes here, apologies for the lack of specificity.

That's okay, I had also assumed (erroneously, and I'm not sure why) that most of your maintenance time went into gitoxide. :)

EliahKagan added 18 commits October 8, 2023 05:03

Test current expected behavior of git.util.rmtree

2fe7f3c

Revise and update rmtree docstrings and comments

7dd5904

This was referenced Oct 9, 2023

HIDE_WINDOWS_*_ERRORS environment variables are interpreted strangely #1698

Closed

git.util.rmtree's callback wraps too many exceptions #1699

Closed

EliahKagan commented Oct 9, 2023

View reviewed changes

git/util.py Show resolved Hide resolved

EliahKagan commented Oct 9, 2023

View reviewed changes

EliahKagan force-pushed the untangle-skips branch from f0e15e8 to 100ab98 Compare October 9, 2023 11:44

EliahKagan force-pushed the untangle-skips branch from a9b05ec to f0e15e8 Compare October 9, 2023 12:05

EliahKagan marked this pull request as ready for review October 9, 2023 12:22

EliahKagan mentioned this pull request Oct 9, 2023

Ask git where its daemon is and use that #1697

Merged

Byron approved these changes Oct 10, 2023

View reviewed changes

Byron merged commit d77fadd into gitpython-developers:main Oct 10, 2023
15 of 16 checks passed

EliahKagan deleted the untangle-skips branch October 10, 2023 23:27

This was referenced Nov 3, 2023

Overbroad xfail marks will eventually make CI fail #1728

Closed

Reorganize test_util and make xfail marks precise #1729

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bugs affecting exception wrapping in rmtree callback #1700

Fix bugs affecting exception wrapping in rmtree callback #1700

EliahKagan commented Oct 9, 2023 •

edited by Byron

Loading

EliahKagan Oct 9, 2023

Byron Oct 10, 2023

EliahKagan Oct 10, 2023 •

edited

Loading

Byron left a comment

Byron Oct 10, 2023

EliahKagan commented Oct 10, 2023

Byron commented Oct 11, 2023 •

edited

Loading

EliahKagan commented Oct 11, 2023

Byron commented Oct 12, 2023

EliahKagan commented Oct 12, 2023

Fix bugs affecting exception wrapping in rmtree callback #1700

Fix bugs affecting exception wrapping in rmtree callback #1700

Conversation

EliahKagan commented Oct 9, 2023 • edited by Byron Loading

EliahKagan Oct 9, 2023

Choose a reason for hiding this comment

Byron Oct 10, 2023

Choose a reason for hiding this comment

EliahKagan Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Byron left a comment

Choose a reason for hiding this comment

Byron Oct 10, 2023

Choose a reason for hiding this comment

EliahKagan commented Oct 10, 2023

Byron commented Oct 11, 2023 • edited Loading

EliahKagan commented Oct 11, 2023

Byron commented Oct 12, 2023

EliahKagan commented Oct 12, 2023

EliahKagan commented Oct 9, 2023 •

edited by Byron

Loading

EliahKagan Oct 10, 2023 •

edited

Loading

Byron commented Oct 11, 2023 •

edited

Loading