caplog un-disable logging, feedback per review #8711

- Resolves issues from review provided by https://github.com/andrewdotn

- Fixed spelling errors

- Improved logic in `force_enable_logging`

- Make a fixture to un-disable logging after a test so that logging will be un-disabled even if the test fails due to an assertion error.
The fixture is in `test_fixture.py` because we can't `import logging` in `conftest.py` because there is a module called `logging` in the same folder.

- Mypy implicit import error. I can't commit without this.

Issue: #8711
PR: #8758

src/_pytest/config/__init__.py

@@ -48,7 +48,7 @@
 from _pytest._code import filter_traceback
 from _pytest._io import TerminalWriter
 from _pytest.compat import final
-from _pytest.compat import importlib_metadata
+from _pytest.compat import importlib_metadata  # type: ignore[attr-defined]
 from _pytest.outcomes import fail
 from _pytest.outcomes import Skipped
 from _pytest.pathlib import absolutepath

src/_pytest/logging.py

@@ -456,7 +456,7 @@ def force_enable_logging(
     ) -> int:
         """Enable the desired logging level if the level was disabled.
 
-        Only enables logging levels equal too and higher than the requested ``level``.
+        Only enables logging levels greater than or equal to the requested ``level``.
 
         Does nothing if the desired ``level`` wasn't disabled.
 
@@ -471,16 +471,17 @@ def force_enable_logging(
         original_disable_level: int = logger_obj.manager.disable  # type: ignore[attr-defined]
 
         if isinstance(level, str):
+            # Try to translate the level string to an int for `logging.disable()`
             level = logging.getLevelName(level)
 
         if not isinstance(level, int):
+            # The level provided was not valid, so just un-disable all logging.
             logging.disable(logging.NOTSET)
-            return original_disable_level
-        if logger_obj.isEnabledFor(level):
-            return original_disable_level
-
-        disable_level = max(level - 10, logging.NOTSET)
-        logging.disable(disable_level)
+        elif not logger_obj.isEnabledFor(level):
+            # Each level is `10` away from other levels.
+            # https://docs.python.org/3/library/logging.html#logging-levels
+            disable_level = max(level - 10, logging.NOTSET)
+            logging.disable(disable_level)
 
         return original_disable_level
 
@@ -491,7 +492,7 @@ def set_level(self, level: Union[int, str], logger: Optional[str] = None) -> Non
             The levels of the loggers changed by this function will be
             restored to their initial values at the end of the test.
 
-        Will enable the requesed logging level if it was disabled via :meth:`logging.disable`.
+        Will enable the requested logging level if it was disabled via :meth:`logging.disable`.
 
         :param int level: The level.
         :param str logger: The logger to update. If not given, the root logger.
@@ -515,7 +516,7 @@ def at_level(
         the end of the 'with' statement the level is restored to its original
         value.
 
-        Will enable the requesed logging level if it was disabled via :meth:`logging.disable`.
+        Will enable the requested logging level if it was disabled via :meth:`logging.disable`.
 
         :param int level: The level.
         :param str logger: The logger to update. If not given, the root logger.

testing/logging/test_fixture.py

@@ -9,6 +9,19 @@
 sublogger = logging.getLogger(__name__ + ".baz")
 
 
+@pytest.fixture
+def cleanup_disabled_logging():
+    """Simple fixture that ensures that a test doesn't disable logging.
+
+    This is necessary because ``logging.disable()`` is global, so a test disabling logging
+    and not cleaning up after will break every test that runs after it.
+
+    This behavior was moved to a fixture so that logging will be un-disabled even if the test fails an assertion.
+    """
+    yield
+    logging.disable(logging.NOTSET)
+
+
 def test_fixture_help(pytester: Pytester) -> None:
     result = pytester.runpytest("--fixtures")
     result.stdout.fnmatch_lines(["*caplog*"])
@@ -29,7 +42,7 @@ def test_change_level(caplog):
     assert "CRITICAL" in caplog.text
 
 
-def test_change_level_logging_disabled(caplog):
+def test_change_level_logging_disabled(caplog, cleanup_disabled_logging):
     logging.disable(logging.CRITICAL)
     assert logging.root.manager.disable == logging.CRITICAL
     caplog.set_level(logging.WARNING)
@@ -45,9 +58,6 @@ def test_change_level_logging_disabled(caplog):
     assert "SUB_WARNING" not in caplog.text
     assert "SUB_CRITICAL" in caplog.text
 
-    # logging.disable needs to be reset because it's global and causes future tests will break.
-    logging.disable(logging.NOTSET)
-
 
 def test_change_level_undo(pytester: Pytester) -> None:
     """Ensure that 'set_level' is undone after the end of the test.
@@ -75,7 +85,9 @@ def test2(caplog):
     result.stdout.no_fnmatch_line("*log from test2*")
 
 
-def test_change_disabled_level_undo(pytester: Pytester) -> None:
+def test_change_disabled_level_undo(
+    pytester: Pytester, cleanup_disabled_logging
+) -> None:
     """Ensure that 'force_enable_logging' in 'set_level' is undone after the end of the test.
 
     Tests the logging output themselves (affected by disabled logging level).
@@ -103,9 +115,6 @@ def test2(caplog):
     result.stdout.fnmatch_lines(["*log from test1*", "*2 failed in *"])
     result.stdout.no_fnmatch_line("*log from test2*")
 
-    # logging.disable needs to be reset because it's global and causes future tests will break.
-    logging.disable(logging.NOTSET)
-
 
 def test_change_level_undos_handler_level(pytester: Pytester) -> None:
     """Ensure that 'set_level' is undone after the end of the test (handler).
@@ -150,7 +159,7 @@ def test_with_statement(caplog):
     assert "CRITICAL" in caplog.text
 
 
-def test_with_statement_logging_disabled(caplog):
+def test_with_statement_logging_disabled(caplog, cleanup_disabled_logging):
     logging.disable(logging.CRITICAL)
     assert logging.root.manager.disable == logging.CRITICAL
     with caplog.at_level(logging.WARNING):
@@ -175,9 +184,6 @@ def test_with_statement_logging_disabled(caplog):
     assert "SUB_CRITICAL" in caplog.text
     assert logging.root.manager.disable == logging.CRITICAL
 
-    # logging.disable needs to be reset because it's global and causes future tests will break.
-    logging.disable(logging.NOTSET)
-
 
 def test_log_access(caplog):
     caplog.set_level(logging.INFO)