pytest: Confusion between message and match parameters in pytest.raises

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.

@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.

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.

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

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.

Guess we’ll have to agree to disagree

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 notifications@github.com 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 .

@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.

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.)

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.

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.

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.

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.

Great idea, done. 👍

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!

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 Right, thanks for the tip!

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.

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')

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

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.