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

Should pytest.warns check for issubclass rather than class equality? #2151

Closed
lesteve opened this issue Dec 20, 2016 · 5 comments
Closed

Should pytest.warns check for issubclass rather than class equality? #2151

lesteve opened this issue Dec 20, 2016 · 5 comments

Comments

@lesteve
Copy link
Contributor

lesteve commented Dec 20, 2016

pytest.warns check for class equality and not subclass relationship. This snippet for example fails:

import warnings

import pytest


with pytest.warns(Warning):
    warnings.warn('user', UserWarning)

This was slightly surprising to me since my expectation was that it would be similar to pytest.raises where you can check for a base class:

import pytest


with pytest.raises(Exception):
    raise ValueError()

What do you guys think? After all you can always use with pytest.warns(None) as warninfo and check issubclass(warninfo[0].message, a_warning_class) if you want to check a subclass relationship. At the same time, I don't really how it would hurt to check subclass relationship.

I guess that is just a matter of changing this line by:

if not any(issubclass(r.category, each) for each in self.expected_warning for r in self):

I am willing to open a PR about this if you think it is worth it.

@lesteve lesteve changed the title Should pytest.warns check for issubclass rather than class equality Should pytest.warns check for issubclass rather than class equality? Dec 20, 2016
@decentral1se
Copy link
Contributor

Documentation reads:

You can check that code raises a particular warning using pytest.warns,
which works in a similar manner to raises

It isn't really clear how 'similar' this works since your example shows a clear difference.

Not sure if the functionality should change (seems so, though?) but if it doesn't, then the documentation should be made clearer.

@lesteve
Copy link
Contributor Author

lesteve commented Dec 20, 2016

Not sure if the functionality should change (seems so, though?)

I am kind of leaning this way too. Two additional data points outside of nose:

stdlib warning filters

import warnings

warnings.simplefilter('error', Warning)
warnings.warn('user', UserWarning)  # warning turned into error

nose assert_warnings

import nose

with nose.tools.assert_warnings(Warning):
    warnings.warn('user', UserWarning)  # pass because UserWarning is a subclass of Warning

@blueyed
Copy link
Contributor

blueyed commented Dec 25, 2016

@lesteve
I agree also.
Please go ahead and create a PR.
While it could be seen as a bugfix, I think the features branch is better, since it changes API/behavior.

@nicoddemus
Copy link
Member

stdlib warning filters
nose assert_warnings

Since the other examples do match subclasses, I think it kind makes sense as well (specially with the similarity with pytest.raises).

While it could be seen as a bugfix, I think the features branch is better, since it changes API/behavior.

Agreed!

@lesteve
Copy link
Contributor Author

lesteve commented Jan 9, 2017

Closed by #2166.

@lesteve lesteve closed this as completed Jan 9, 2017
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

No branches or pull requests

4 participants