Confusion between message and match parameters in pytest.raises

[rth]

After analyzing several code bases, it appears there is frequent confusion by users between the match and message parameters in pytest.raises (and consequentially pytest.warns).

In most cases, users probably would want match but use message by accident, because is it visually similar and the naming does not differentiate their functionality enough. As a result the written test produces false positive which is quite dangerous.

For instance, here are instances of such errors in popular open-source projects,

• TST Replace unittest.TestCase.fail calls, use pytest.raises instead of legacy constructs pandas-dev/pandas#22681 (comment)
• TST Use assert_raises "match" parameter instead of the "message" parameter. scipy/scipy#9264
• [MRG]Fix pytest message that silently passes no matter if it should scikit-learn/scikit-learn#12001

the fact that these cases ended up in the code despite fairly thorough review is telling.

There was also an earlier issue illustrating this confusion in #3350

Searching on github for "pytest.raises(ValueError, message=" for instance, also shows a number of examples that meant to use "match" instead of "message" (though sometimes it's hard to tell).

One possibility could be to deprecate the message parameter and rename it to something more explicit. I saw there were some deprecations related to the use of node.warn in 3.8, not sure if that's related. However, following the same logic even if pytest.raises(ValueError('expected message')) worked it would still not address all the existing code that currently uses the message parameter incorrectly.

---

EDIT by @nicoddemus

The message parameter has been deprecated and will be removed in pytest 5.0, please see the docs for an alternative which provides the same functionality:

https://docs.pytest.org/en/latest/deprecations.html#raises-message-deprecated

[RonnyPfannschmidt]

after checking we should deprecate the message name and rename it to something like failure_message

[rth]

Thanks for the feedback @RonnyPfannschmidt !

Naming is tricky: at some level even pytest.raises(ValueEror, failure_message='some message') could be interpreted as the expected failure message of that ValueError. Something like show_failure_mesage might be bit more explicit, but it's probably too long and not very practical..

[RonnyPfannschmidt]

@rth yes - the hardness of naming this the bane here ^^

[asottile]

big +1 to this, we have an internal linter that looks for pytest.raises(...message= and just bans it -- maybe we should consider removing the parameter altogether

[nicoddemus]

Hi @rth thanks for the excellent description and examples, they are extremely helpful in making the case about doing something about this matter.

So we have two proposals:

1. Deprecate message and use another name. I think using a longer name like show_failure_message is perfectly fine, my gut feeling is that this parameter is rarely used (only misused). This option to me is safest.

2. Removing the parameter altogether (after deprecating it of course). This would make it match (heh) pytest.warn signature as well. This is an excellent option in the long term if there are no good use-cases for it.

I'm writing an e-mail to the mailing list to ask opinions to this, in particular if there are good cases for message/show_failure_message that I'm not visualizing.

[nicoddemus]

Message posted to the mailing list: https://mail.python.org/pipermail/pytest-dev/2018-September/004599.html

[nicoddemus]

While reading our Backward Compatibility Policy:

... while giving you enough time to adjust your tests or raise concerns if there are valid reasons to keep deprecated functionality around.

I realized that option 2) can use a warning like this:

with pytest.raises(RuntimeError, message="some regex"):
    ...

   did you mean to use `match="some regex"` to check the exception message? the "message" parameter is scheduled for removal, please join https://github.com/pytest-dev/pytest/issues/3974 if you have concerns.

The feature keeps working, people can chime in with valid use cases, and we don't introduce a new parameter to keep around forever. Hmm.

[flub]

I'm tentatively agreeing with removing it fully to match pytest.warn(). But we'll probably upset some people by doing this.

From a consistency point of view we'd want the message parameter, if it is to stay, to also match pytest.fail() and pytest.skip() I think however.

[nicoddemus]

Hi @flub,

One of the options would be to deprecate the parameter first, without introducing an alternative, and see if there's feedback (#3974 (comment)). If people speak up, we can proceed with keeping it deprecated but we can introduce it as a new name to avoid the problems presented here. What do you think of that?

[flub]

seems reasonable

…

On Sat, 15 Sep 2018 at 22:38, Bruno Oliveira ***@***.***> wrote: Hi @flub <https://github.com/flub>, One of the options would be to deprecate the parameter first, without introducing an alternative, and see if there's feedback (#3974 (comment) <#3974 (comment)>). If people speak up, we can proceed with keeping it deprecated but we can introduce it as a new name to avoid the problems presented here. What do you think of that? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3974 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAI9IE28EwLa9PsYzdO_FeAfGycmIr98ks5ubWUkgaJpZM4Wm5hN> .

[WolfgangFellger]

You asked, so… I have used this in some cases with asyncio.TimeoutError / CancelledError, which by themselves are about as nondescript as can be.

[RonnyPfannschmidt]

@WolfgangFellger but the message parameter is NEVER checked against a exception, its an alternate failure message

[WolfgangFellger]

Yes. The default "DID NOT RAISE: TimeoutError" is not very helpful as a failure message. (Obviously, having to type three lines more is not the end of the world.)

[RonnyPfannschmidt]

@WolfgangFellger oh i see, thanks for that note - i beelive we should bring it back in some way then

i would propose introducing a message_when_not_raised parameter and having the old name work but triggering a deprecation warning

CC @nicoddemus

[nicoddemus]

@WolfgangFellger thanks for the feedback. Out of curiosity, could you please post a simple snippet showcasing your example?

message_when_not_raised might be a good candidate indeed.

[asottile]

if (and big if) we bring it back it should be keyword-only, we can use this pattern to enforce this in python2.x as well:

def f(**kwargs):
    msg = kwargs.pop('message_if_raised', ...)
    if kwargs:
        raise TypeError('Unexpected arguments to {}: {}'.format(f.__name__, ', '.join(sorted(kwargs))))

    ...

[nicoddemus]

Actually we already have that documented in the "deprecation" docs, but I have added links to it in the main documentation (#5218). Thanks!

[mverleg]

I feel like, if the name of the argument was the problem, the obvious solution is renaming it. It seems there were multiple people using this and wanting to continue using this. Not really sure how pytest improves by insisting on workarounds.

[nicoddemus]

I feel like, if the name of the argument was the problem, the obvious solution is renaming it.

That's one approach, definitely. But, contrary to what you say, it seems this option is used sparingly, and much more misused than used correctly (this from my own experience: in our huge codebase at work, the message parameter was being used intentionally in only a single test out of 30k+ tests, while being used incorrectly in more than 50 tests).

The people who have replied here at least seemed content to use the alternative approach.

Not really sure how pytest improves by insisting on workarounds.

Not really a workaround, just a different way to obtain the same result, IMHO.

This improves pytest by removing from its API a confusing and rarely used option, which makes it simpler to use and reduces our maintenance, even if by a little.

[asottile]

I feel like, if the name of the argument was the problem, the obvious solution is renaming it. It seems there were multiple people using this and wanting to continue using this.

there were few using it correctly and many accidentally using it incorrectly -- a common pitfall. The name of the argument was not the only issue here, but also the inflexibility of the api and complexity of the framework.

Not really sure how pytest improves by insisting on workarounds.

This isn't a workaround, we're removing a common pitfall, simplifying our interface, and encouraging a more readable and more flexible replacement.

[mverleg]

I don't dispute that there were a lot of mistaken usages. But it's not about the ratio of correct and in correct, because supporting one does not rule out the other: both groups could get what they want without bothering each other, by just changing the name. And I support the removing of api pitfalls like this. But my point is that if the argument has a clear name, there is no pitfall. It's hard to argue about readabilty because it's subjective, but I personally can't agree that throwing inside the with block is more clear than a properly named parameter.

…

On Mon, May 6, 2019, 22:41 Anthony Sottile ***@***.***> wrote: I feel like, if the name of the argument was the problem, the obvious solution is renaming it. It seems there were multiple people using this and wanting to continue using this. there were *few* using it correctly and *many* accidentally using it incorrectly -- a common pitfall. The name of the argument was not the only issue here, but also the inflexibility of the api and complexity of the framework. Not really sure how pytest improves by insisting on workarounds. This isn't a workaround, we're removing a common pitfall, simplifying our interface, and encouraging a more readable and more flexible replacement. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3974 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABRE636JMAH5HMW5IUXMZ73PUCJYDANCNFSM4FU3TBGQ> .

[asottile]

Right, but even if it is "properly named" (which we couldn't agree on a name that wasn't more confusing) it is still a point of maintenance.

We decided that even if it was named properly, the usecase for the feature is so rare (and in most cases makes the test failure output worse) that retaining api complexity to support such a thing was not a good idea and therefore the removal.

[nicoddemus]

But it's not about the ratio of correct and in correct, because supporting
one does not rule out the other: both groups could get what they want
without bothering each other, by just changing the name.

I would agree if we were talking about something like 50% of users use the parameter, the other 50% don't. But our take from this thread was that it was used very little, so we would rather not keeping it in the API at all.

Our take comes from the fact that this has been issuing a warning since 4.1, stating that people could comment in this thread if they felt the parameter was useful; the few people who ended up commenting did use the alternative method or even stopped using the message parameter at all because it didn't really help in their case. At least this is my personal impression.

[mverleg]

Guess we'll have to agree to disagree

[ndevenish]

I've just encountered this in a test that was using it correctly. I got confused because I, first changed message to match but it was being used correctly (that it was used correctly in our code wasn't entirely clear from the code context). Then I looked at the documentation to find what message was supposed to do and found it had been scrubbed completely.

Then came here and it's only after scanning through this whole thread that I see a replacement solution that doesn't seem very satisfactory:

        with pytest.raises(exceptions.PropertyValidationError):
            prop.load(email)
            pytest.fail(f"wrongly validated: {email}")

Because - well, because it feels like I'm manually doing pytest's job; working around something that pytest should do. It feels like rather than encoding the test expectation in two places and requires thinking about whether the tests are competing. I'd be better off writing the more familiar:

try:
    prop.load(email)
    pytest.fail(f"wrongly validated: {email}")
except exceptions.PropertyValidationError:
    pass

which somewhat defeats the point of raises in the first place.

Perhaps in the future, I feel that it would have been better to have a solution or at least suggestion in place before telling everyone not to do what they've been told to do until then.

[nicoddemus]

Hi @ndevenish,

We did point the alternative in two places in the docs:

• https://docs.pytest.org/en/latest/reference.html#pytest-raises
• https://docs.pytest.org/en/latest/deprecations.html#raises-message-deprecated

But you are right that we should have done better and point to those links directly in the warning itself. What happened is that the warning came first and the docs later, and nobody realized that we should update the warning message.

[ndevenish]

Ah, that explains it - I tend to navigate to the docs through google and hit the non-reference https://docs.pytest.org/en/latest/assert.html#assertions-about-expected-exceptions.

No harm done, like you said above it looks like very few people used this (and FWIW I didn't know this parameter and when I saw it I assumed it was what match= does so changing good idea)

[ndevenish]

Don't know if it's something you generally try not to do but since people are sent to this ticket by the message (and update lag times) I wonder if it might be worth editing the first post to put a link to the deprecation workaround page at the top.

[nicoddemus]

Great idea, done. 👍

[nicoddemus]

@ndevenish: opened #5421, feel free to comment on there. 👍

[abadger]

I'm glad this change has been made (it's allowed me to finally figured out why tests some people contributed weren't properly failing when the exception message didn't match while other people's tests were properly failing! yay!)

To stop this from being a simple me too.... I've also noticed that the recipe solution is more accurately reflecting the prior behaviour than what I thought the prior behaviour would be. ie:

with pytest.raises(ValueError, message='Show this failure message'):
    raise TypeError()

will not display Show this failure message. It's not obvious why that is the case with the parameter to pytest.raises but it is glaringly obvious with the recipe solution.