[MRG]Fix pytest message that silently passes no matter if it should#12001
[MRG]Fix pytest message that silently passes no matter if it should#12001jnothman merged 2 commits intoscikit-learn:masterfrom
Conversation
| with pytest.raises(error_type, message=error_message): | ||
| with pytest.raises(error_type) as raised_error: | ||
| ParameterGrid(input) | ||
| assert str(raised_error.value) == error_message |
There was a problem hiding this comment.
here we want to test the exact message, and the message contains characters that can be parsed in regexp (like (), so I thought this was the way to go
There was a problem hiding this comment.
I didn't use it to avoid putting backslashes in front of parenthesis, and to be more restrictive (exact match vs partial match), but I guess indeed it is simpler with match, and match seems more used in the rest of scikit-learn
| X = retype(np.arange(4).reshape(2, 2).astype(np.float)) | ||
| X[0, 0] = value | ||
| with pytest.raises(ValueError, message=match_msg): | ||
| with pytest.raises(ValueError, match=match_msg): |
There was a problem hiding this comment.
Here we only test that part of the message matches
| (np.nan, True, 'Input contains NaN, infinity'), | ||
| (np.nan, 'allow-inf', 'force_all_finite should be a bool or "allow-nan"'), | ||
| (np.nan, 1, 'force_all_finite should be a bool or "allow-nan"')] | ||
| (np.nan, 1, 'Input contains NaN, infinity')] |
There was a problem hiding this comment.
Here changing the test with match instead of message revealed that it was in fact failing so here is the fix
|
Which version of pytest are you using? Once we figure out the minimum pytest version, we should add it to the docs... |
|
I use pytest 3.7.4 |
|
After looking into the docs, I think that the But I also saw that indeed as you said this argument (and I think the |
|
Yes, I was confusing |
|
Regarding the silent ignore of arguments in previous versions, according to this changelog, |
rth
left a comment
rth
left a comment
There was a problem hiding this comment.
Minor comment below, otherwise LGTM, thanks!
should I add pytest>=v3.3.0 here and/or here and/or here ?
In a separate PR ?
+1 for a separate PR
| with pytest.raises(error_type, message=error_message): | ||
| with pytest.raises(error_type) as raised_error: | ||
| ParameterGrid(input) | ||
| assert str(raised_error.value) == error_message |
I just created the PR here: #12002 |
|
Thanks @wdevazelhes. Will keep a look out for these in the future. |
…12001) Previously these assertions would pass without matching.
…12001) Previously these assertions would pass without matching.
3 participants
What does this implement/fix? Explain your changes.
I found that there were two tests using
with pytest.raises(SomeError, message='some message'), which does not check that the error raised is equal to 'some message', but returns 'some message' in case the statement inside thewithfails. This can make the tests pass even if they should fail because the error message was the wrong one.