clear root log handlers between tests

[r0fls]

Addresses #2158 which qualifies as a bug to me. Hence target should be master. The implementation is not ideal: it clears module level logging settings, whereas ideally those would be preserved. However, I think it's better than the current behavior, where the module settings overlap between different tests.

[RonnyPfannschmidt]

Evon your proposed change is not something we can release in a bugfix release

I suspect this can only be done correct in a full logging integration

[nicoddemus]

@r0fls first of all thanks for taking the time to write this PR.

Since pytest doesn't currently handle logging at all, I suggest to use the pytest-catchlog plugin, which I suspect handles the peculiarities of the logging module during testing correctly.

To me it feels weird to bake into pytest a partial handling of the the logging module, without adding for example logging capture per test like pytest-catchlog does. One could argue that we should integrate pytest-catchlog into pytest but we maintainers are leaning to keep the core small, with plugins feeling the gaps when possible.

If pytest-catchlog solves this issue I would rather decline this PR, or at most start a new proposal to a complete logging integration into the core like @RonnyPfannschmidt mentioned.

[r0fls]

Thanks @RonnyPfannschmidt and @nicoddemus for the feedback, and explaining your stance. Yes, this was more of an attempt to get a discussion going and demonstrate the issue, so I understand this isn't suitable to merge. If this change could be implemented at the module/file level (so, only clear the root logger between separate test files), then I would be far more adamant about it being a good idea. However I could not find a straightforward way to do that. So, I agree we can close this since catchlog does have support for this using their caplog argument:

def test_foo(caplog):
    caplog.set_level(logging.CRITICAL, logger='root.baz')
    pass

Whether or not to add full blown logging support I would see as a separate question, yet I also agree it may not be necessary with such nice plugins. I wasn't aware of those, thank you. I was shocked when logging settings were shared between separate test files. So I do think this is a pretty annoying bug. Once discovered, it's definitely easy to handle from a user's perspective. For the first 10-20 minutes of test agony though... it was a bit perplexing. Since pytest is really the one at fault here, it does feel like we should do something to prevent users from the confusion that I encountered. However if the fix is substantial, I wouldn't prioritize it. In that case I do think it would be worth adding a description of the issue here, just for the record: http://docs.pytest.org/en/latest/faq.html

[nicoddemus]

If this change could be implemented at the module/file level (so, only clear the root logger between separate test files), then I would be far more adamant about it being a good idea. However I could not find a straightforward way to do that. So, I agree we can close this since catchlog does have support for this using their caplog argument

I think pytest-catchlog already handles clearing the logs between the tests even when not using the caplog fixture, but if for some reason one doesn't want to install the plugin it is easy to implement it in a conftest.py file:

@pytest.hookimpl(hookwrapper=True)
def pytest_runtest_teardown(self, item):
    yield
    for handler in logging.root.handlers[:]:
        logging.root.removeHandler(handler)

In that case I do think we should add a description of the issue here though, just for the record: http://docs.pytest.org/en/latest/faq.html

Definitely. Documentation is one of the areas we really need to improve and help is welcome. In fact, once eisensheng/pytest-catchlog#19 is solved I'm thinking it might be a good idea to add an explicit page to the official docs named "Logging" which explains that pytest doesn't handle logging in any way and users which use logging should checkout the pytest-logging plugin. Having a specific page for logging would help bring awareness to the plugin and the fact that pytest doesn't handle logging, avoiding frustration and wasted time.

[nicoddemus]

Given the context above, are you OK with closing this PR @r0fls?

[r0fls]

Yup! Thank you

[tatango-pmaj]

It would be worth to add here that nowadays pytest-catchlog is merged to core, so for ppl just finding this conversation, it is not worth reading any more :)