-
Notifications
You must be signed in to change notification settings - Fork 409
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
[MRG] Use 'with pytest.warns' blocks to assert warnings. #465
Conversation
@lesteve Please have a round of reviews 😄 |
cf818f9
to
0a54bbb
Compare
# Check that multiple use of lambda will raise collisions | ||
memory = Memory(cachedir=tmpdir.strpath, verbose=0) | ||
# For isolation with other tests | ||
memory.clear() | ||
memory.clear(warn=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Actually thinking about it, I think Now I am wondering though, is there a clear advantage using |
@lesteve I am not sure about the prospects of avoiding a ridiculous error of The only advantage here, again are less noisy error logs upon failure.. |
Do you mind posting tracebacks to compare? |
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:
Looks like |
I think using 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. |
Yes, indeed, that was something new for me..
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 ! |
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.. |
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. |
@lesteve: I think that's a nice topic for an enhancement issue in pytest. Maybe we should bring this in notice of nicoddemus. |
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments. The main one is to use with pytest.warns(None)
for cases where you may have no warnings rather than recwarn
.
with warnings.catch_warnings(record=True) as w: | ||
# Cause all warnings to always be triggered. | ||
warnings.simplefilter("always") | ||
with warns(JobLibCollisionWarning) as warninfo: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
assert len(w) == 1 | ||
assert "collision" in str(w[-1].message) | ||
assert len(warninfo) == 1 | ||
assert "collision" in str(warninfo[-1].message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use warninfo[0]
. It's the same since there is only one warning but it's less surprising.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[-1]
suggests the most recent warning, but yes - [0]
is less surprising..
'be ignored.' % {'filename': this_filename, | ||
'mmap_mode': 'r+'}) | ||
assert len(warninfo) == 1 | ||
assert (str(warninfo[-1].message) == |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use [0]
same reason as above
"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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use with warns(None) as record
as the doc says for this use case instead of using recwarn
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about using with warns(None) as record
for this case.
'"%(mmap_mode)s" flag will be ignored.' | ||
% {'fileobj': f, 'mmap_mode': 'r+'}) | ||
assert len(warninfo) == 1 | ||
assert (str(warninfo[-1].message) == |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[0]
instead of [-1]
# The multiprocessing backend will raise a warning when detecting that is | ||
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use with warns(None) as record
and then check len(record) == 0
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 |
If you haven't seen this yet, I'm quoting it straight from the documentation:
Task for a separate PR ? |
Yeah I saw that, that is why I mentioned it. Different PR if you want indeed. |
9234f19
to
d257fd0
Compare
d257fd0
to
a7ea74c
Compare
"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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loop over 0 or 1 element is a bit misleading. Oh well I'll tackle that myself.
05d6611
to
4b7bafe
Compare
LGTM, I'll merge this when all the CIs come back green. Thanks a lot! |
@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). |
Merged, thanks! There is also #455 than you can do in the same PR as the |
Fourth Phase PR on #411 ( Succeeding PR #464 )
This PR uses
with pytest.warns
blocks (initially plannedrecwarn
fixture of pytest) to record raised warnings in a particular test and assert warning messages, instead ofwith warning.catch_warnings
blocks.Reference: http://doc.pytest.org/en/latest/recwarn.html