Pytest: RFC: parametrizing conditional raising with pytest.raises

[The-Compiler]

Currently, parametrizing conditional raising of an exception is rather
cumbersome. Consider this:

def foo(inp):
    if inp:
        raise ValueError("Just a silly example")

When we write a test for it, we need to repeat ourselves:

@pytest.mark.parametrize('inp, raising', [(True, True), (False, False)])
def test_foo(raising):
    if raising:
        with pytest.raises(ValueError):
           foo(inp)
    else:
        foo(inp)

An easy way to solve this would be to add something like an activated
argument to pytest.raises, where it simply does nothing:

@pytest.mark.parametrize('inp, raising', [(True, True), (False, False)])
def test_foo(inp, raising):
    with pytest.raises(FooError, activated=raising):
        foo(inp)

---

But then sometimes, it's handy to parametrize the exception too - consider
this:

def bar(inp):
    if inp == 1:
        raise ValueError
    elif inp == 2:
        raise TypeError
    else:
        pass

and the test:

@pytest.mark.parametrize('inp, exception', [(1, ValueError), (2, TypeError),
                                            (3, None)])
def test_bar(inp, exception):
    if exception is None:
        bar(inp)
    else:
        with pytest.raises(exception):
            bar(inp)

So maybe we could just use None as a special value where pytest.raises does
nothing?

@pytest.mark.parametrize('inp, exception', [(1, ValueError), (2, TypeError),
                                            (3, None)])
def test_bar(inp, exception):
    with pytest.raises(exception):
        bar(inp)

Or if we're worried about None being accidentally used (though I'm not sure
how?), we could use a special value, potentially pytest.raises.nothing or so
:laughing:

Opinions?

[nicoddemus]

I think None being considered by pytest.raises as doing nothing seems sensible.

[RonnyPfannschmidt]

how about something completely different

like a different context manager, so that instead of parameterizing with booleans, we pass different contexts

@pytest.mark.parametrize('inp, expectation', [
    (1, pytest.raises(ValueError),
    (2, pytest.raises(TypeError),
    (3, pytest.wont_raise)])
def test_bar(inp, exception):
    with expectation:
        bar(inp)

[purpleP]

@RonnyPfannschmidt @nicoddemus I have an idea for completely different implementation.

You can see it here

https://github.com/purpleP/pytest_functest

[nicoddemus]

@purpleP thanks for sharing!

Your implementation seems interesting, but I would say it is a new way to declare tests altogether (more declarative than imperative) and independent from this proposal. :grin:

[nicoddemus]

@RonnyPfannschmidt

like a different context manager, so that instead of parameterizing with booleans, we pass different contexts

Seems interesting and just as easy to implement as the original proposal. @The-Compiler what do you think?

[purpleP]

@nicoddemus It is indeed declarative and IMHO it's good for such simple tests. What I think you wrong about is that it isn't independent. It's dependent in a way, that if one would use my plugin or something similar there wouldn't be need to rewriting pytest.raises in the first place.

[nicoddemus]

It's dependent in a way, that if one would use my plugin or something similar there wouldn't be need to rewriting pytest.raises in the first place.

Not sure, I think both can co-exist: declarative-style is great for code which is functional and withougt side-effects, but might not scale well depending on what you are testing. And we are talking about a small addition/improvement to pytest.raises, I'm pretty sure it won't require rewriting the whole thing. :wink:

[RonnyPfannschmidt]

what i am worried about is loosneing a contract

taking a function, and the suddenly making it work different seems very problematic to me

in particular since even a optiona_raises function is 7 lines including the contextmanager decorator
and it would not require a change to the original function to make something like that

making loose contracts in general is a maintenance nightmare later on because functions loose unifom behaviour since they act all over the place

im strictly opposed to modifying pytest-raises ot support none

[purpleP]

@nicoddemus Yes, declarative style isn't great when we're trying to test side-effects. But I thought that OP is talking exactly about that.
This change makes sense only in parameterized tests and parameterized tests make sense only when testing things without side-effects, don't you think? I mean I don't see how you would parametrize side-effects test in a way that it's readable etc. Maybe I'm wrong about that, but If I'm not, then my argument still holds.

[nicoddemus]

@RonnyPfannschmidt

taking a function, and the suddenly making it work different seems very problematic to me

Yeah I agree, and I think your proposal fits nicely with what @The-Compiler had in mind. @The-Compiler, thoughts?

Here is an example which I think also illustrates @The-Compiler's use case, taken from pytest-qt:

def context_manager_wait(qtbot, signal, timeout, multiple, raising,
                         should_raise):
    """
    Waiting for signal using context manager API.
    """
    func = qtbot.waitSignals if multiple else qtbot.waitSignal
    if should_raise:
        with pytest.raises(qtbot.SignalTimeoutError):
            with func(signal, timeout, raising=raising) as blocker:
                pass
    else:
        with func(signal, timeout, raising=raising) as blocker:
            pass
    return blocker

This could be changed to:

def context_manager_wait(qtbot, signal, timeout, multiple, raise_expectation,
                         should_raise):
    """
    Waiting for signal using context manager API.
    """
    func = qtbot.waitSignals if multiple else qtbot.waitSignal
    with raise_expectation:
        with func(signal, timeout, raising=raising) as blocker:
            return blocker

This change makes sense only in parameterized tests and parameterized tests make sense only when testing things without side-effects, don't you think? I mean I don't see how you would parametrize side-effects test in a way that it's readable etc. Maybe I'm wrong about that, but If I'm not, then my argument still holds.

I see your point, but are you arguing for us to drop this topic because your solution is enough? In that case I disagree, it is still worth discussing how change pytest.raises (or add a new pytest.no_raise). If on the other hand you are just pointing out an alternative, that's perfectly valid and welcome, but doesn't bar the current discussion.

[purpleP]

@nicoddemus And about whether or not rewrite pytest.raises. I'm not a pytest developer, so that's for you to decide, but it's seems the wrong thing to do. I'll give you my reasons.

1. Why waste time doing something, when you can just not doing it in the first place?
2. The whole thing about pytest.raises and parametrized tests is showing why Either Monad is better than Exceptions. So what we're doing is just working around python's limitations.
   With my approach what's happening is that we kind of turning side-effecting exception tests into declarative. From imperative try to execute this function and if it's not raises raise AssertionError into here is a function, here's check you need to try.
   And with your approach we making a workaround for a workaround. Meaning pytest.raises is a pretty much workaround from lacking Either and pattern matching. And adding None value is a workaround for a workaround.

If anything I'd think @RonnyPfannschmidt approach is better. Only I propose a slightly different version.

What if we make context manager that would kinda turn Exception into Either.

def either(f, exception_type, *args, **kwargs):
   try:
        return f(*args, **kwargs)
   except exception_type as e:
        return e

And then users can write tests like this

@parametrize('input,output',
    (('some_input', SomeError('some message')))
)
def test_some(input, output):
    assert some(input) == output

If we can somehow implement custom comparison for exceptions in assert statement than this should work.

[purpleP]

@nicoddemus No, I'm not arguing to drop the topic, I'm arguing that making new default value isn't the best way to do it.

[nicoddemus]

I'm arguing that making new default value isn't the best way to do it.

Oh sorry, I wasn't very clear then. After seeing @RonnyPfannschmidt's response, I think the best approach is to just provide a new context manager which just ensures nothing is raised. This way users can use it in the parametrization:

@pytest.mark.parametrize('inp, expectation', [
    (-1, pytest.raises(ValueError)),
    (3.5, pytest.raises(TypeError)),
    (5, pytest.does_not_raise),
    (10, pytest.does_not_raise),
])
def test_bar(inp, expectation):
    with expectation:
        validate_positive_integer(inp)

Just to clarify, this does not require changes to pytest.raises at all, only creating does_not_raise which should be very simple.

[The-Compiler]

@purpleP This is mainly a matter of taste of course, but I don't find your declarative test examples very readable personally. I still think having something like my or @RonnyPfannschmidt's proposal would make sense and is completely orthogonal to what you propose.

The proposal from @RonnyPfannschmidt sounds interesting indeed. There's one issue I have with it though: Seeing how many beginners I've seen asking "how do I check with pytest that a code doesn't raise an exception?" I kind of fear it being overused where it actually wouldn't be needed at all...

[RonnyPfannschmidt]

"this is a contextmanager that does nothing, it exists simply to ease composition of test parameters that include failure/nonfailure cases"

plus including a example how 2 tests can be turned into one should sufficie

[nicoddemus]

There's one issue I have with it though: Seeing how many beginners I've seen asking "how do I check with pytest that a code doesn't raise an exception?" I kind of fear it being overused where it actually wouldn't be needed at all...

But notice that you original proposal also has the same opportunity for "abuse":

with pytest.raises(None):
    validate_positive_integer(5) 

:wink:

But I think the advantage overweights this small disadvantage. Plus, we can (and should) mention in pytest.not_raises documentation that pytest doesn't differentiate between "errors" and "failures" like other test frameworks so using it in isolation is not that useful while at the same time demonstrating where it is useful: parametrization to check different exceptions.

[nicoddemus]

"this is a contextmanager that does nothing, it exists simply to ease composition of test parameters that include failure/nonfailure cases"

@RonnyPfannschmidt beat me to the reply for a few seconds, but I agree, I think this could be addressed in the docs.

[purpleP]

@The-Compiler How about my other proposal? Doesn't introduce overusing or anything, automatically compares messages from exception?

[The-Compiler]

@purpleP I don't understand it. You're saying you define an either context manager (which isn't a context manager?), and then don't use it, but use some for which I have no idea what it does?

[purpleP]

@The-Compiler Yeah, that's a typo. some is a function to test. And it's call should be packed into either. But, I don't think that that's a good idea now. It should work, and had small benefit of comparing message attribute and other attributes, but I guess this approach is alien in python, which is a shame.

[RonnyPfannschmidt]

@The-Compiler can we close this issue - and should we open up one for a doesn't raise context manager?

[The-Compiler]

@RonnyPfannschmidt I'm a bit late - but I don't see the point in opening a separate issue for the same thing (and then "losing" the history and rationale behind doing it that way)

[jakirkham]

Have found myself wanting this exact functionality a few times now. Seems there was a PR put together a few months back, but it was closed. 😕 Would be really nice to see something coalesce. What seems to be the blocker to implementing this in some form?

[nicoddemus]

Hey @jakirkham! 😄

It was closed only because we couldn't really reach a consensus on the naming. 😕

[jakirkham]

On the naming of the argument passed to pytest.raises or something else? What are the options?

[RonnyPfannschmidt]

from my pov the correct way was a new name because its a new behaviour and bike-shedding killed it in the end

[The-Compiler]

See the mailing list threads:
https://mail.python.org/pipermail/pytest-dev/2017-March/thread.html
https://mail.python.org/pipermail/pytest-dev/2017-April/thread.html

The main options were:

• with pytest.raises(None):
• with pytest.raises(pytest.NoException): or similar (like the above, but it makes it more explicit, and it won't be done by accident)
• with pytest.does_not_raise: or similar (which means you need to parametrize the whole "expectation")

Then it was bikeshedded to death. 😢

(Honestly, I stopped caring. I just started adding stuff to my own testsuite instead of contributing them to pytest. I'd still be happy for this to be added, but if it ends like this, I'm out.)

[The-Compiler]

By the way, something which wasn't mentioned yet: The problem with having a separate context manager like does_not_raise is that you have to parametrize the whole expectation like so:

@pytest.mark.parametrize('inp, expectation', [
    (1, pytest.raises(ValueError),
    (2, pytest.raises(FooError, match="Match on error message"),
    (3, pytest.does_not_raise)])
def test_bar(inp, expectation):
    with expectation:
        bar(inp)

However, if you want to use other nice features of pytest.raises like match (see above), this gets really cumbersome, and your message is "far away" from the rest of your test.

So my vote would still be for with pytest.raises(pytest.NoException): or the (simpler but less explicit) with pytest.raises(None):, for very much practical rather than ideological reasons :wink:

[RonnyPfannschmidt]

the same kind of practical process is the reason why marks are broken so badly and collection structure is broken so badly - if we don't consistently stop it we are just going to be in a mess

[The-Compiler]

The mailinglist thread already showed you that the implementation is trivial. I really fail to see where your problem is, this is something a lot different to the collection trees or marks.

[RonnyPfannschmidt]

@The-Compiler this is about consistency in development process - practices tend to to proliferate, so bad practices on small scale will spill over to larger scale if you deem it acceptable,

and if you cant even keep bad practices (like mangled responsibilities) from small scale things, how can we hope to keep them from slipping into larger things

[The-Compiler]

"Although practicality beats purity", as per the Zen of Python :wink:

How is this a mangled responsibility though? I'm not saying pytest.raises should, like, handle Qt warnings or whatever (there's pytest-qt for that :wink:). There's a very good usecase for what I proposed (judging from the +1's, and the ML threads that's not only me), and "It does not raise anything" seems to fit very well with pytest.raises. From what I see, everyone except you seems to agree it's a good idea.

...and frankly, this kind of stuff is driving me (and possibly others) away from contributing further to pytest. :cry:

[RonnyPfannschmidt]

for raises the progression would be starting at ensures an exception is raised
we slipped in a ensure its text matches something, which even unittest solved better (new responsibility new name)

now we want to slip in ensure no exception is raised which is a completely different responsibility
and we encode it using a magical constant thats just a mess right in front of you

and this is also the same process in which the collection tree and marks slowly grew more and more broken responsibilities (and by now i keep sinking weeks into them trying to fix stuff without breaking more)

[jakirkham]

Didn't realize this before, but it appears pytest.warns does something special for None. Seems like there is a good argument for a similar strategy with pytest.raises.

[RonnyPfannschmidt]

that just the same api design mistake already taken root :(

[massich]

Using None makes sense to me. It just comes really natural specially for pytest.warns even more when its present in the documentation as pointed out by @jakirkham. But maybe this is the whole point of this discussion, using None might lead to misunderstanding. Because None does not do what I was expecting. In the following example I would expect pytest.warns to fail in the second case:

>>> import pytest
>>> import warnings
>>> with pytest.warns(None):
...     pass
>>> with pytest.warns(None):
...     warnings.warn(FutureWarning)
>>>

So where do we go from here? Modify the doc? tackle the feature?

~If I understand it properly, the problem is mainly the use of None to trigger this behavior due to the implications of None. So couldn't we create an object called NoWarning (or NoError for pytest.raises) and compare the expected_warning (or expected_exception) to them to trigger the numpy's assert_no_warning behavior?~

[RonnyPfannschmidt]

please read again - the problem i see and complain about is using any magic constant to trigger reversed/distinct behaviour when one could just use a new function with a name telling exactly what it does

[massich]

(Honestly, I stopped caring. I just started adding stuff to my own testsuite instead of contributing them to pytest. I'd still be happy for this to be added, but if it ends like this, I'm out.)

@The-Compiler can you point me out where? I'm curious to see how did you implement it

[arel]

For anyone looking for a solution to this _now_ before people agree on naming, you can wrap pytest.raises to support None like this:

from contextlib import contextmanager

import pytest


def raises(error):
    """Wrapper around pytest.raises to support None."""
    if error:
        return pytest.raises(error)
    else:
        @contextmanager
        def does_not_raise():
            yield
        return does_not_raise()

Then you can use it with parameterize:

@pytest.mark.parametrize('example_input,expected_error', [
    (3, None),
    (2, None),
    (1, None),
    (0, ZeroDivisionError),
])
def test_division(example_input, expected_error):
    """Test how much I know division."""
    with raises(expected_error):
        assert (6 / example_input) is not None

[arel]

Also, I'm not sure None is any more magic than 0 is to the integers.

[RonnyPfannschmidt]

@arel magic numbers are any time when one uses a general value that encodes specific meaning without naming it

so using None to mean "throws no exception" makes it a magical constant

given that it needs a new name to encode new behaviour, its far more simple to just make a new function with a new name and keeping the logic paths more simple

[Sup3rGeo]

Just needed this same functionality but surprised to know that, even though almost all alternatives discussed would be good enough for the job (at least for me), none was implemented.

It seems though that Python 3.7 onwards will have a null context manager that could be used in this case too:
https://stackoverflow.com/a/45187287

[jakirkham]

Friendly nudge. Ran into this problem again and figured I'd put it back on people's radar. 😉

Clearly this is desirable to many. Any ideas how we can unstick this one? 😄

[massich]

No idea but let's unstick it :)

On Mon, Apr 2, 2018, 19:50 jakirkham [e-mail] wrote:

Friendly nudge. Ran into this problem again and figured I'd put it back on
people's radar. 😉

Clearly this is desirable to many. Any ideas how we can unstick this one?
😄

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/pytest-dev/pytest/issues/1830#issuecomment-377992242,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGt-49tm1lR-4Xnh1mCmOnYaQ6qU7m4Oks5tkmTsgaJpZM4JpKXC
.

[jakirkham]

One attempt was PR ( https://github.com/pytest-dev/pytest/pull/2339 ), which seemed to be mostly agreeable except for naming. It went to this mailing list thread to determine a better name IIRC. Using None came up again, which was not agreeable. There was a poll along these line, but IDK what the results were. Maybe just coming up with a new name that sounds good would be enough.

[nicoddemus]

pytest.do_not_raise was the winner in the end I think.

[massich]

If pytest.do_not_raise is used, then how would you rewrite this?

@pytest.mark.parametrize('example_input,expected_error', [
    (3, None),
    (2, None),
    (1, None),
    (0, ZeroDivisionError),
])
def test_division(example_input, expected_error):
    """Test how much I know division."""
    with raises(expected_error):
        assert (6 / example_input) is not None
 4  

[Sup3rGeo]

@massich

@pytest.mark.parametrize('example_input,expectation', [
    (3, do_not_raise),
    (2, do_not_raise),
    (1, do_not_raise),
    (0, raises(ZeroDivisionError)),
])
def test_division(example_input, expectation):
    """Test how much I know division."""
    with expectation:
        assert (6 / example_input) is not None

[massich]

Does this mean that we should add pytest.do_not_warn??

[nicoddemus]

Good point, it would make sense from a consistency point of view.

[RonnyPfannschmidt]

+1 - if we add do_not_warn we should deprecate the magic value parameter to warn at the same time

[massich]

Sounds like a plan!

[nicoddemus]

if we add do_not_warn we should deprecate the magic value parameter to warn at the same time

What do you mean @RonnyPfannschmidt ?

[RonnyPfannschmidt]

@nicoddemus they currently accept some magical value as argument (which is an api oversight and was used as a argument for pytest.raises(None) before

[nicoddemus]

@RonnyPfannschmidt Oh I did not know/remember, thanks.

[ashemedai]

Did anything happen with this proposal? Because it seems the idea died out again judging from just this issue.

[Sup3rGeo]

@nicoddemus Should we revive PR #2339 with the new name?

[massich]

I was planning to put some time on it ~this weekend~ and also modify the
warnings.
last weekend did not happen. Lets see next ;)

[asottile]

Just my 2c, this is going to get abused

The original example in the issue would really be two tests as it is testing two behaviors (one with acceptable inputs and one with unacceptable (exceptional) inputs)

The reason I say this is going to be abused is we had a similar context manager in testify and found that developers (especially beginners) would add it to every test despite it being a noop. Their thought being that it is better to be explicit that it doesn't raise but the mistake being that an exception _already_ failed the test.

EDIT (for example):

def test_x():
    with pytest.does_not_raise():
        assert x(13) == 37

when really they wanted just

def test_x():
    assert x(13) == 37

[nicoddemus]

Thanks @asottile, that's important information to have.

Given @asottile experience of people abusing this feature with testify (and all the controversy about how to name it), I propose that we don't introduce this in pytest.

[massich]

Is there any option to prevent users to misuse it, apart from doc?

[nicoddemus]

Not sure, don't think so...

[asottile]

fortunately, if you _really_ want this behaviour you can get it today from the standard library:

if sys.version_info >= (3, 7):
    from contextlib import nullcontext
elif sys.version_info >= (3, 3):
    from contextlib import ExitStack as nullcontext
else:
    from contextlib2 import ExitStack as nullcontext


@pytest.mark.parametrize(
    ('x', 'expected_raises'),
    (
        (True, pytest.raises(ValueError)),
        (False, nullcontext()),
    ),
)
def test(x, expected_raises):
    with expected_raises:
        foo(x)

• nullcontext
• ExitStack
• contextlib2

[Sup3rGeo]

Let's consider the scenarios of having/not having the noop context manager (considering here do_not_raise):

Worst thing that can happen if we DO have do_not_raise:
a1 - Proponents of pytest.raises(None) won't be 100% satisfied. But maybe still 80% satisfied?
a2 - Beginners can abuse it. This can happen with a lot of things as well, but just because some users can abuse it, it is not fair to prevent all the other users from having it.

Worst thing that can happen if we DON'T have do_not_raise
b1 - Increasing the LOC and maintenance passive where we require to parametrize on exception raise.
b2 - Not consistent with warns in terms of noop.

Bottom line: Just implement it - its a small and uncertain harm (I would doubt someone will complain) compared to a big and certain benefit (many happy users).

[asottile]

@Sup3rGeo my problem is it encourages two bad patterns:

1. tests with logic, (abuse of parametrize to test disparate behaviours)
2. tests with unnecessary contexts (noop do_not_raise when it is unnecessary)

is it insufficient to from contextlib import nullcontext as do_not_raise?

[massich]

from contextlib import suppress as do_not_raise

@pytest.mark.parametrize('example_input,expectation', [
    (3, do_not_raise()),
    (2, do_not_raise()),
    (1, do_not_raise()),
    (0, raises(ZeroDivisionError)),
])
def test_division(example_input, expectation):
    """Test how much I know division."""
    with expectation:
        assert (6 / example_input) is not None

Test session starts (platform: linux, Python 3.6.6, pytest 3.6.3, pytest-sugar 0.9.1)
rootdir: /home/sik/code/mne-python, inifile: setup.cfg
plugins: smother-0.2, sugar-0.9.1, pudb-0.6, ipdb-0.1, faulthandler-1.5.0, cov-2.5.1

 matplotlib_test.py ✓✓✓✓                                                                  100% ██████████
======================================= slowest 20 test durations =======================================
0.00s setup    matplotlib_test.py::test_division[3-expectation0]
0.00s setup    matplotlib_test.py::test_division[2-expectation1]
0.00s setup    matplotlib_test.py::test_division[1-expectation2]
0.00s teardown matplotlib_test.py::test_division[3-expectation0]
0.00s setup    matplotlib_test.py::test_division[0-expectation3]
0.00s call     matplotlib_test.py::test_division[3-expectation0]
0.00s teardown matplotlib_test.py::test_division[2-expectation1]
0.00s teardown matplotlib_test.py::test_division[1-expectation2]
0.00s call     matplotlib_test.py::test_division[2-expectation1]
0.00s call     matplotlib_test.py::test_division[1-expectation2]
0.00s teardown matplotlib_test.py::test_division[0-expectation3]
0.00s call     matplotlib_test.py::test_division[0-expectation3]

Results (0.08s):
       4 passed

[RonnyPfannschmidt]

as a backport is availiable in contextlib2 i propose documenting this
CC @nicoddemus

[asottile]

I provided a full recipe here including contextlib2 if someone wants to copypaste into docs

[RonnyPfannschmidt]

closing after the follow-up is created

[wimglenn]

Other options not bikeshedded here yet:

• Implement this in a plugin: then it can quietly disappear in a Python 3 only world, rather than hanging around forever / wanting to be deprecated?
• Parametrize cases that raise and cases that don't raise in separate tests (less logic in the tests)

Regarding this second point, it's arguably clearer this way anyhow since the use cases are sufficiently different. If code is raising, you usually want to make an assertion on the _exception_ somehow, if code is not raising, you usually want to make an assertion on the _results_ somehow. That's a high level difference, suggesting they should be different tests in the first place.

It's already somewhat apparent even in the example tests of the PR recently created, there is assert (6 / example_input) is not None, but shouldn't you really want to be asserting on the result of the division here?

[nicoddemus]

A plugin indeed seems like a good option