[MRG] Use 'with pytest.warns' blocks to assert warnings. by kdexd · Pull Request #465 · joblib/joblib

kdexd · 2016-12-19T10:55:09Z

Fourth Phase PR on #411 ( Succeeding PR #464 )

This PR uses with pytest.warns blocks (initially planned recwarn fixture of pytest) to record raised warnings in a particular test and assert warning messages, instead of with warning.catch_warnings blocks.

Reference: http://doc.pytest.org/en/latest/recwarn.html

kdexd · 2016-12-19T11:15:03Z

@lesteve Please have a round of reviews 😄

kdexd · 2016-12-19T11:23:23Z

joblib/test/test_memory.py

    memory = Memory(cachedir=tmpdir.strpath, verbose=0)
    # For isolation with other tests
-    memory.clear()
+    memory.clear(warn=False)


This gives a DeprecationWarning which isn't ignored for Python >= 3. So the assertion below failed certain jobs on CI. I hope this is fine.

lesteve · 2016-12-19T13:26:53Z

Actually thinking about it, I think pytest.warns looks better than recwarn. It is closer to what we do for exception with pytest.raises.

Now I am wondering though, is there a clear advantage using pytest.warns compared towarnings.catch_warnings?

kdexd · 2016-12-19T13:30:06Z

@lesteve I am not sure about the prospects of avoiding a ridiculous error of warnings.catch_warnings with a particular pytest version (like you caught one earlier with assert_raises of unittest xD )..

The only advantage here, again are less noisy error logs upon failure..

lesteve · 2016-12-19T13:38:25Z

The only advantage here, again are less noisy error logs upon failure..

Do you mind posting tracebacks to compare?

kdexd · 2016-12-19T14:48:30Z

Code snippet:

def test_false_warning_through_warnings_module():
    with warnings.catch_warnings(UserWarning) as catched:
        warnings.warn('runtime!', RuntimeWarning)
    assert catched[0].category == UserWarning

def test_false_warning_through_pytest():
    with pytest.warns(UserWarning):
        warnings.warn('runtime!', RuntimeWarning)
        a = 'foo'

def test_false_warning_through_recwarn(recwarn):
    warnings.warn('runtime!', RuntimeWarning)
    w = recwarn.pop(UserWarning)

def test_no_warning_through_warnings_module2():
    with warnings.catch_warnings(UserWarning) as catched:
        a = 'foo'

    # no 'length of catched' comparison just to see the log
    assert catched[0].category == UserWarning


def test_no_warning_through_pytest2(recwarn):
    a = 'foo'

    # no 'length of recwarn' comparison just to see the log
    w = recwarn.pop(UserWarning)

Error Log:

===== test session starts =====
platform linux2 -- Python 2.7.12, pytest-3.0.5, py-1.4.31, pluggy-0.4.0
rootdir: /home/kd, inifile: 
plugins: cov-2.4.0
collected 5 items 

test_foo.py F.FFF

===== FAILURES =====
_____ test_false_warning_through_warnings_module ______

    def test_false_warning_through_warnings_module():
        with warnings.catch_warnings(UserWarning) as catched:
            warnings.warn('runtime!', RuntimeWarning)
    
        assert len(catched) == 1
        w = catched[0]
>       assert w.category == UserWarning
E       assert <type 'exceptions.RuntimeWarning'> == UserWarning
E        +  where <type 'exceptions.RuntimeWarning'> = <warnings.WarningMessage object at 0x7f82ea595090>.category

test_foo.py:11: AssertionError
_____ test_false_warning_through_recwarn ______

recwarn = <_pytest.recwarn.WarningsRecorder object at 0x7f82ea5a2350>

    def test_false_warning_through_recwarn(recwarn):
        warnings.warn('runtime!', RuntimeWarning)
>       w = recwarn.pop(UserWarning)
E       AssertionError: <type 'exceptions.UserWarning'> not found in warning list

test_foo.py:24: AssertionError
----- Captured stderr call -----
/home/kd/test_foo.py:23: RuntimeWarning: runtime!
  warnings.warn('runtime!', RuntimeWarning)
_____ test_no_warning_through_warnings_module2 _____

    def test_no_warning_through_warnings_module2():
        with warnings.catch_warnings(UserWarning) as catched:
            a = 'foo'
    
        # no 'length of catched' comparison just to see the log
>       assert catched[0].category == UserWarning
E       IndexError: list index out of range

test_foo.py:32: IndexError
_____ test_no_warning_through_pytest2 _____

recwarn = <_pytest.recwarn.WarningsRecorder object at 0x7f82ea595a10>

    def test_no_warning_through_pytest2(recwarn):
        a = 'foo'
    
        # no 'length of recwarn' comparison just to see the log
>       w = recwarn.pop(UserWarning)
E       AssertionError: <type 'exceptions.UserWarning'> not found in warning list

test_foo.py:39: AssertionError
===== 4 failed, 1 passed in 0.02 seconds =====

Looks like with block is safest to use, since len(catched) == 1 might be true if we would capture any other warning than desired.

lesteve · 2016-12-19T15:21:26Z

I think using with pytest.warns looks good, since you can restrict the warnings check to the block you want.

Having said that I think the error you get when you are trying to catch the wrong type of warnings, i.e. "Failed: DID NOT WARN", is not great so as a best practice always do name the WarningChecker and check something about it:

from joblib.testing import warns

with warns(UserWarning) as wc:
    # the code that should warn
assert len(wc.list) == 1

The message will still be not great, but at least with a debugger you will be able to spot which kind of warnings have been emitted.

Completely unrelated, try your snippet with Python 3 and you'll realise it does not quite do what you thought (catch_warnings does not take a warning type argument). If you don't have a particularly good reason to stick with Python 2, I would strongly recommend to use Python 3 as your day-to-day Python.

kdexd · 2016-12-19T15:45:51Z

Completely unrelated, try your snippet with Python 3 and you'll realise it does not quite do what you thought (catch_warnings does not take a warning type argument).

Yes, indeed, that was something new for me..

If you don't have a particularly good reason to stick with Python 2, I would strongly recommend to use Python 3 as your day-to-day Python.

Well I don't have a specific reason but I didn't think of the transition since the time I started. I guess this change is long overdue. 😅 Thanks for the guidance, I'm changing now !

kdexd · 2016-12-19T16:08:42Z

Also, I ensured that:

def test_foo():
    with pytest.warns(UserWarning) as warninfo:
        warnings.warn("user warning", UserWarning)
        warnings.warn("runtime warning", RuntimeWarning)
    assert len(warninfo.list) == 1    # fails

So pytest looks for a specific warning though records all of them..

lesteve · 2016-12-19T17:12:52Z

So pytest looks for a specific warning though records all of them..

Yeah it makes sure there is at least one warning of the right type, that's why I would be in favour of checking length + message always.

What is not so good I find is that it could have a better message where no warnings of the right type is found, saying which other kind of warnings were seen.

kdexd · 2016-12-19T17:19:40Z

@lesteve: I think that's a nice topic for an enhancement issue in pytest. Maybe we should bring this in notice of nicoddemus.

kdexd · 2016-12-20T03:15:25Z

@lesteve I have used the best out of two ways at different places. In some tests, the number of warnings can either be 0 or 1 depending upon the condition. Using recwarn keeps the code comparatively more straightforward, while at most of the places, with warns() block fit in very nicely.

lesteve

A few comments. The main one is to use with pytest.warns(None) for cases where you may have no warnings rather than recwarn.

lesteve · 2016-12-20T06:13:17Z

joblib/test/test_memory.py

-    with warnings.catch_warnings(record=True) as w:
-        # Cause all warnings to always be triggered.
-        warnings.simplefilter("always")
+    with warns(JobLibCollisionWarning) as warninfo:


The pytest use record, I would be in favour of using the same naming everywhere unless you have seen warninfo used a lot in your experience with pytest.

I haven't neither seen elsewhere, nor have myself followed any strict convention with this name particularly. So I guess pytest's standard name record is just fine to use.

I changed completely my mind on this one, warninfo is a nice parallel to excinfo and is more specific than record. So I pushed some changes to revert back to the original state, sorry about that.

lesteve · 2016-12-20T06:13:54Z

joblib/test/test_memory.py

-        assert len(w) == 1
-        assert "collision" in str(w[-1].message)
+    assert len(warninfo) == 1
+    assert "collision" in str(warninfo[-1].message)


Use warninfo[0]. It's the same since there is only one warning but it's less surprising.

[-1] suggests the most recent warning, but yes - [0] is less surprising..

lesteve · 2016-12-20T06:15:24Z

joblib/test/test_numpy_pickle.py

-                    'be ignored.' % {'filename': this_filename,
-                                     'mmap_mode': 'r+'})
+    assert len(warninfo) == 1
+    assert (str(warninfo[-1].message) ==


Use [0] same reason as above

lesteve · 2016-12-20T06:17:34Z

joblib/test/test_numpy_pickle.py

-                        "used 'cache_size={0}'".format(cache_size))
+        warnings.simplefilter("always")
+        numpy_pickle.dump(a, filename, cache_size=cache_size)
+        expected_nb_warnings = 1 if cache_size is not None else 0


You can use with warns(None) as record as the doc says for this use case instead of using recwarn

lesteve · 2016-12-20T06:18:20Z

joblib/test/test_numpy_pickle.py

+            filename_base = os.path.basename(filename)
+            expected_nb_warnings = 1 if ("_0.9" in filename_base or
+                                         "_0.8.4" in filename_base) else 0
+            assert len(recwarn) == expected_nb_warnings


Same comment about using with warns(None) as record for this case.

lesteve · 2016-12-20T06:19:06Z

joblib/test/test_numpy_pickle.py

-                        '"%(mmap_mode)s" flag will be ignored.'
-                        % {'fileobj': f, 'mmap_mode': 'r+'})
+        assert len(warninfo) == 1
+        assert (str(warninfo[-1].message) ==


[0] instead of [-1]

lesteve · 2016-12-20T06:21:15Z

joblib/test/test_parallel.py

    # started from the non-main thread. Let's check that there is no false
    # positive because of the name change.
-    assert caught_warnings == []
+    assert len(recwarn) == 0


I would use with warns(None) as record and then check len(record) == 0

lesteve · 2016-12-20T06:27:45Z

Also before I forget I would be interested to see whether we could add check for the filename of the warnings. It is very easy to get stacklevel wrong in warnings so that the warnings show a different file that does not make sense.

kdexd · 2016-12-20T06:38:16Z

Also before I forget I would be interested to see whether we could add check for the filename of the warnings.

If you haven't seen this yet, I'm quoting it straight from the documentation:

Each recorded warning has the attributes message, category, filename, lineno, file, and line. The category is the class of the warning. The message is the warning itself; calling str(message) will return the actual message of the warning.

Task for a separate PR ?

lesteve · 2016-12-20T07:19:35Z

If you haven't seen this yet, I'm quoting it straight from the documentation:

Yeah I saw that, that is why I mentioned it. Different PR if you want indeed.

lesteve · 2016-12-20T15:38:58Z

joblib/test/test_numpy_pickle.py

-                        "used 'cache_size={0}'".format(cache_size))
+        expected_nb_warnings = 1 if cache_size is not None else 0
+        assert len(warninfo) == expected_nb_warnings
+        for w in warninfo:


This loop over 0 or 1 element is a bit misleading. Oh well I'll tackle that myself.

lesteve · 2016-12-20T15:45:41Z

LGTM, I'll merge this when all the CIs come back green. Thanks a lot!

kdexd · 2016-12-20T15:51:16Z

@lesteve That's right ! So finally after this PR, the last thing left is to go through the undone modules left out during third phase and exercise parametrization (only if the code doesn't look too twisted).

lesteve · 2016-12-20T18:23:37Z

Merged, thanks!

There is also #455 than you can do in the same PR as the parametrize PR if you feel it is more convenient.

Karan Desai added 3 commits December 19, 2016 15:36

Use recwarn fixture in test_memory.py

7086822

Use recwarn fixture in test_numpy_pickle.py

8a27c13

Use recwarn fixture in test_parallel.py

c7cd65f

Fix failure under python>=3 in test_memory_warnings_lambda_collisions.

0a54bbb

kdexd force-pushed the use-recwarn-everywhere branch from cf818f9 to 0a54bbb Compare December 19, 2016 11:21

kdexd commented Dec 19, 2016

View reviewed changes

Replace some recwarns by pytest.warns blocks.

e05034e

lesteve reviewed Dec 20, 2016

View reviewed changes

Replace all recwarns by pytest.warns blocks.

6643215

kdexd changed the title ~~[MRG] Use pytest's recwarn fixture to assert warnings.~~ [MRG] Use 'with pytest.warns' blocks to assert warnings. Dec 20, 2016

kdexd force-pushed the use-recwarn-everywhere branch from 9234f19 to d257fd0 Compare December 20, 2016 09:24

Rename all warninfo variables to record.

a7ea74c

kdexd force-pushed the use-recwarn-everywhere branch from d257fd0 to a7ea74c Compare December 20, 2016 09:46

Replace record by warninfo

4b7bafe

lesteve reviewed Dec 20, 2016

View reviewed changes

lesteve force-pushed the use-recwarn-everywhere branch from 05d6611 to 4b7bafe Compare December 20, 2016 15:45

lesteve merged commit 2275925 into joblib:master Dec 20, 2016

kdexd deleted the use-recwarn-everywhere branch December 21, 2016 02:47

This was referenced Dec 21, 2016

Move to py.test? #411

Closed

[MRG] Refactor test_memory.py as per pytest design. #466

Merged

Conversation

kdexd commented Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fourth Phase PR on #411 ( Succeeding PR #464 )

Reference: http://doc.pytest.org/en/latest/recwarn.html

Uh oh!

kdexd commented Dec 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lesteve commented Dec 19, 2016

Uh oh!

kdexd commented Dec 19, 2016

Uh oh!

lesteve commented Dec 19, 2016

Uh oh!

kdexd commented Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lesteve commented Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kdexd commented Dec 19, 2016

Uh oh!

kdexd commented Dec 19, 2016

Uh oh!

lesteve commented Dec 19, 2016

Uh oh!

kdexd commented Dec 19, 2016

Uh oh!

kdexd commented Dec 20, 2016

Uh oh!

lesteve left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lesteve commented Dec 20, 2016

Uh oh!

kdexd commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lesteve commented Dec 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lesteve commented Dec 20, 2016

Uh oh!

kdexd commented Dec 20, 2016

Uh oh!

lesteve commented Dec 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kdexd commented Dec 19, 2016 •

edited

Loading

kdexd commented Dec 19, 2016 •

edited

Loading

lesteve commented Dec 19, 2016 •

edited

Loading

kdexd commented Dec 20, 2016 •

edited

Loading