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.

Author: EliahKagan

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

test/deprecation/test_basic.py

@@ -15,16 +15,15 @@
 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
 from git.objects.util import Traversable
 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

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,15 +122,15 @@ 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
         # during the yield. (The outer try-finally catches exceptions in this fixture.)
         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()