[pytest-dev] [3.1 feature] "assert not raise exception" helper: opinions about the name
Bruno Oliveira
nicoddemus at gmail.com
Fri Mar 31 07:24:34 EDT 2017
Hi all,
On Fri, Mar 31, 2017 at 7:13 AM Vasily Kuznetsov <kvas.it at gmail.com> wrote:
> Hi Holger and Ronny,
>
> I see merit in both of your points. All those "is not None" checks in
> between other logic and the proposed "raises unless the argument is None"
> semantics of pytest.raises do increase complexity (I'm not qualified to
> predict whether or not this increase is significant in terms of further
> maintenance though) but at the same time "pytest.raises(None)" API is
> convenient in some cases. What if we do something like this:
> ...
> The "is not None" checks are gone from the main logic and both APIs are
> available. Or if we would rather not have more than one way to do it, we
> can drop "does_not_raise" but otherwise still keep it a separate context.
>
> Seems like if we can agree on the API, a not-complexity-increasing option
> of implementation is possible.
>
Thanks for that Vasily!
I sympathize with Ronny's points about the problem of apparently harmless
changes on the outside causing increase in internal complexity being a
problem for maintenance down the road. We should also strive to refactor
the known pain points of the internal code (mark handling and fixture cache
come readily to mind), and Ronny is the main champion in that regard.
I agree with Holger's sentiment of aiming for the user interface we feel is
better, but we must then look at the implementation and weigh the cost of
maintenance and increased complexity and step back if we find it too costly.
Now for the specific case of pytest.raises(None), I believe we can strike a
good balance between a nice user interface and minimal internal impact like
Vasily proposes, unless Ronny or others feel that pytest.raises(None) is
not a good interface for this functionality.
My 2 cc.
Cheers,
Bruno.
> Cheers,
> Vasily
>
> On Fri, Mar 31, 2017 at 10:31 AM holger krekel <holger at merlinux.eu> wrote:
>
> 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
>
> _______________________________________________
> pytest-dev mailing list
> pytest-dev at python.org
> https://mail.python.org/mailman/listinfo/pytest-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/pytest-dev/attachments/20170331/87ce346a/attachment.html>
More information about the pytest-dev
mailing list