Skip to content
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
Merged

Conversation

twmr
Copy link
Contributor

@twmr twmr 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;

@twmr twmr changed the title Catchlog Merge pytest-catchlog plugin Sep 22, 2017
@The-Compiler
Copy link
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)

@twmr twmr changed the base branch from master to features September 22, 2017 20:29
@RonnyPfannschmidt
Copy link
Member

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

@twmr
Copy link
Contributor Author

twmr 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
Copy link

Coverage Status

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

@RonnyPfannschmidt
Copy link
Member

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

@twmr
Copy link
Contributor Author

twmr commented Oct 12, 2017

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

@twmr twmr changed the title WIP Merge pytest-catchlog plugin Merge pytest-catchlog plugin Oct 12, 2017
@nicoddemus
Copy link
Member

Oh thanks I didn't notice that! 👍

LGTM, thanks again for tackling this awesome new feature.

@twmr
Copy link
Contributor Author

twmr 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
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@RonnyPfannschmidt RonnyPfannschmidt merged commit c750a5b into pytest-dev:features Oct 12, 2017
gtmanfred added a commit to gtmanfred/salt that referenced this pull request Oct 17, 2017
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
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
Copy link
Member

@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):
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

we should nonetheless mark them for removal in future

renefritze added a commit to pymor/pymor that referenced this pull request Dec 6, 2017
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
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants