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

Merge pytest-catchlog plugin #2794

Merged
merged 33 commits into from Oct 12, 2017

Conversation

Projects
None yet
5 participants
@thisch
Contributor

thisch commented Sep 22, 2017

In this PR the pytest-catchlog plugin is merged (develop branch of the
pytest-catchlog repo) into pytest-core, as
suggested in (eisensheng/pytest-catchlog#8). In the course of that I've renamed the
plugin from CatchLogPlugin to LoggingPlugin (see eisensheng/pytest-catchlog#8).

pytest-catchlog contains a backward compatibilty code for pytest-capturelog,
which I removed in this PR, since I don't think that it makes sense to merge
this code into pytest-core. Futhermore, the pytest-catchlog repo contains
performance tests
(https://github.com/eisensheng/pytest-catchlog/tree/develop/tests/perf)
which are not yet part of this PR. Do we want to integrate them? If
yes, how?

The documentation for this PR still needs to be integrated into this
repo. I'll probably copy the docu from the readme of pytest-catchlog and
adapt it a bit.

  • Add a new news fragment into the changelog folder
    • name it $issue_id.$type for example (588.bug)
    • 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;

@thisch thisch changed the title from Catchlog to Merge pytest-catchlog plugin Sep 22, 2017

@The-Compiler

This comment has been minimized.

Member

The-Compiler commented Sep 22, 2017

This should go to the features branch, not master.

(Also, check the other things in the PR template, unless you intend to wait for some discussion before finishing this)

@thisch thisch changed the base branch from master to features Sep 22, 2017

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Sep 22, 2017

thanks for bringing this forward, i'd love to participate in the finalizing discussions but im off the grid next week

please dont forget to mark provided fixtures/apis as experimental so we can more easily bring in changes that may turn necessary by feedback coming in from wider adoption

@thisch

This comment has been minimized.

Contributor

thisch commented Sep 22, 2017

please dont forget to mark provided fixtures/apis as experimental so we can more easily bring in changes that may turn necessary by feedback coming in from wider adoption

Is it documented somewhere how I can mark the features/apis as experimental?

See you in a week @RonnyPfannschmidt

@coveralls

This comment has been minimized.

coveralls commented Sep 22, 2017

Coverage Status

Coverage increased (+30.2%) to 92.595% when pulling 4a54743 on thisch:catchlog into de0d19c on pytest-dev:features.

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Sep 22, 2017

@thisch we dont yet have a formal/in code mechanism, for now please document, and lets add a note/issue for FutureWarnings

@coveralls

This comment has been minimized.

coveralls commented Sep 23, 2017

Coverage Status

Coverage increased (+29.02%) to 92.322% when pulling 1c65ea3 on thisch:catchlog into 966391c on pytest-dev:master.

@nicoddemus

Overall excellent work @thisch, thanks a lot for taking up on this.

Besides the comments I left in the code, we need to introduce documentation. I think copying the current documentation from the README is a good start.

Defaults to the root logger.
"""
if logger is None or isinstance(logger, py.builtin._basestring):

This comment has been minimized.

@nicoddemus

nicoddemus Sep 26, 2017

Member

You can use six instead of py.builtin, we are phasing out py usage where we can.

if logger is None or isinstance(logger, six.string_types):
Defaults to the root logger.
"""
if logger is None or isinstance(logger, py.builtin._basestring):

This comment has been minimized.

@nicoddemus

nicoddemus Sep 26, 2017

Member

This implementation feels a little weird to me, if someone passes something other than a str or None, it will return None as well. Seems simpler to just forward the call to getLogger and let it deal with it:

def get_logger_obj(logger=None):
    return logging.getLogger(logger)

In fact is debatable if the function is needed at all.

This comment has been minimized.

@thisch

thisch Sep 26, 2017

Contributor

I also don't like that this function. All test still run if I remove this function and use
logger = logger or getlogger(logger) instead.

log_level = get_option_ini(config, setting_name)
if not log_level:
return
if isinstance(log_level, py.builtin.text):

This comment has been minimized.

@nicoddemus

nicoddemus Sep 26, 2017

Member

You can use six here as well

def pytest_configure(config):
config.pluginmanager.register(LoggingPlugin(config), 'loggingp')

This comment has been minimized.

@nicoddemus

nicoddemus Sep 26, 2017

Member

Typo in the plugin name

This comment has been minimized.

@thisch

thisch Sep 26, 2017

Contributor

No, I suffixed the plugin name with 'p' on purpose, since the _pytest.logging module is automatically registered as a pytest plugin(?) somehow. The name 'logging' is therefore already used. Do you have a better idea than coming up with a plugin name, which does not conflict with the name of the python module?

This comment has been minimized.

@nicoddemus

nicoddemus Sep 26, 2017

Member

Ah OK, thanks for the explanation.

That's how pytest works, _pytest.logging is already a plugin, and inside that plugin you are registering another plugin. We have the same in _pytest.capture, where it installs a CaptureManager plugin as "capman".

I suggest using "logging-plugin" for consistency with LoggingPlugin.

if log_cli_level is None:
# No log_level was provided, default to WARNING
log_cli_level = logging.WARNING
self.log_cli_level = log_cli_level

This comment has been minimized.

@nicoddemus

nicoddemus Sep 26, 2017

Member

I suggest changing get_actual_log_level to receive a list of options, and it returns the first non-None. This way this entire block becomes:

self.log_cli_level = get_actual_log_level(config, 'log_cli_level', 'log_level') or logging.WARNING

The same is valid for get_option_ini.

This comment has been minimized.

@thisch

thisch Sep 26, 2017

Contributor

Nice, thx for this suggestion, this works and simplifies the code a bit.

logger = logging.getLogger(__name__)
sublogger = logging.getLogger(__name__+'.baz')
u = (lambda x: x.decode('utf-8')) if sys.version_info < (3,) else (lambda x: x)

This comment has been minimized.

@nicoddemus

nicoddemus Sep 26, 2017

Member

I don't need this, you can just use plain unicode literals (use u'bū' instead of u('bū')). This code was needed for Python <3.3 which reintroduced the u prefix for unicode literals.

This comment has been minimized.

@nicoddemus

nicoddemus Sep 26, 2017

Member

Btw just to be clear: I know you just ported the code and didn't wrote that bit. My suggestions are small improvements before merging the code. 😁

@@ -344,10 +344,7 @@ def teardown_module(function):
def test_logging_initialized_in_test(self, testdir):
p = testdir.makepyfile("""
import sys

This comment has been minimized.

@nicoddemus

nicoddemus Sep 26, 2017

Member

Hmm I fear if we will reintroduce the bug described in 2e80512. It was so long ago (issue number 8) that it probably is fine now.

If we decide this is no longer relevant, we should just remove this test then.

This comment has been minimized.

@thisch

thisch Sep 27, 2017

Contributor

Who can tell if this is still relevant? Probably the author of the fix (@hpk42)

This comment has been minimized.

@nicoddemus

nicoddemus Oct 3, 2017

Member

If nobody manifests themselves, I propose just removing the test. I don't see how to keep the test around if we introduce the logging module into the core, after all.

@thisch

This comment has been minimized.

Contributor

thisch commented Oct 12, 2017

I squashed your suggested changelog entry into the 'Add changelog ...' commit

@thisch thisch changed the title from WIP Merge pytest-catchlog plugin to Merge pytest-catchlog plugin Oct 12, 2017

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Oct 12, 2017

Oh thanks I didn't notice that! 👍

LGTM, thanks again for tackling this awesome new feature.

@thisch

This comment has been minimized.

Contributor

thisch commented Oct 12, 2017

I did this a few minutes ago.

Note that the performance tests are still not part of this PR. I don't plan on merging them into pytest, though.

@coveralls

This comment has been minimized.

coveralls commented Oct 12, 2017

Coverage Status

Coverage increased (+0.2%) to 92.664% when pulling 77c326c on thisch:catchlog into e7a4d3d on pytest-dev:features.

@coveralls

This comment has been minimized.

coveralls commented Oct 12, 2017

Coverage Status

Coverage increased (+0.1%) to 92.664% when pulling af75ca4 on thisch:catchlog into 1480aed on pytest-dev:features.

gtmanfred added a commit to gtmanfred/salt that referenced this pull request Oct 12, 2017

add catchlog to py3 dev
this is installed with py2, but not py3 for some reason

Though, we can remove it once pytest-dev/pytest#2794 is merged

@RonnyPfannschmidt RonnyPfannschmidt merged commit c750a5b into pytest-dev:features Oct 12, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

gtmanfred added a commit to gtmanfred/salt that referenced this pull request Oct 12, 2017

add catchlog to py3 dev
this is installed with py2, but not py3 for some reason

Though, we can remove it once pytest-dev/pytest#2794 is merged

gtmanfred added a commit to gtmanfred/salt that referenced this pull request Oct 17, 2017

add catchlog to py3 dev
this is installed with py2, but not py3 for some reason

Though, we can remove it once pytest-dev/pytest#2794 is merged

dis-xcom added a commit to Mirantis/tcp-qa that referenced this pull request Nov 28, 2017

Disable pytest logs capture
In pytest 3.3.0 was merged the pytest-cachlog plugin
which is now enabled by default [1]

- Disable this logging module for tcp-qa tests.

[1] pytest-dev/pytest#2794

Change-Id: I60d9ba42ac88af80d263c6cadff10cb69d6d5e2a
@The-Compiler

This comment has been minimized.

Member

The-Compiler commented Dec 5, 2017

@thisch If you have the time, can you please take a look at the remaining pytest-catchlog issues/PRs? See #3003. I also opened eisensheng/pytest-catchlog#73 to make it clear what's going on.

"""Add options to control log capturing."""
group = parser.getgroup('logging')
def add_option_ini(option, dest, default=None, type=None, **kwargs):

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Dec 5, 2017

Member

while reiterating this as it got back into my notifications i beleive this one should have never met a release,
literally all the options we add are technically superseeded by the much better option setting argument

This comment has been minimized.

@nicoddemus

nicoddemus Dec 5, 2017

Member

I see what you mean, all command-line options could be replaced by using the -o option instead.

While I agree with your reasoning, we have to remember that one of the objectives was to merge this plugin as is to allow a drop-in-replacement for users, so removing these options would break their usage.

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Dec 5, 2017

Member

we should nonetheless mark them for removal in future

renefritze added a commit to pymor/pymor that referenced this pull request Dec 6, 2017

[deps] increase py.test req. to 3.3, remove capturelog
Log capturing has been merged into py.test proper:
pytest-dev/pytest#2794

and pytest-capturelog blacklisted:
pytest-dev/pytest#3004

To avoid potential snafus I'm bumping the py.test req right away
and removing capturelog from our deps.

zenhack added a commit to zenhack/hil that referenced this pull request Feb 15, 2018

Remove pytest-catchlog from dependences.
We've been getting warnings about it having been merged into pytest
core, e.g:

    https://travis-ci.org/CCI-MOC/hil/jobs/341455006#L2725

This also bumps the minimum version for pytest in our setup.py, since
the change was made in 3.3.0:

* https://docs.pytest.org/en/latest/changelog.html#id56
* pytest-dev/pytest#2794
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment