Pytest: 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,

• https://github.com/pandas-dev/pandas/pull/22681#discussion_r217181448
• https://github.com/scipy/scipy/pull/9264
• https://github.com/scikit-learn/scikit-learn/pull/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 https://github.com/pytest-dev/pytest/issues/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 (https://github.com/pytest-dev/pytest/issues/3974#issuecomment-421478825). 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 [e-mail]
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)
https://github.com/pytest-dev/pytest/issues/3974#issuecomment-421478825).
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
https://github.com/pytest-dev/pytest/issues/3974#issuecomment-421630939,
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]

... it should be keyword-only

definitely!

[WolfgangFellger]

Was:

with pytest.raises(asyncio.TimeoutError, message="Client got unexpected message"):
    await asyncio.wait_for(websocket.recv(), 0.5)

and is now back to the more explicit:

try:
    msg = await asyncio.wait_for(websocket.recv(), 0.5)
    pytest.fail(f"Client got unexpected message: {msg}")
except asyncio.TimeoutError:
    pass

...which has the benefit of also printing the message if the test fails, so is all around better I guess ;)

+1 on changing this argument BTW, I've fallen into that trap as well.

[nicoddemus]

@WolfgangFellger actually we planned on removing it entirely, given that we couldn't find very useful cases for it. You have posted a valid use case however, so we need to decide if we want, in a future version, to rename the argument or just remove it altogether.

Given your example, do you prefer the original version or the new, more explicit, version?

[asottile]

I'm still not convinced -- I think the stacktrace should already have sufficient information that you don't need the message parameter -- it shows exactly the line of code you're looking at and the test name. And actually, I think the message= parameter makes the output _worse_.

In the example you get a stacktrace (doctored because I don't have your code):

$ pytest t.py 
============================= test session starts ==============================
platform darwin -- Python 3.7.0, pytest-4.1.0, py-1.7.0, pluggy-0.8.1
rootdir: /private/tmp/t, inifile:
collected 1 item                                                               

t.py F                                                                   [100%]

=================================== FAILURES ===================================
________________________________ test_name_here ________________________________

        with pytest.raises(asyncio.TimeoutError):
>           await asyncio.wait_for(websocket.recv(), 0.5)
E           Failed: DID NOT RAISE <class 'asyncio.TimeoutError'>

t.py:12: Failed
=========================== 1 failed in 0.08 seconds ===========================

That is, I'd much rather see and debug that output than your current failure scenario (again doctored):

$ pytest t.py 
============================= test session starts ==============================
platform darwin -- Python 3.7.0, pytest-4.1.0, py-1.7.0, pluggy-0.8.1
rootdir: /private/tmp/t, inifile:
collected 1 item                                                               

t.py F                                                                   [100%]

=================================== FAILURES ===================================
________________________________ test_name_here ________________________________

        with pytest.raises(asyncio.TimeoutError, message='Client got unexpected message'):
>           await asyncio.wait_for(websocket.recv(), 0.5)
E           Failed: Client got unexpected message

t.py:12: Failed
=========================== 1 failed in 0.08 seconds ===========================

(note that the doctoring was just to put your code example in and I didn't change any of the pytest-parts)

If you want the exact behaviour you had before, I'd suggest this instead:

with pytest.raises(asyncio.TimeoutError):
    await asyncio.wait_for(websocket.recv(), 0.5)
    pytest.fail('Client got unexpected message')  # I'd use `raise AssertionError(...)` myself, but this is an exact equivalent

[WolfgangFellger]

I don't have a strong opinion on this. The deprecation warning asked me to reply if I used that feature, so I did ;) The three-liner suggested by @asottile seems like an okay compromise. Let's see if somebody else speaks up.

[nicoddemus]

I don't have a strong opinion on this. The deprecation warning asked me to reply if I used that feature, so I did ;)

Sure, and thanks for that, we appreciate it. 👍

[tomaskrizek]

I've also run into the DeprecationWarning. I think there's some use for message/message_when_not_raised, but I don't mind if it's removed, as long as an alternative solution is properly documented.

Thanks for the example snippet, @WolfgangFellger !

[nicoddemus]

Thanks @tomaskrizek for the feedback.

I think the alternative proposed by @asottile is on point:

with pytest.raises(asyncio.TimeoutError):
    await asyncio.wait_for(websocket.recv(), 0.5)
    pytest.fail('Client got unexpected message')

[pawelswiecki]

I used message parameter together with mark.parametrize in testing validation. It was used to state what exactly is being tested in the current parameter set. Using message parameter was 50% for the user running tests and 50% for documenting purposes. Now, to avoid the warning, I'm forced to change it to a message in comments, which is not that bad (I still have 50% functionality left), yet it's annoying.

[nicoddemus]

@pawelswiecki thanks for the feedback!

You can get the same functionality by calling pytest.fail right after the code which is supposed to raise the exception, like this:

with pytest.raises(asyncio.TimeoutError):
    await asyncio.wait_for(websocket.recv(), 0.5)
    pytest.fail('Client got unexpected message')

This way you will still get the exception message if the exception is not raised.

[pawelswiecki]

@nicoddemus Right, thanks for the tip!

[namurphy]

You can get the same functionality by calling pytest.fail right after the code which is supposed to raise the exception

One thing to be careful of is that this doesn't work as intended for pytest.warns. The following test will always fail, even though a warning is appropriately issued.

def test_pytest_warns_with_pytest_fail():
    with pytest.warns():
        warnings.warn()       
        pytest.fail("A warning was not issued as expected.")  # This line always runs

We can get the intended behavior with try and except.

def test_pytest_warns_with_try_except():
    try:
        with pytest.warns():
            warnings.warn()
    except pytest.fail.Exception:
        pytest.fail("A warning was not issued as expected.")  # Runs only when intended

The problem with this approach is that it leads to more complex code than if we could use something like the message argument, though I'm curious if there's a simpler way to do this. In this case it would be useful to have the message keyword for pytest.warns but renamed it to something less ambiguous, like maybe failure_message which was mentioned above.

[asottile]

warns never supported the message= kwarg -- though you can get what you want with either the snippet you've provided or by doing this:

def test():
    with pytest.warns(Warning) as warninfo:
        f()
        if not warninfo:
            pytest.fail('Expected a warning!')

[namurphy]

warns never supported the message= kwarg

Yep, I just saw that in the documentation, after finding a test I wrote last year that used message= in pytest.warns. Oops!

you can get what you want with either the snippet you've provided or by doing this:

def test():
    with pytest.warns(Warning) as warninfo:
        f()
        if not warninfo:
            pytest.fail('Expected a warning!')

Yes, that's a cleaner way to do what I was trying to do. I like it! It would be great to include that in the pytest documentation as well. Thanks!

[asottile]

Yes, that's a cleaner way to do what I was trying to do. I like it! It would be great to include that in the pytest documentation as well. Thanks!

🎉 -- would you be interested in contributing a patch that adds that recipe to the docs? Definitely agree it would be useful

[schmittlauch]

Just ran into the deprecation message, and I think the message parameter is still useful, both for for documentation purposes and user info.
For example in a project with different authorships of different modules/ parts of code you could provide a high-level message of what went wrong, even when you don't understand the code of someone else's module at first sight.
One could also just put comments before each call to raises, but I find "message" still a nicer solution.

I'm using the described alternative now.

[nicoddemus]

Hi @schmittlauch, thanks for the response.

I think the main problem is not that message is only useful sometimes, but that a lot of people fall into the trap of using message when they meant match (myself included). The alternative of using pytest.fail right at the end of the with statement seems like a good compromise.

[scottbelden]

Should the docs be updated? I stumbled on to this because I was looking at the documentation for pytest.raises and the part about message does not indicate that it is deprecated so I didn't find out until I saw the warnings after adding it. It also comes before match so that might explain why people sometimes use message when they mean match.

In my case I wanted the functionality of message, but I saw the example in this thread of calling pytest.fail which accomplishes the same thing and is perfectly reasonable to me.

[nicoddemus]

Hi @scottbelden,

Should the docs be updated?

Indeed, thanks for the heads up!

In my case I wanted the functionality of message, but I saw the example in this thread of calling pytest.fail which accomplishes the same thing and is perfectly reasonable to me.

Great, thanks for the feedback!

[nicoddemus]

@scottbelden https://github.com/pytest-dev/pytest/pull/4798, thanks again. 👍

[mverleg]

I would like to cast my vote in favour of message_when_not_raised.

I think it's useful to be able to provide a message for failed assertions. That's why the assert keyword has that option, right? I'm honestly surprised I seem to be the minority.

And this

with raises(TypeError, message_when_not_raised='There is no hook to serialize '
        'RLock, so this should fail, otherwise some hook is too eager.'):
    dumps([RLock()])

...seems more intuitive than...

with raises(TypeError):
    dumps([RLock()])
    # TypeError did not get raised
    fail('There is no hook to serialize RLock, so this should fail, '
        'otherwise some hook is too eager.')

...to me.

I understand that the original name might've been confusing and should be changed. But I did use message for the original purpose and am sad to see it gone.

[manoadamro]

What did this end up getting renamed to?
I get the deprecation warning, but can't seem to find out what it was replaced with 😢

I am testing a massive list of email addresses:

    for email in INVALID_EMAILS:
        with pytest.raises(exceptions.PropertyValidationError, message=f"wrongly validated: {email}"):
            prop.load(email)

I know I could write out a test case for each of the 100+ strings... but this was a lot more convenient. So this would be my use case for keeping some sort of message parameter 😄

[fmigneault]

So basically, because people cannot read docs, you penalize users that where using message correctly? :/
I would also like to have at least a replacement keyword to keep functionality.
Sometime the reason as to why the raised exception is expected is not as obvious for other coders...

Maybe use reason as it seems to be used for skip markers?
Otherwise, +1 if message_when_not_raised is the preferred way to go.

[nicoddemus]

@manoadamro and @fmigneault,

So far we have considered message very error prone, regardless if the docs state otherwise: if an API consistently trips the users, it is not a good API, regardless of the docs.

Having said that, please consider that we have a very easy alternative without relying on the message parameter. Using @manoadamro's example:

    for email in INVALID_EMAILS:
        with pytest.raises(exceptions.PropertyValidationError, message=f"wrongly validated: {email}"):
            prop.load(email)

To get the exact same functionality all you need to do is change it to:

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

IMHO this seems like a good solution while avoiding the pitfalls that the message parameter brings.

[fmigneault]

@nicoddemus I understand. Maybe the deprecation warning should be updated to tell people to use this alternative with pytest.fail ?

[nicoddemus]

Actually we already have that documented in the "deprecation" docs, but I have added links to it in the main documentation (https://github.com/pytest-dev/pytest/pull/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 [e-mail] 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
https://github.com/pytest-dev/pytest/issues/3974#issuecomment-489768678,
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.