Description
As noted in #790, really no exceptions should be wrapped in unittest.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 in unittest.SkipTest
, and which explicitly reports them as PermissionError
:
Lines 185 to 196 in 8107cbf
The original logic for that was introduced in be44602. It occurs multiple times, but this is actually redundant because they all call rmtree
from git.util
, so the code in the callback defined and used in git.util.rmtree
is sufficient. The goal--considering the context in which it was introduced, the code comments, and the explicit message--is to convert PermissionError
to SkipTest
.
But it catches the much more general exception type Exception
. Besides PermissionError
, there are some other subclasses of OSError
whose instances can be raised by shutil.rmtree
, such as FileNotFoundError
. More surprisingly, I have found that TypeError
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 a PermissionError
, since that name is hard-coded.
I think this should be fixed by having the callback catch PermissionError
instead of Exception
, which I think is what anyone who is relying on the existing of wrapping exceptions in SkipTest
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).