GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-40275: Avoid importing logging in test.support by serhiy-storchaka · Pull Request #19601 · python/cpython

Conversation

@serhiy-storchaka

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Import logging lazily in assertLogs() in unittest.
  • Move TestHandler from test.support to test_logging.

https://bugs.python.org/issue40275

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@serhiy-storchaka
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 serhiy-storchaka added the skip news label Apr 19, 2020
@serhiy-storchaka
serhiy-storchaka requested a review from vsajip as a code owner April 19, 2020 11:23
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Apr 19, 2020
@bedevere-bot bedevere-bot added the awaiting core review label Apr 19, 2020
@tirkarthi tirkarthi mentioned this pull request Apr 19, 2020
[WIP] bpo-40275: Add loggingutils in test.support #19599
Closed
@serhiy-storchaka

serhiy-storchaka commented Apr 19, 2020

Copy link
Copy Markdown
Member Author

It depends on #19600 because asyncio imports logging.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
shihai1991
shihai1991 reviewed Apr 19, 2020
View reviewed changes
Comment thread Lib/test/test_logging.py Outdated
])


class TestHandler(logging.handlers.BufferingHandler):

@shihai1991 shihai1991 Apr 19, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
vsajip
vsajip requested changes Apr 20, 2020
View reviewed changes

@vsajip vsajip left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting core review labels Apr 20, 2020
@bedevere-bot

bedevere-bot commented Apr 20, 2020

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka
Move TestHandler to logging_helper.
3f1fc37
@serhiy-storchaka

serhiy-storchaka commented Apr 20, 2020

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Apr 20, 2020
View reviewed changes
Comment thread Lib/unittest/case.py Outdated
logger = self.logger = logging.getLogger(self.logger_name)
formatter = logging.Formatter(self.LOGGING_FORMAT)

class _CapturingHandler(logging.Handler):

@vstinner vstinner Apr 20, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@serhiy-storchaka
Import logging and create _CapturingHandler only once.
5d59d23
@serhiy-storchaka
serhiy-storchaka force-pushed the test-support-logging branch from f9b43f8 to 5d59d23 Compare April 20, 2020 16:10
vstinner
vstinner reviewed Apr 20, 2020
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/unittest/case.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/support/logging_helper.py
@@ -0,0 +1,29 @@
import logging.handlers

class TestHandler(logging.handlers.BufferingHandler):

@vstinner vstinner Apr 20, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@serhiy-storchaka serhiy-storchaka Apr 20, 2020

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@serhiy-storchaka
Move _AssertLogsContext in a private unittest._log submodule.
58ae645
@vstinner

vstinner commented Apr 20, 2020

Copy link
Copy Markdown
Member

@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Apr 20, 2020
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/unittest/_log.py Outdated
self._raiseFailure(
"no logs of level {} or higher triggered on {}"
.format(logging.getLevelName(self.level), self.logger.name))

@vstinner vstinner Apr 20, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please remove empty lines at the end.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
serhiy-storchaka added 3 commits April 25, 2020 10:27
@serhiy-storchaka
Merge branch 'master' into test-support-logging
7144f73
@serhiy-storchaka
Remove empty lines at the end.
3464c9a
@serhiy-storchaka
Fix merging error.
01ba090
@serhiy-storchaka
serhiy-storchaka merged commit 515fce4 into python:master Apr 25, 2020
@bedevere-bot bedevere-bot removed the awaiting changes label Apr 25, 2020
@serhiy-storchaka
serhiy-storchaka deleted the test-support-logging branch April 25, 2020 08:35
CuriousLearner added a commit to CuriousLearner/cpython that referenced this pull request Apr 27, 2020
@CuriousLearner
Merge branch 'master' of github.com:python/cpython into fix-issue21914
42c39a3 
* '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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments
@shihai1991 shihai1991 shihai1991 left review comments
@vsajip vsajip vsajip requested changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@serhiy-storchaka @bedevere-bot @vstinner @vsajip @shihai1991 @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.