Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork947
Description
As noted in#790, reallyno exceptions should be wrapped inunittest.SkipTest in production, but there is a more specific problem with what exceptions are wrapped, which this issue is about. This issue could be fixed together with#1698 or separately, but the two are independent.
git.util.rmtree handles errors with this callback, which wraps them inunittest.SkipTest, and which explicitly reports themasPermissionError:
Lines 185 to 196 in8107cbf
| defonerror(func:Callable,path:PathLike,exc_info:str)->None: | |
| # Is the error an access error ? | |
| os.chmod(path,stat.S_IWUSR) | |
| try: | |
| func(path)# Will scream if still not possible to delete. | |
| exceptExceptionasex: | |
| ifHIDE_WINDOWS_KNOWN_ERRORS: | |
| fromunittestimportSkipTest | |
| raiseSkipTest("FIXME: fails with: PermissionError\n {}".format(ex))fromex | |
| raise |
The original logic for that was introduced inbe44602. It occurs multiple times, but this is actually redundant because they all callrmtree fromgit.util, so the code in the callback defined and used ingit.util.rmtree is sufficient. The goal--considering the context in which it was introduced, the code comments, and the explicit message--is to convertPermissionError toSkipTest.
But it catches the much more general exception typeException. BesidesPermissionError, there are some other subclasses ofOSError whose instances can be raised byshutil.rmtree, such asFileNotFoundError. More surprisingly, I have found thatTypeError can be raised if directory traversal is itself what fails, due to a function used in opening the directory not supporting being called with just a path.
Even if this logic were only operational when the project's tests are running, which is not the case, I would take the view that extra exceptions should not be caught and wrapped. However, there is a more confusing effect:no matter what exception is wrapped, the message claims it is aPermissionError, since that name is hard-coded.
I think this should be fixed by having the callback catchPermissionError instead ofException, which I think is what anyone who is relying on the existing of wrapping exceptions inSkipTest is assuming. (I have found that this change does not cause any fewer tests in the test suite to fail on native Windows systems, at least when running them in Python 3.12 on my WIndows 10 system.) The duplication of that logic can be eliminated at the same time. With the same rationale as in#1698, I think it makes sense to fix this now, rather than deferring it to when a full fix for#790 can be implemented. In addition, and unlike in#1698, a fix for this would somewhat decrease the impact of#790.
I've included proposed such a fix for this in#1700 (which also would fix#1698).