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:
diff --git a/_pytest/python.py b/_pytest/python.py
index e763aa8..27ece6b 100644
--- a/_pytest/python.py
+++ b/_pytest/python.py
@@ -131,6 +131,7 @@ def pytest_namespace():
raises.Exception = pytest.fail.Exception
return {
'raises': raises,
+ 'does_not_raise': does_not_raise,
'approx': approx,
'collect': {
'Module': Module,
@@ -1194,6 +1195,8 @@ def raises(expected_exception, *args, **kwargs):
information.
"""
+ if expected_exception is None:
+ return does_not_raise
__tracebackhide__ = True
msg = ("exceptions must be old-style classes or"
" derived from BaseException, not %s")
@@ -1267,6 +1270,17 @@ class RaisesContext(object):
return suppress_exception
+class DoesNotRaiseContext(object):
+ def __enter__(self):
+ pass
+
+ def __exit__(self, *tp):
+ pass
+
+
+does_not_raise = DoesNotRaiseContext()
+
+
# builtin pytest.approx helper
class approx(object):
diff --git a/testing/python/raises.py b/testing/python/raises.py
index 21a6f80..b00fe28 100644
--- a/testing/python/raises.py
+++ b/testing/python/raises.py
@@ -118,6 +118,17 @@ class TestRaises(object):
for o in gc.get_objects():
assert type(o) is not T
+ @pytest.mark.parametrize('does_not_raise', [
+ pytest.does_not_raise,
+ pytest.raises(None)
+ ])
+ def test_does_not_raise(self, does_not_raise):
+ with does_not_raise:
+ pass
+
+ with pytest.raises(ValueError):
+ with does_not_raise:
+ raise ValueError
def test_raises_match(self):
msg = r"with base \d+"
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.
Cheers,
Vasily
On Fri, Mar 31, 2017 at 10:31 AM holger krekel <[email protected]> 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
> [email protected]
> https://mail.python.org/mailman/listinfo/pytest-dev
>
_______________________________________________
pytest-dev mailing list
[email protected]
https://mail.python.org/mailman/listinfo/pytest-dev
