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
😆

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

[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. 😉

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