Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bugs affecting exception wrapping in rmtree callback #1700

Merged
merged 19 commits into from
Oct 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 11 additions & 33 deletions git/objects/submodule/base.py
Original file line number Diff line number Diff line change
@@ -1,44 +1,37 @@
# need a dict to set bloody .name field
from io import BytesIO
import logging
import os
import os.path as osp
import stat
import uuid

import git
from git.cmd import Git
from git.compat import (
defenc,
is_win,
)
from git.config import SectionConstraint, GitConfigParser, cp
from git.compat import defenc, is_win
from git.config import GitConfigParser, SectionConstraint, cp
from git.exc import (
BadName,
InvalidGitRepositoryError,
NoSuchPathError,
RepositoryDirtyError,
BadName,
)
from git.objects.base import IndexObject, Object
from git.objects.util import TraversableIterableObj

from git.util import (
join_path_native,
to_native_path_linux,
IterableList,
RemoteProgress,
join_path_native,
rmtree,
to_native_path_linux,
unbare_repo,
IterableList,
)
from git.util import HIDE_WINDOWS_KNOWN_ERRORS

import os.path as osp

from .util import (
SubmoduleConfigParser,
find_first_remote_branch,
mkhead,
sm_name,
sm_section,
SubmoduleConfigParser,
find_first_remote_branch,
)


Expand Down Expand Up @@ -1060,28 +1053,13 @@ def remove(
import gc

gc.collect()
try:
rmtree(str(wtd))
except Exception as ex:
if HIDE_WINDOWS_KNOWN_ERRORS:
from unittest import SkipTest

raise SkipTest("FIXME: fails with: PermissionError\n {}".format(ex)) from ex
raise
rmtree(str(wtd))
# END delete tree if possible
# END handle force

if not dry_run and osp.isdir(git_dir):
self._clear_cache()
try:
rmtree(git_dir)
except Exception as ex:
if HIDE_WINDOWS_KNOWN_ERRORS:
from unittest import SkipTest

raise SkipTest(f"FIXME: fails with: PermissionError\n {ex}") from ex
else:
raise
rmtree(git_dir)
# end handle separate bare repository
# END handle module deletion

Expand Down
68 changes: 42 additions & 26 deletions git/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,24 +5,24 @@
# the BSD License: https://opensource.org/license/bsd-3-clause/

from abc import abstractmethod
import os.path as osp
from .compat import is_win
import contextlib
from functools import wraps
import getpass
import logging
import os
import os.path as osp
import pathlib
import platform
import subprocess
import re
import shutil
import stat
from sys import maxsize
import subprocess
import sys
import time
from urllib.parse import urlsplit, urlunsplit
import warnings

# from git.objects.util import Traversable
from .compat import is_win

# typing ---------------------------------------------------------

Expand All @@ -42,22 +42,17 @@
Tuple,
TypeVar,
Union,
cast,
TYPE_CHECKING,
cast,
overload,
)

import pathlib

if TYPE_CHECKING:
from git.remote import Remote
from git.repo.base import Repo
from git.config import GitConfigParser, SectionConstraint
from git import Git

# from git.objects.base import IndexObject


from .types import (
Literal,
SupportsIndex,
Expand All @@ -75,7 +70,6 @@

# ---------------------------------------------------------------------


from gitdb.util import ( # NOQA @IgnorePep8
make_sha,
LockedFD, # @UnusedImport
Expand All @@ -88,7 +82,6 @@
hex_to_bin, # @UnusedImport
)


# NOTE: Some of the unused imports might be used/imported by others.
# Handle once test-cases are back up and running.
# Most of these are unused here, but are for use by git-python modules so these
Expand Down Expand Up @@ -116,14 +109,33 @@

log = logging.getLogger(__name__)

# types############################################################

def _read_env_flag(name: str, default: bool) -> bool:
try:
value = os.environ[name]
except KeyError:
return default

log.warning(
"The %s environment variable is deprecated. Its effect has never been documented and changes without warning.",
EliahKagan marked this conversation as resolved.
Show resolved Hide resolved
name,
)

adjusted_value = value.strip().lower()

if adjusted_value in {"", "0", "false", "no"}:
return False
if adjusted_value in {"1", "true", "yes"}:
return True
log.warning("%s has unrecognized value %r, treating as %r.", name, value, default)
return default


#: We need an easy way to see if Appveyor TCs start failing,
#: so the errors marked with this var are considered "acknowledged" ones, awaiting remedy,
#: till then, we wish to hide them.
HIDE_WINDOWS_KNOWN_ERRORS = is_win and os.environ.get("HIDE_WINDOWS_KNOWN_ERRORS", True)
HIDE_WINDOWS_FREEZE_ERRORS = is_win and os.environ.get("HIDE_WINDOWS_FREEZE_ERRORS", True)
HIDE_WINDOWS_KNOWN_ERRORS = is_win and _read_env_flag("HIDE_WINDOWS_KNOWN_ERRORS", True)
HIDE_WINDOWS_FREEZE_ERRORS = is_win and _read_env_flag("HIDE_WINDOWS_FREEZE_ERRORS", True)

# { Utility Methods

Expand Down Expand Up @@ -177,25 +189,29 @@ def patch_env(name: str, value: str) -> Generator[None, None, None]:


def rmtree(path: PathLike) -> None:
"""Remove the given recursively.
"""Remove the given directory tree recursively.

:note: we use shutil rmtree but adjust its behaviour to see whether files that
couldn't be deleted are read-only. Windows will not remove them in that case"""
:note: We use :func:`shutil.rmtree` but adjust its behaviour to see whether files that
couldn't be deleted are read-only. Windows will not remove them in that case."""

def onerror(func: Callable, path: PathLike, exc_info: str) -> None:
# Is the error an access error ?
def handler(function: Callable, path: PathLike, _excinfo: Any) -> None:
"""Callback for :func:`shutil.rmtree`. Works either as ``onexc`` or ``onerror``."""
# Is the error an access error?
os.chmod(path, stat.S_IWUSR)

try:
func(path) # Will scream if still not possible to delete.
except Exception as ex:
function(path)
except PermissionError as ex:
if HIDE_WINDOWS_KNOWN_ERRORS:
from unittest import SkipTest

raise SkipTest("FIXME: fails with: PermissionError\n {}".format(ex)) from ex
raise SkipTest(f"FIXME: fails with: PermissionError\n {ex}") from ex
raise

return shutil.rmtree(path, False, onerror)
if sys.version_info >= (3, 12):
shutil.rmtree(path, onexc=handler)
else:
shutil.rmtree(path, onerror=handler)


def rmfile(path: PathLike) -> None:
Expand Down Expand Up @@ -995,7 +1011,7 @@ def __init__(
self,
file_path: PathLike,
check_interval_s: float = 0.3,
max_block_time_s: int = maxsize,
max_block_time_s: int = sys.maxsize,
) -> None:
"""Configure the instance

Expand Down
1 change: 1 addition & 0 deletions test-requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@ pre-commit
pytest
pytest-cov
pytest-instafail
pytest-subtests
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

@EliahKagan EliahKagan Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

pytest-sugar
5 changes: 4 additions & 1 deletion test/test_docs.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@ def tearDown(self):

gc.collect()

# @skipIf(HIDE_WINDOWS_KNOWN_ERRORS, ## ACTUALLY skipped by `git.submodule.base#L869`.
# ACTUALLY skipped by git.util.rmtree (in local onerror function), from the last call to it via
# git.objects.submodule.base.Submodule.remove (at "handle separate bare repository"), line 1062.
#
# @skipIf(HIDE_WINDOWS_KNOWN_ERRORS,
# "FIXME: helper.wrapper fails with: PermissionError: [WinError 5] Access is denied: "
# "'C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\test_work_tree_unsupportedryfa60di\\master_repo\\.git\\objects\\pack\\pack-bc9e0787aef9f69e1591ef38ea0a6f566ec66fe3.idx") # noqa E501
@with_rw_directory
Expand Down
13 changes: 9 additions & 4 deletions test/test_submodule.py
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,10 @@ def _do_base_tests(self, rwrepo):
True,
)

# @skipIf(HIDE_WINDOWS_KNOWN_ERRORS, ## ACTUALLY skipped by `git.submodule.base#L869`.
# ACTUALLY skipped by git.util.rmtree (in local onerror function), called via
# git.objects.submodule.base.Submodule.remove at "method(mp)", line 1011.
#
# @skipIf(HIDE_WINDOWS_KNOWN_ERRORS,
# "FIXME: fails with: PermissionError: [WinError 32] The process cannot access the file because"
# "it is being used by another process: "
# "'C:\\Users\\ankostis\\AppData\\Local\\Temp\\tmp95c3z83bnon_bare_test_base_rw\\git\\ext\\gitdb\\gitdb\\ext\\smmap'") # noqa E501
Expand Down Expand Up @@ -819,9 +822,11 @@ def test_git_submodules_and_add_sm_with_new_commit(self, rwdir):
assert commit_sm.binsha == sm_too.binsha
assert sm_too.binsha != sm.binsha

# @skipIf(HIDE_WINDOWS_KNOWN_ERRORS, ## ACTUALLY skipped by `git.submodule.base#L869`.
# "FIXME: helper.wrapper fails with: PermissionError: [WinError 5] Access is denied: "
# "'C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\test_work_tree_unsupportedryfa60di\\master_repo\\.git\\objects\\pack\\pack-bc9e0787aef9f69e1591ef38ea0a6f566ec66fe3.idx") # noqa E501
@pytest.mark.xfail(
HIDE_WINDOWS_KNOWN_ERRORS,
reason='"The process cannot access the file because it is being used by another process" on call to sm.move',
raises=PermissionError,
)
@with_rw_directory
def test_git_submodule_compatibility(self, rwdir):
parent = git.Repo.init(osp.join(rwdir, "parent"))
Expand Down
Loading
Loading