[pytest-dev] [3.1 feature] "assert not raise exception" helper: opinions about the name

[holger krekel]

Hi Ronny,

On Fri, Mar 31, 2017 at 09:55 +0200, Ronny Pfannschmidt wrote:
> Hi Holger,
> 
> what we do here is to add support for "inverted behavior" to a function!
> 
> this is a direct increase in inner complexity that looks easy on the
> outside, but is not all all simple on the inside

The complexity of pytest.raises stems from its 10 years history of
supporting exception-control interactions with the dependent code block
and especially the grown ways to specify the code blocks -- as a string,
as function with args and finally through a context manager.  

Now, allowing None to mean "no exception expected" is not changing the
meaning of the "expected_exception" argument like string-func-contextmanager
did for the rest of the pytest.raises arguments.

> its changes like that that ensure maintenance pain in future,
> 
> in reference, just take a look at marks,
> 
> that get copied around between different classes on a inheritance tree
> and are a major mess to fix because of the sheer amount of easy changes
> that being them to where they are now

i never considered marks easy and some early decisions lead to later
complexity, agreed.  And let's not talk of the wonderful collection tree ...
 
> same goes for the mess with fixture declaration that double as object
> caches braking
> while trying to introduce multi-skoped fixtures.
> 
> I am increasingly hostile to "easy" things that have a larger cost in
> inner complexity,
> because we don't have full time employees to deal with the fallout.
> 
> And the needless increase in complexity directly devalues the time i
> spend volunteering
> (because fixing finer details gets so much harder when complexity increases)
> 
> And with a Kid on the way i'm no longer willing to take those kinds of
> blows in personal time at least.

Mine just turned six and i can relate to such considerations ... and i agree
it's worthwhile to be critical when "easy" is used for advertising a
change -- e.g. i was skeptical with merging pytest.yield_fixture and
pytest.fixture for that reason but eventually was ok with it because the 
concrete code changes did not look too frightening ...  so, the
pytest.raises(None) change has roughly this patch (docs missing):

    diff --git a/_pytest/python.py b/_pytest/python.py
    index 59492bc4..32954ad4 100644
    --- a/_pytest/python.py
    +++ b/_pytest/python.py
    @@ -1175,20 +1175,22 @@ def raises(expected_exception, *args, **kwargs):
     
         """
         __tracebackhide__ = True
         if expected_exception is AssertionError:
             # we want to catch a AssertionError
             # replace our subclass with the builtin one
             # see https://github.com/pytest-dev/pytest/issues/176
             from _pytest.assertion.util import BuiltinAssertionError \
                 as expected_exception
    -    msg = ("exceptions must be old-style classes or"
    -           " derived from BaseException, not %s")
    -    if isinstance(expected_exception, tuple):
    -        for exc in expected_exception:
    -            if not isclass(exc):
    -                raise TypeError(msg % type(exc))
    -    elif not isclass(expected_exception):
    -        raise TypeError(msg % type(expected_exception))
    +    elif expected_exception is not None:
    +        msg = ("exceptions must be old-style classes or"
    +               " derived from BaseException, not %s")
    +        if isinstance(expected_exception, tuple):
    +            for exc in expected_exception:
    +                if not isclass(exc):
    +                    raise TypeError(msg % type(exc))
    +        elif not isclass(expected_exception):
    +            raise TypeError(msg % type(expected_exception))
     
         message = "DID NOT RAISE {0}".format(expected_exception)
     
    @@ -1231,6 +1233,8 @@ class RaisesContext(object):
         def __exit__(self, *tp):
             __tracebackhide__ = True
             if tp[0] is None:
    +            if self.expected_exception is None:
    +                return
                 pytest.fail(self.message)
             if sys.version_info < (2, 7):
                 # py26: on __exit__() exc_value often does not contain the
    @@ -1240,7 +1244,8 @@ class RaisesContext(object):
                     exc_type, value, traceback = tp
                     tp = exc_type, exc_type(value), traceback
             self.excinfo.__init__(tp)
    -        suppress_exception = issubclass(self.excinfo.type, self.expected_exception)
    +        suppress_exception = self.expected_exception is not None and \
    +                             issubclass(self.excinfo.type, self.expected_exception)
             if sys.version_info[0] == 2 and suppress_exception:
                 sys.exc_clear()
             return suppress_exception
    diff --git a/testing/python/raises.py b/testing/python/raises.py
    index 8f141cfa..17a30dc6 100644
    --- a/testing/python/raises.py
    +++ b/testing/python/raises.py
    @@ -126,3 +126,10 @@ class TestRaises:
             for o in gc.get_objects():
                 assert type(o) is not T
     
    +    def test_raises_non(self):
    +        with pytest.raises(None):
    +            pass
    +
    +        with pytest.raises(ValueError):
    +            with pytest.raises(None):
    +                raise ValueError


Does this really look like increasing maintenance and complexity that much?
I suggest to look concretely at things here and not further argue from first
principles and ... well, a sleepless night because of sudden child illness 
is orders of magnitudes more draining like i experienced two days ago ...

holger