[MRG]Fix pytest message that silently passes no matter if it should #12001

wdevazelhes · 2018-09-04T12:53:54Z

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 the with fails. This can make the tests pass even if they should fail because the error message was the wrong one.

wdevazelhes · 2018-09-04T12:56:17Z

sklearn/model_selection/tests/test_search.py

        ParameterGrid(input)
+    assert str(raised_error.value) == error_message


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

Maybe use match here?

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

wdevazelhes · 2018-09-04T12:56:43Z

sklearn/utils/tests/test_validation.py

@@ -182,7 +182,7 @@ def test_check_array_force_all_finiteinvalid(value, force_all_finite,
                                             match_msg, retype):
    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):


Here we only test that part of the message matches

wdevazelhes · 2018-09-04T12:57:53Z

sklearn/utils/tests/test_validation.py

@@ -172,7 +172,7 @@ def test_check_array_force_all_finite_valid(value, force_all_finite, retype):
     (np.inf, 'allow-nan', 'Input contains infinity'),
     (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')]


Here changing the test with match instead of message revealed that it was in fact failing so here is the fix

rth · 2018-09-04T13:07:14Z

Which version of pytest are you using? pytest.raises(SomeError, message='some message') is indeed ignored on older pytest versions, but it should work with reasonably recent ones.

Once we figure out the minimum pytest version, we should add it to the docs...

wdevazelhes · 2018-09-04T13:14:27Z

I use pytest 3.7.4

wdevazelhes · 2018-09-04T13:31:46Z

After looking into the docs, I think that the message argument is not ignored in my version of pytest, but it is just that what it does is to raise a "custom error message" if the statement in the with fails (and this statement only checks the type of the error)
It does not check that the raised error message matches the expected error message, hence the test silently passing

But I also saw that indeed as you said this argument (and I think the match argument too) were introduced in recent versions of pytest and ignored before, so I can look at the version of introduction and do a PR to include it in the docs indeed

rth · 2018-09-04T13:33:08Z

Yes, I was confusing message and match arguments. In this PR, it definitely makes sense to replace message with match, as this is a bug.

wdevazelhes · 2018-09-04T13:59:46Z

Regarding the silent ignore of arguments in previous versions, according to this changelog, pytest.warns(match=) was introduced in v 3.3.0, pytest.raises(match=) in v 3.1.0, and pytest.raises(message=) in v 3.0.0 (I didn't see pytest.warns(message=))
so should I add pytest>=v3.3.0 here and/or here and/or here ?
In a separate PR ?

rth

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

rth · 2018-09-04T15:10:16Z

sklearn/model_selection/tests/test_search.py

        ParameterGrid(input)
+    assert str(raised_error.value) == error_message


Maybe use match here?

wdevazelhes · 2018-09-04T15:36:27Z

should I add pytest>=v3.3.0 here and/or here and/or here ?
In a separate PR ?

+1 for a separate PR

I just created the PR here: #12002

jnothman · 2018-09-04T21:36:35Z

Thanks @wdevazelhes. Will keep a look out for these in the future.

…12001) Previously these assertions would pass without matching.

FIX: fix pytest message that silently passes no matter if it should

b644b9b

wdevazelhes commented Sep 4, 2018

View reviewed changes

rth approved these changes Sep 4, 2018

View reviewed changes

MAINT: use match instead of string equality assertion

5891dbc

wdevazelhes mentioned this pull request Sep 4, 2018

[MRG] DOC: Add pytest version in documentation #12002

Merged

jnothman approved these changes Sep 4, 2018

View reviewed changes

jnothman merged commit a041821 into scikit-learn:master Sep 4, 2018

wdevazelhes deleted the fix/fix_pytest_message branch September 5, 2018 07:00

jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Sep 6, 2018

TST FIX use match rather than message in pytest.raises (scikit-learn#…

d2f5652

…12001) Previously these assertions would pass without matching.

rth mentioned this pull request Sep 13, 2018

Confusion between message and match parameters in pytest.raises pytest-dev/pytest#3974

Closed

jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Sep 17, 2018

TST FIX use match rather than message in pytest.raises (scikit-learn#…

3909395

…12001) Previously these assertions would pass without matching.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[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 #12001

Uh oh!

wdevazelhes commented Sep 4, 2018

Uh oh!

wdevazelhes Sep 4, 2018

Uh oh!

rth Sep 4, 2018

Uh oh!

wdevazelhes Sep 4, 2018

Uh oh!

wdevazelhes Sep 4, 2018

Uh oh!

wdevazelhes Sep 4, 2018

Uh oh!

rth commented Sep 4, 2018 •

edited

Loading

Uh oh!

wdevazelhes commented Sep 4, 2018

Uh oh!

wdevazelhes commented Sep 4, 2018

Uh oh!

rth commented Sep 4, 2018

Uh oh!

wdevazelhes commented Sep 4, 2018

Uh oh!

rth left a comment

Uh oh!

rth Sep 4, 2018

Uh oh!

wdevazelhes commented Sep 4, 2018

Uh oh!

jnothman commented Sep 4, 2018

Uh oh!

Uh oh!

		ParameterGrid(input)
		assert str(raised_error.value) == error_message

Uh oh!

[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 #12001

Uh oh!

Conversation

wdevazelhes commented Sep 4, 2018

What does this implement/fix? Explain your changes.

Uh oh!

wdevazelhes Sep 4, 2018

Choose a reason for hiding this comment

Uh oh!

rth Sep 4, 2018

Choose a reason for hiding this comment

Uh oh!

wdevazelhes Sep 4, 2018

Choose a reason for hiding this comment

Uh oh!

wdevazelhes Sep 4, 2018

Choose a reason for hiding this comment

Uh oh!

wdevazelhes Sep 4, 2018

Choose a reason for hiding this comment

Uh oh!

rth commented Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wdevazelhes commented Sep 4, 2018

Uh oh!

wdevazelhes commented Sep 4, 2018

Uh oh!

rth commented Sep 4, 2018

Uh oh!

wdevazelhes commented Sep 4, 2018

Uh oh!

rth left a comment

Choose a reason for hiding this comment

Uh oh!

rth Sep 4, 2018

Choose a reason for hiding this comment

Uh oh!

wdevazelhes commented Sep 4, 2018

Uh oh!

jnothman commented Sep 4, 2018

Uh oh!

Uh oh!

rth commented Sep 4, 2018 •

edited

Loading