Feature request: refactor the pytest.raises interface

[wimglenn]

Interface for pytest.raises is a stumbling block. Problems:

1. The "message" kwarg violates principle of least surprise. User probably expected: simple match on exception message (useful), but it's actually for providing custom failure message (not very useful). Recommend deprecation. message has now been deprecated.
2. The "match" kwarg violates principle of least surprise. It will catch substrings, for example. And it's annoying when the error message itself contains regex syntax that you'll need to escape. Could possibly be renamed to "regexp" or something? ...don't care / don't use it
3. No easy way to achieve the most simple and basic use-case: assert exact match on an exception instance args with basic types such as strings and/or numbers. use the plugin instead: pytest-raisin

Here are some examples of problematic usage.

# looks right, but doesn't assert what it seems like it's asserting. trappy for beginners!
with pytest.raises(Exception, message="boom"):
    Exception("boom")

# whoops!
with pytest.raises(Exception, message="boom"):
    Exception("bang")

# whoops again! still not there yet..
with pytest.raises(Exception, match="boom"):
    Exception("kaboom")

# correct usage
with pytest.raises(Exception, match="^boom$"):
    Exception("kaboom")

What if the thing you want to match is not really a string? Can encounter this frequently when testing the CLI parts of code (since argparse is gonna raise SystemExit with an integer return code). Problems:

# passes, but not why you thought
with pytest.raises(SystemExit, match=0):
    sys.exit(0)

# passes??
with pytest.raises(SystemExit, match=0):
    sys.exit(1)

# crashes, but crashes too late (`TypeError` in re compile)
with pytest.raises(SystemExit, match=1):
    sys.exit(1)

# correct usage
with pytest.raises(SystemExit) as cm:
    sys.exit(0)
assert cm.value.args == (0,)

# another trap! d'oh!
with pytest.raises(SystemExit) as cm:
    sys.exit(0)
    assert cm.value.args == (0,)

I would like to propose using exception instances to overcome these stumbling blocks.

Example usage:

with pytest.raises(SystemExit(2)):
    sys.exit(2)

testfixtures.ShouldRaise is an example of this usage, and I'd love to have that interface in pytest directly. The comparison objects which make it work are non-trivial. A much easier to implement alternative, though less beautiful, might be just to support matching the args attribute as tuple.

with pytest.raises(SomeError, exc_args=(2, "code two")):
    raise SomeError(2, "code two")

Admittedly it won't help people who put too much logic into their custom exception classes, but it should cover majority standard use-cases (where exceptions are dumb subclasses, just with a custom numeric error code or message).

What do you think?

[nicoddemus]

Hi @wimglenn,

You are absolutely right. In fact we had a long discussion about this in #3974.

I would like to avoid breaking things, even if match might be a little misleading as you comment.

We had some consensus with the proposal in #3974 (comment) about the message parameter.

Your proposal then would be improve pytest.raises to match an exception exactly. I particularly like the idea of passing the exception instance that should be matched exactly (SystemExit(2)).

[asottile]

one problem I see with passing exception instances into raises is few exception types properly implement __eq__ and so you'd potentially enable another pitfall. I'd rather lean towards fewer behaviours of raises and leave the assertion up to the user.

I think we should steer people more towards the context manager -- that said, I don't have any counters to this example other than linting 🤷‍♂️ (pytest knows the full ast of your tests, maybe it could warn on this behaviour during discovery)

with pytest.raises(SystemExit) as cm:
    sys.exit(0)
    assert cm.value.args == (0,)

actually, now that I think about it -- any code coverage tool is going to expose this mistake so maybe it's less of a bad problem than I think.

I'm in favor of deprecating and removing `message=

Should we consolidate this discussion into #3974 (duplicate)?

[nicoddemus]

I'm in favor of deprecating and removing `message=

Too slow! 😁 #4539

[wimglenn]

@asottile To clarify, my suggestion wasn't to rely on exception __eq__ (which in almost all cases will be identity-based / useless) but for pytest.raises itself to do a more nuanced comparison of exception instances.

[asottile]

@asottile To clarify, my suggestion was not be to rely on exception's __eq__ (which in almost all cases will be identity-based) but for pytest.raises itself to do a more nuanced comparison of exception instances.

yes I understand that, implementing our own custom idea of "equality" seems like a place to write bugs and surprising behaviour.

[wimglenn]

I see the point, and could be convinced either way, however this wouldn't be using anywhere an equality assertion like assert exc1 == exc2. Rather, the means of comparison would be a documented behaviour of pytest.raises (probably just something simple like comparing equality of the args attribute).

For the surprising behaviour, we already have it ;)

[asottile]

Other than message= I don't find anything else you've posted "surprising". Adding a custom non-equality "equality" check seems like a bad idea

[wimglenn]

Not even this one?

with pytest.raises(SystemExit, match=0):
    sys.exit(2)

Why match=0 was treated the same as match=None or match=".*"?

Adding a custom non-equality "equality" check seems like a bad idea

Yes, agreed.

[asottile]

Ooh missed that one. Seems like a micro bug needing an is somewhere

[asottile]

Here's a fix for that one: #4540

[nicoddemus]

message is now deprecated as of #4539. 🎉

I guess what's left here is to discuss if/how we can improve pytest.raises to also check for the exception's args, correct?

[wimglenn]

Correct.

Sensible options I see:

1. allow to pass exception instance, and equality compare args attributes. any more sophisticated comparison has to use cm.value instead
2. allow to pass exception instance and allow user to provide a callable, takes two arguments, and compares exc. optional - default callable just compares exc args.
3. do not allow to pass exception instances. add an "exc_args" keyword for pytest.raises to allow easier assertion on those.
4. do nothing.

[asottile]

my vote is for the simplest interface: 4 😆

[wimglenn]

4 is certainly simplest to code up. I could have it ready in a day or two.. 😉

my vote for 1.

[asottile]

1, 2, and 3 are all an "equality that is not __eq__" which to me is a double edged sword

[nicoddemus]

While I sympathize with the sentiment of using an exception instance, I'm afraid we will start to receive bug reports when users use custom exceptions and pytest doesn't behave the way they expect, even if we explicitly state the behavior in the docs.

If we are to do something, I would go for 3, naming it match_args.

But I'm not sure if we are not bloating the API by doing so, it seems the vast majority of use cases is to check the exception message using match. When needing to check non-string .args, using cm.value.args seems to be sufficient IMHO.

In your experience, does that happen a lot of the pytest.raises statements you write?

[asottile]

most of my calls follow one of these three patterns:

with pytest.raises(tp): 
    thing_that_raises()

with pytest.raises(tp) as excinfo:
    thing_that_raises()
assert excinfo.value.args == (...,)

with pytest.raises(tp) as excinfo:
    thing_that_raises()
msg, = excinfo.value.args
assert msg == ...

the second and third could "benefit" from the proposed 3, not really the proposed 2 (which was shown problematic in testify). That said, I don't think I'd ever write the unwieldy pytest.raises(tp, exc_args=(...,)) and stick with my current patterns. I also avoid match because every time I've used it I've needed to look up the regular expression rules and in most cases use '^' + re.escape(msg) + '$' to get the behaviour I want

[RonnyPfannschmidt]

its impossible to do it correct, its easy to check on the object - lets let others experiment with it for a while and keep it out of the core - if its truely needed, a good pattern will emerge and we can take a look at taking it in

[wimglenn]

Since there was not the support for this yet, I've added what interface I wanted in a plugin instead.

https://github.com/wimglenn/pytest-raisin

During implementation I noticed a few other weird buglets in the other usages of pytest.raises.

The code string version silently ignores match, message:

>>> pytest.raises(Exception, "2+'two'", match="cant put 2 and two together")
<ExceptionInfo TypeError tblen=2>

The code string version tries to unpack multiple args:

>>> pytest.raises(Exception, "2+'two'", "what")
ValueError: too many values to unpack (expected 1)

The passing a callable version is sharing the namespace for the callable args and also the args to pytest.raises itself in kwargs. So there is no way to use the match there. It's just bad design and impossible to handle the edge cases correctly.

>>> def func():
...     raise ValueError("wtf")
... 
>>> pytest.raises(Exception, func, match="yeah")
<ExceptionInfo TypeError tblen=1>
>>> pytest.raises(ValueError, func, match="nah")
TypeError: func() got an unexpected keyword argument 'match'

I could be convinced otherwise if there is a good use-case for the callable and exec forms, otherwise I'd recommend deprecation of those legacy forms, as they complicate the implementation too much. The context-manager usage is the one-obvious way to do it!

[asottile]

code string is already deprecated: #4443

[wimglenn]

Ah, good!

[asottile]

yikes can't imagine that breaking anything ;)

[wimglenn]

I just fallback to original impl when arg0 is not an exception instance. What can it break and how?

[asottile]

oh no problem with the implementation that seems fine, just I've seen this backfire one too many times where someone assumes that's how it always works and then breaks some code elsewhere

[RonnyPfannschmidt]

@wimglenn unfortunately the call/exec based variant of pytest.raises which pre-dates the context-manager had supported arbitrary arguments already

when i introduced the raises context manager i made the fatal choice of using the same name for both functionalities (this was for pytest 1.3.2 more than 8 years ago)

[wimglenn]

Can close this now, the plugin does the needfuls.

pip install pytest-raisin