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

showing pytest warnings summary by default. #1672

Closed
wants to merge 3 commits into from

Conversation

aostr
Copy link

@aostr aostr commented Jun 25, 2016

Hi,

Did the following

  • added ``--disable-pytest-warnings` flag to let users disable the warnings summary.
  • extended/changed unit tests for the changes in the pytest core.

Additionally, I decided to add myself to the authors.

* added ``--disable-pytest-warnings` flag to let users disable the warnings summary.
* extended/changed unit tests for the changes in the pytest core.
@coveralls
Copy link

coveralls commented Jun 25, 2016

Coverage Status

Coverage decreased (-0.007%) to 92.279% when pulling e04d9ff on aostr:master into c519b95 on pytest-dev:master.

@@ -36,6 +36,10 @@
Thanks `@bagerard`_ for reporting (`#1503`_). Thanks to `@davehunt`_ and
`@tomviner`_ for PR.

* Whitelisted pytest warnings to show up warnings summary by default. Added a new
Copy link
Member

Choose a reason for hiding this comment

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

"Whitelisted pytest warnings" seems confusing - it sounds like some warnings are on a whitelist.

Copy link
Author

Choose a reason for hiding this comment

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

I will rephrase this once I get to a place with a stable internet connection :)

@The-Compiler
Copy link
Member

With the goal of keeping commandline options low you mentioned in #1668, what's the reason of using a quite specific --disable-pytest-warnings instead of having a more generic -R which does the inverse of -r, so you could do -Rw to hide warnings, but also e.g. -Rs to hide skipped tests if someone did -rs in their pytest.ini but you really don't want to see them?

@aostr
Copy link
Author

aostr commented Jun 26, 2016

I don't have a preference on the commandline options here. I was talking to Holger about this, also about inverse options and the conclusion was that adding the --disable-pytest-warnings is what the discussion ended up with. Again, I can't really tell which one is better.
I think that I would chose the --disable-pytest-warnings here because it requires a little bit more typing to disable the warnings. It could push people to update the code as opposed to just typing -Rw. If they are just as lazy as I am :)

@coveralls
Copy link

coveralls commented Jun 26, 2016

Coverage Status

Coverage decreased (-0.007%) to 92.279% when pulling b4e0fab on aostr:master into c519b95 on pytest-dev:master.

@nicoddemus
Copy link
Member

nicoddemus commented Jun 26, 2016

This should be rebased on features.

@aostr could you please rebase into features, implement the requested changes and open a new PR? Unfortunately GH does not support changing the target branch of PRs.

Also, please add a line "Fix #1668" in a commit so that issue gets closed automatically when merged. 😁

@nicoddemus nicoddemus closed this Jun 26, 2016
@@ -20,10 +20,15 @@ def pytest_addoption(parser):
group._addoption('-q', '--quiet', action="count",
dest="quiet", default=0, help="decrease verbosity."),
group._addoption('-r',
action="store", dest="reportchars", default=None, metavar="chars",
action="store", dest="reportchars", default='', metavar="chars",
help="show extra test summary info as specified by chars (f)ailed, "
"(E)error, (s)skipped, (x)failed, (X)passed (w)pytest-warnings "
Copy link
Member

Choose a reason for hiding this comment

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

Nice, but also I think we should remove (w)pytest-warnings from the list as it no longer has an effect.

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.

4 participants