Skip to content

[MRG] warning class should be passed to ignore_warnings as the category parameter.#11594

Closed
lesteve wants to merge 1 commit intoscikit-learn:masterfrom
lesteve:fix-ignore-warnings-category
Closed

[MRG] warning class should be passed to ignore_warnings as the category parameter.#11594
lesteve wants to merge 1 commit intoscikit-learn:masterfrom
lesteve:fix-ignore-warnings-category

Conversation

@lesteve
Copy link
Copy Markdown
Member

@lesteve lesteve commented Jul 17, 2018

I found this one while commenting in #11580 (comment):

This is a slightly dangerous caveat of ignore_warnings: if you pass the warning class as the first argument, then the test is not run:

from sklearn.utils.testing import ignore_warnings

@ignore_warnings(UserWarning)
def test():
    1/0
❯ pytest /tmp/test.py
======================================================== test session starts ========================================================
platform linux -- Python 3.6.5, pytest-3.6.0, py-1.5.3, pluggy-0.6.0
rootdir: /tmp, inifile:
plugins: cov-2.5.1, hypothesis-3.56.0
collected 0 items                                                                                                                   

=================================================== no tests ran in 0.14 seconds ====================================================

The reason is that the first argument is the test callable. Maybe that could be fixed not sure.

@lesteve
TST warning class should be passed as the category parameter.
dbce1b2 
The first argument is the test callable. Doing it like this cause the test
function to not be called
@lesteve lesteve added this to the 0.20 milestone Jul 17, 2018
@rth
Copy link
Copy Markdown
Member

rth commented Jul 17, 2018

Wow, thanks for catching this!

Maybe add,

if isinstance(obj, Warn):
   raise ValueError('obj=%s should not be a warning type'
                    % obj)

to ignore_warnings to prevent this in the future. Otherwise LGTM.

@rth rth mentioned this pull request Jul 17, 2018
Merged
@massich
Copy link
Copy Markdown
Contributor

massich commented Jul 17, 2018

isn't this tested?

@rth
Copy link
Copy Markdown
Member

rth commented Jul 17, 2018

.. and a test, it doesn't seem to be tested @massich as far as I can tell

@lesteve
Copy link
Copy Markdown
Member Author

lesteve commented Jul 17, 2018 •
edited
Loading

I opened #11599 to tackle this.

@lesteve
Copy link
Copy Markdown
Member Author

lesteve commented Jul 17, 2018

Closing in favour of #11599.

@lesteve lesteve closed this Jul 17, 2018
@lesteve lesteve deleted the fix-ignore-warnings-category branch February 9, 2021 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Blocker Bug

Projects

None yet

Milestone

0.20

Development

Successfully merging this pull request may close these issues.

3 participants

@lesteve @rth @massich