Skip to content

Lower the root logger's log level to --log-level instead of to NOTSET #3027

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

segevfiner
Copy link
Contributor

logging is a seriously confusing module, I'm not really sure that's the right way to fix this, so review with care.

Fixes #2977

Checklist

Thanks for submitting a PR, your contribution is really appreciated!

Here's a quick checklist that should be present in PRs:

  • Add a new news fragment into the changelog folder
    • name it $issue_id.$type for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of removal, feature, bugfix, vendor, doc or trivial
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."
  • Target: for bugfix, vendor, doc or trivial fixes, target master; for removals or features target features;
  • Make sure to include reasonable tests for your change if necessary

Unless your change is a trivial or a documentation fix (e.g., a typo or reword of a small section) please:

  • Add yourself to AUTHORS, in alphabetical order;

@nicoddemus
Copy link
Member

logging is a seriously confusing module

I agree. 😓

It seems the tests are now failing... @segevfiner if you have the time can you perhaps take a look?

@@ -242,6 +242,8 @@ def __init__(self, config):
The formatter can be safely shared across all handlers so
create a single one for the entire test session here.
"""
self.log_level = get_actual_log_level(
config, 'log_level') or logging.WARNING
Copy link
Member

Choose a reason for hiding this comment

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

We might consider to use None as default; then if None, we don't touch the log level at all all.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that if you pass None to logger.setLevel or handler.setLevel a TypeError is raised.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, but actually I meant to check for None before calling setLevel.

@segevfiner
Copy link
Contributor Author

The failing tests (There might be other failures too) show why the root logger was probably set to logging.NOTSET in the first place. The way, I think, logging works when you write a message to a logger:

  1. The logger finds it's log level. If the logger's level is NOTSET it goes upwards in the logger hierarchy looking for the closest logger that has a level that is different than NOTSET.
  2. If the message's level is lower than the log level it doesn't get passed to any handler at all. This means that by setting the root logger and LogCaptureHandler to level WARNING, you get no logs lower than that level captured.
  3. If the message is not dropped by the level, each handler is called, where it's own set log level get's another chance to filter the message. Handlers will be called up the hierarchy if the logger's propogate field is True.
    (And this is ignoring some other nasty mechanisms such as log disabling and log filters...)

I guess that means that simply not setting the root logger's log level won't be enough since I think the module is currently expected to catch all logs by default.

That's what I get for underestimating logging... I guess this PR can be closed... 😩

@segevfiner segevfiner closed this Dec 13, 2017
@segevfiner segevfiner deleted the root-logger-level branch December 13, 2017 23:02
@nicoddemus
Copy link
Member

Thanks anyway @segevfiner, this certaily helped to shed some more light in the subtle details involved. We continue the discussion about logging in general in #3013.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pytest 3.3: root logger has wrong logging level
3 participants