From cf2576e406006ec28bcc85565a7ef85864cbd39e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 29 Mar 2024 23:15:52 -0400 Subject: [PATCH] Make/use test.deprecation.lib; abandon idea to filter by module This creates a module test.deprecation.lib in the test suite for a couple general helpers used in deprecation tests, one of which is now used in two of the test modules and the other of which happens only to be used in one but is concepually related to the first. The assert_no_deprecation_warning context manager function fixes two different flawed approaches to that, which were in use earlier: - In test_basic, only DeprecationWarning and not the less significant PendingDeprecationWarning had been covere, which it should, at least for symmetry, since pytest.deprecated_call() treats it like DeprecationWarning. There was also a comment expressing a plan to make it filter only for warnings originating from GitPython, which was a flawed idea, as detailed below. - In test_cmd_git, the flawed idea of attempting to filter only for warnings originating from GitPython was implemented. The problem with this is that it is heavily affected by stacklevel and its interaction with the pattern of calls through which the warning arises: warnings could actually emanate from code in GitPython's git module, but be registered as having come from test code, a callback in gitdb, smmap, or the standard library, or even the pytest test runner. Some of these are more likely than others, but all are possible especially considering the possibility of a bug in the value passed to warning.warn as stacklevel. (It may be valuable for such bugs to cause tests to fail, but they should not cause otherwise failing tests to wrongly pass.) It is probably feasible to implement such filtering successfully, but I don't think it's worthwhile for the small reduction in likelihood of failing later on an unrealted DeprecationWarning from somewhere else, especially considering that GitPython's main dependencies are so minimal. --- test/deprecation/lib.py | 27 +++++++++++++++++++++++++++ test/deprecation/test_basic.py | 26 ++++++++------------------ test/deprecation/test_cmd_git.py | 24 +++++------------------- 3 files changed, 40 insertions(+), 37 deletions(-) create mode 100644 test/deprecation/lib.py diff --git a/test/deprecation/lib.py b/test/deprecation/lib.py new file mode 100644 index 000000000..9fe623a3a --- /dev/null +++ b/test/deprecation/lib.py @@ -0,0 +1,27 @@ +# This module is part of GitPython and is released under the +# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ + +"""Support library for deprecation tests.""" + +__all__ = ["assert_no_deprecation_warning", "suppress_deprecation_warning"] + +import contextlib +import warnings + +from typing import Generator + + +@contextlib.contextmanager +def assert_no_deprecation_warning() -> Generator[None, None, None]: + """Context manager to assert that code does not issue any deprecation warnings.""" + with warnings.catch_warnings(): + warnings.simplefilter("error", DeprecationWarning) + warnings.simplefilter("error", PendingDeprecationWarning) + yield + + +@contextlib.contextmanager +def suppress_deprecation_warning() -> Generator[None, None, None]: + with warnings.catch_warnings(): + warnings.simplefilter("ignore", DeprecationWarning) + yield diff --git a/test/deprecation/test_basic.py b/test/deprecation/test_basic.py index 8ee7e72b1..6235a836c 100644 --- a/test/deprecation/test_basic.py +++ b/test/deprecation/test_basic.py @@ -15,9 +15,6 @@ It is inapplicable to the deprecations whose warnings are tested in this module. """ -import contextlib -import warnings - import pytest from git.diff import NULL_TREE @@ -25,6 +22,8 @@ from git.repo import Repo from git.util import Iterable as _Iterable, IterableObj +from .lib import assert_no_deprecation_warning + # typing ----------------------------------------------------------------- from typing import Generator, TYPE_CHECKING @@ -38,15 +37,6 @@ # ------------------------------------------------------------------------ -@contextlib.contextmanager -def _assert_no_deprecation_warning() -> Generator[None, None, None]: - """Context manager to assert that code does not issue any deprecation warnings.""" - with warnings.catch_warnings(): - # FIXME: Refine this to filter for deprecation warnings from GitPython. - warnings.simplefilter("error", DeprecationWarning) - yield - - @pytest.fixture def commit(tmp_path: "Path") -> Generator["Commit", None, None]: """Fixture to supply a one-commit repo's commit, enough for deprecation tests.""" @@ -72,7 +62,7 @@ def test_diff_renamed_warns(diff: "Diff") -> None: def test_diff_renamed_file_does_not_warn(diff: "Diff") -> None: """The preferred Diff.renamed_file property issues no deprecation warning.""" - with _assert_no_deprecation_warning(): + with assert_no_deprecation_warning(): diff.renamed_file @@ -84,13 +74,13 @@ def test_commit_trailers_warns(commit: "Commit") -> None: def test_commit_trailers_list_does_not_warn(commit: "Commit") -> None: """The nondeprecated Commit.trailers_list property issues no deprecation warning.""" - with _assert_no_deprecation_warning(): + with assert_no_deprecation_warning(): commit.trailers_list def test_commit_trailers_dict_does_not_warn(commit: "Commit") -> None: """The nondeprecated Commit.trailers_dict property issues no deprecation warning.""" - with _assert_no_deprecation_warning(): + with assert_no_deprecation_warning(): commit.trailers_dict @@ -102,7 +92,7 @@ def test_traverse_list_traverse_in_base_class_warns(commit: "Commit") -> None: def test_traversable_list_traverse_override_does_not_warn(commit: "Commit") -> None: """Calling list_traverse on concrete subclasses is not deprecated, does not warn.""" - with _assert_no_deprecation_warning(): + with assert_no_deprecation_warning(): commit.list_traverse() @@ -114,7 +104,7 @@ def test_traverse_traverse_in_base_class_warns(commit: "Commit") -> None: def test_traverse_traverse_override_does_not_warn(commit: "Commit") -> None: """Calling traverse on concrete subclasses is not deprecated, does not warn.""" - with _assert_no_deprecation_warning(): + with assert_no_deprecation_warning(): commit.traverse() @@ -128,7 +118,7 @@ class Derived(_Iterable): def test_iterable_obj_inheriting_does_not_warn() -> None: """Subclassing git.util.IterableObj is not deprecated, does not warn.""" - with _assert_no_deprecation_warning(): + with assert_no_deprecation_warning(): class Derived(IterableObj): pass diff --git a/test/deprecation/test_cmd_git.py b/test/deprecation/test_cmd_git.py index 3f87037a0..6c592454a 100644 --- a/test/deprecation/test_cmd_git.py +++ b/test/deprecation/test_cmd_git.py @@ -56,12 +56,10 @@ metaclass marginal enough, to justify introducing a metaclass to issue the warnings. """ -import contextlib import logging import sys from typing import Generator import unittest.mock -import warnings if sys.version_info >= (3, 11): from typing import assert_type @@ -73,6 +71,8 @@ from git.cmd import Git +from .lib import assert_no_deprecation_warning, suppress_deprecation_warning + _USE_SHELL_DEPRECATED_FRAGMENT = "Git.USE_SHELL is deprecated" """Text contained in all USE_SHELL deprecation warnings, and starting most of them.""" @@ -82,13 +82,6 @@ _logger = logging.getLogger(__name__) -@contextlib.contextmanager -def _suppress_deprecation_warning() -> Generator[None, None, None]: - with warnings.catch_warnings(): - warnings.simplefilter("ignore", DeprecationWarning) - yield - - @pytest.fixture def restore_use_shell_state() -> Generator[None, None, None]: """Fixture to attempt to restore state associated with the USE_SHELL attribute. @@ -129,7 +122,7 @@ def restore_use_shell_state() -> Generator[None, None, None]: # Try to save the original public value. Rather than attempt to restore a state # where the attribute is not set, if we cannot do this we allow AttributeError # to propagate out of the fixture, erroring the test case before its code runs. - with _suppress_deprecation_warning(): + with suppress_deprecation_warning(): old_public_value = Git.USE_SHELL # This doesn't have its own try-finally because pytest catches exceptions raised @@ -137,7 +130,7 @@ def restore_use_shell_state() -> Generator[None, None, None]: yield # Try to restore the original public value. - with _suppress_deprecation_warning(): + with suppress_deprecation_warning(): Git.USE_SHELL = old_public_value finally: # Try to restore the original private state. @@ -323,14 +316,7 @@ def test_execute_without_shell_arg_does_not_warn() -> None: USE_SHELL is to be used, the way Git.execute itself accesses USE_SHELL does not issue a deprecation warning. """ - with warnings.catch_warnings(): - for category in DeprecationWarning, PendingDeprecationWarning: - warnings.filterwarnings( - action="error", - category=category, - module=r"git(?:\.|$)", - ) - + with assert_no_deprecation_warning(): Git().version()