bpo-40275: Avoid importing logging in test.support #19601

serhiy-storchaka · 2020-04-19T11:23:33Z

Import logging lazily in assertLogs() in unittest.
Move TestHandler from test.support to test_logging.

https://bugs.python.org/issue40275

* Import logging lazily in assertLogs() in unittest. * Move TestHandler from test.support to test_logging.

serhiy-storchaka · 2020-04-19T13:29:52Z

It depends on #19600 because asyncio imports logging.

shihai1991 · 2020-04-19T13:34:32Z

Lib/test/test_logging.py

@@ -3477,6 +3477,35 @@ def test_logrecord_class(self):
        ])


+class TestHandler(logging.handlers.BufferingHandler):


Hm, I am not sure it's only test_logging use this common class forever.

vsajip

The whole purpose of placing TestHandler in test.support was that it could be used/useful in other tests. I understand if test.support needs to be subdivided further into subpackages, e.g. test.support.logging to hold this kind of code, but I don't think it's appropriate to move it into test_logging (even if it's the only current user of TestHandler) because it then wouldn't be obvious that it was intended for use across any tests that wanted to use logging.

I also see the microbenchmarks on the issue. but what is the practical impact on the time taken for test runs? How much does the total time for a test run reduce as a result of refactoring test.support to minimise imports?

bedevere-bot · 2020-04-20T05:43:41Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

serhiy-storchaka · 2020-04-20T09:54:42Z

TestHandler was added 9.5 years ago, and it is still only used in a single test in a single file. I am not sure that it will be used in other tests in the next 10 years. But if you prefer, I'll move it into a test.support submodule.

The goal of this change is not to save the import time, but to minimize possible side effects of importing the logging package. It is relatively complex package which imports 34 modules. Some of them can have side effects which can affect the particular test.

vstinner · 2020-04-20T13:13:54Z

Lib/unittest/case.py

        if isinstance(self.logger_name, logging.Logger):
            logger = self.logger = self.logger_name
        else:
            logger = self.logger = logging.getLogger(self.logger_name)
        formatter = logging.Formatter(self.LOGGING_FORMAT)
+
+        class _CapturingHandler(logging.Handler):


I dislike defining a new _CapturingHandler class each time _AssertLogsContext context manager is used :-( Maybe a lazy import could be used, something similar to PR #19600?

vstinner

Maybe moving the logging changes into a separated PR since test_logging/test.support changes and unittest changes are not directly related.

Lib/unittest/case.py

vstinner · 2020-04-20T16:22:32Z

Lib/test/support/logging_helper.py

@@ -0,0 +1,29 @@
+import logging.handlers
+
+class TestHandler(logging.handlers.BufferingHandler):


It's only used in test_logging. Why not simply moving this class into test_logging?

We don't provide any backward compatibility warranty for test.support. It should not be used outside CPython code base.

It is what I did originally. But then move it into a separate submodule on the request of @vsajip (logging maintainer, who added this class 10 years ago).

vstinner · 2020-04-20T21:35:11Z

@vsajip: "The whole purpose of placing TestHandler in test.support was that it could be used/useful in other tests"

The thing is that 10 years later, it's only used by test_logging. If tomorrow, another test wants to use it, support.logging_helper could be added. But I don't think that it's worth it. I would prefer to move TestHandler in test_logging to be able to remove "import logging" from test.support.

@vsajip: "I also see the microbenchmarks on the issue. but what is the practical impact on the time taken for test runs?"

See https://bugs.python.org/issue40275#msg366823

vstinner

Since there is a disagreement on test.support, maybe keep this PR for the unittest module, and write a new one for test.support changes?

vstinner · 2020-04-20T21:36:08Z

Lib/unittest/_log.py

+            self._raiseFailure(
+                "no logs of level {} or higher triggered on {}"
+                .format(logging.getLevelName(self.level), self.logger.name))
+


Please remove empty lines at the end.

* 'master' of github.com:python/cpython: (2949 commits) Add files in tests/test_peg_generator to the install target lists (pythonGH-19723) bpo-40398: Fix typing.get_args() for special generic aliases. (pythonGH-19720) bpo-40348: Fix typos in the programming FAQ (pythonGH-19729) bpo-38387: Formally document PyDoc_STRVAR and PyDoc_STR macros (pythonGH-16607) bpo-40401: Remove duplicate pyhash.h include from pythoncore.vcxproj (pythonGH-19725) bpo-40387: Improve queue join() example. (pythonGH-19724) bpo-40396: Support GenericAlias in the typing functions. (pythonGH-19718) Fix typo in Lib/typing.py (pythonGH-19717) Fix typo in object.__format__ docs (pythonGH-19504) bpo-40275: Avoid importing logging in test.support (pythonGH-19601) bpo-40275: Avoid importing socket in test.support (pythonGH-19603) bpo-40275: Avoid importing asyncio in test.support (pythonGH-19600) bpo-40279: Add some error-handling to the module initialisation docs example (pythonGH-19705) closes bpo-40385: Remove Tools/scripts/checkpyc.py (pythonGH-19709) bpo-40334: Add What's New sections for PEP 617 and PEP 585 (pythonGH-19704) bpo-40340: Separate examples more clearly in the programming FAQ (pythonGH-19688) bpo-40360: Deprecate lib2to3 module in light of PEP 617 (pythonGH-19663) bpo-40334: Rewrite test_c_parser to avoid memory leaks (pythonGH-19694) bpo-38061: subprocess uses closefrom() on FreeBSD (pythonGH-19697) bpo-38061: os.closerange() uses closefrom() on FreeBSD (pythonGH-19696) ...

bpo-40275: Avoid importing logging in test.support

739599e

* Import logging lazily in assertLogs() in unittest. * Move TestHandler from test.support to test_logging.

serhiy-storchaka added the skip news label Apr 19, 2020

serhiy-storchaka requested a review from vsajip as a code owner April 19, 2020 11:23

the-knights-who-say-ni added the CLA signed label Apr 19, 2020

bedevere-bot added the awaiting core review label Apr 19, 2020

tirkarthi mentioned this pull request Apr 19, 2020

[WIP] bpo-40275: Add loggingutils in test.support #19599

Closed

shihai1991 reviewed Apr 19, 2020

View reviewed changes

vsajip requested changes Apr 20, 2020

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Apr 20, 2020

Move TestHandler to logging_helper.

3f1fc37

vstinner reviewed Apr 20, 2020

View reviewed changes

Import logging and create _CapturingHandler only once.

5d59d23

serhiy-storchaka force-pushed the test-support-logging branch from f9b43f8 to 5d59d23 Compare April 20, 2020 16:10

vstinner reviewed Apr 20, 2020

View reviewed changes

Move _AssertLogsContext in a private unittest._log submodule.

58ae645

vstinner reviewed Apr 20, 2020

View reviewed changes

serhiy-storchaka added 3 commits April 25, 2020 10:27

Merge branch 'master' into test-support-logging

7144f73

Remove empty lines at the end.

3464c9a

Fix merging error.

01ba090

serhiy-storchaka merged commit 515fce4 into python:master Apr 25, 2020

bedevere-bot removed the awaiting changes label Apr 25, 2020

serhiy-storchaka deleted the test-support-logging branch April 25, 2020 08:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-40275: Avoid importing logging in test.support #19601

bpo-40275: Avoid importing logging in test.support #19601

serhiy-storchaka commented Apr 19, 2020 •

edited by bedevere-bot

Loading

serhiy-storchaka commented Apr 19, 2020

shihai1991 Apr 19, 2020

vsajip left a comment •

edited

Loading

bedevere-bot commented Apr 20, 2020

serhiy-storchaka commented Apr 20, 2020

vstinner Apr 20, 2020

vstinner left a comment

vstinner Apr 20, 2020

serhiy-storchaka Apr 20, 2020

vstinner commented Apr 20, 2020

vstinner left a comment

vstinner Apr 20, 2020

		@@ -3477,6 +3477,35 @@ def test_logrecord_class(self):
		])


		class TestHandler(logging.handlers.BufferingHandler):

		@@ -0,0 +1,29 @@
		import logging.handlers

		class TestHandler(logging.handlers.BufferingHandler):

bpo-40275: Avoid importing logging in test.support #19601

bpo-40275: Avoid importing logging in test.support #19601

Conversation

serhiy-storchaka commented Apr 19, 2020 • edited by bedevere-bot Loading

serhiy-storchaka commented Apr 19, 2020

shihai1991 Apr 19, 2020

Choose a reason for hiding this comment

vsajip left a comment • edited Loading

Choose a reason for hiding this comment

bedevere-bot commented Apr 20, 2020

serhiy-storchaka commented Apr 20, 2020

vstinner Apr 20, 2020

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

vstinner Apr 20, 2020

Choose a reason for hiding this comment

serhiy-storchaka Apr 20, 2020

Choose a reason for hiding this comment

vstinner commented Apr 20, 2020

vstinner left a comment

Choose a reason for hiding this comment

vstinner Apr 20, 2020

Choose a reason for hiding this comment

serhiy-storchaka commented Apr 19, 2020 •

edited by bedevere-bot

Loading

vsajip left a comment •

edited

Loading