pytest-dev
Handle exceptions in hookwrapper post stage (#244)
Make hook wrappers more reliable by calling remaining tear down stages even an exception thrown in some wrapper during this step. Currently such code could be called too late when garbage collector deletes generator objects.
Fixes #244
Maybe I have missed something related to calling convention of hook wrappers, so feel free to decline.
Interesting. The "exception-in-finalizer" problem is a classic issue with excpetions. The two approaches I'm aware of are indeed either to suppress or crash. Python itself chooses to suppress with a print message if an exception is raised in a __del__ (see warning here). Pluggy currently crashes effectively.
Generally, I think finalizers should never fail, so if they raise, it is a bug, and a crash is not a bad way to signal that. I wonder if in your case, you did not have a way to fix the problem at the source?
Regarding the patch itself (choosing to suppress), you made so that if a finalizer raises, the outcome changes to that exception for all subsequent finalizers. Why do you think this is the better behavior? (The alternative is to keep calling the other finalizers with the original Result).
Pluggy currently crashes effectively.
No, in the case of "crash", code after yield would not be executed at all. Currently it is called by garbage collector causing undefined behavior in respect to execution order. Try the added tests without fix and notice that "finally" appears before "m1 cleanup". That is what I am trying to fix.
AssertionError: assert ['m1 init', '... 'm1 cleanup'] == ['m1 init', '...p', 'finally']
E At index 3 diff: 'finally' != 'm1 teardown'
E Right contains one more item: 'finally'
E Full diff:
E - ['m1 init', 'm2 init', 'm2 raise', 'm1 teardown', 'm1 cleanup', 'finally']
E + ['m1 init', 'm2 init', 'm2 raise', 'finally', 'm1 cleanup']
If your prefer to skip code after yield then gen.throw(GeneratorExit()) should be sent to remaining generators to force execution of code in finally clauses after an exception is encountered.
I do not consider remaining part of code as finalizers. It is just code after calling of inner hook, so I do not see any problem with exceptions just as in ordinary functions. My approach implements proper stack unwind and it is what I expect in response to exception.
@maxnikulin thanks for this work.
Currently it is called by garbage collector causing undefined behavior in respect to execution order.
I believe this is due to generator.close() being called in __del__ of generators yes?
I do not consider remaining part of code as finalizers. It is just code after calling of inner hook, so I do not see any problem with exceptions just as in ordinary functions.
I believe this logic is correct.
If your prefer to skip code after yield then gen.throw(GeneratorExit()) should be sent to remaining generators to force execution of code in finally clauses after an exception is encountered.
This might actually be the approach we want instead. I have to read through #244 on a clear mind to be more sure of this. ~I am thinking that calling generator.close() as part of teardown is the right way to ensure finally: code is executed. This would make default behavior more along the lines of enforcing async_generator.aclosing() behavior.~ <- it isn't nor is it necessary unless the user writes an incorrect wrapper with >1 yield.
Disregard, I had a misunderstanding of how StopIteration works.
~@maxnikulin yes this seems very similar to issues with async generators that you will often have to battle with without explicit teardown semantics.~
~See these for the analogs in async land:~
- https://async-generator.readthedocs.io/en/latest/reference.html#garbage-collection-hooks
-
discussion regarding
trioissues with this and the discussion around a solution - PEP for deterministic iterator cleanup
Actually I could not get your point. Why do you prefer crash-like behavior to stack unwind? With gen.close() instead of passing exception except clauses may be skipped but finally blocks are still executed. It is neither C++ abort() nor usual exception unwind.
I do not think that the problem is similar to async generator. Pluggy effectively simulates call stack of hook wrappers through sequential calls of generators. It is simply missed error handler that should propagate exception backward to each "stack frame". If you have hand crafted call stack you must implement your own stack unwind.
I believe that hook wrappers are similar to nested calls of regular functions. For convenience they are implemented using generators but I expect behavior as close to regular functions as possible
in respect to try/except/else/finally statements
def hook_with_result(arg):
return arg + 42
def inner_wrapper(arg)
result = hook_with_result(arg)
if result > 50:
# Code after yield in the case of geherators.
# What is the problem with exception here?
raise ValueError("Too much")
return result*2
def outer_hook(arg)
try:
return inner_hook(arg + 10)
except ValueError:
# Why it should not be possible to
# to handle exception and return result here
# or convert it to another exception?
return 13
print(outer_hook(100))
I even suppose that it should be more reliable to send exception into hook wrapper using gen.throw() instead of putting it into outcome container. It ensures that the exception is risen inside the hook wrapper even if explicit call of outcome.get_result() is missed.
So if the word "finalizer" is not used for the code after yield in hook wrapper, fair propagation of exception (that can be suppressed or converted into another type) looks more consistent with stack unwind for regular functions in comparison to just using gen.close() for remaining generators.
I believe this is due to
generator.close()being called in__del__of generators yes?
I agree with you, but I do not see __del__ in traceback.print_stack()
@maxnikulin sorry maybe I wasn't being clear enough.
Why do you prefer crash-like behavior to stack unwind? With gen.close() instead of passing exception except clauses may be skipped but finally blocks are still executed.
I think that in the non-error case we should be doing this regardless such that finally: blocks are executed when the hook call completes not when the garbage collector calls .close(). This might also provide for easier support of yield from (not sure if we'll ever use it or not). This signal indicates that the generator is truly complete and can't be used further. Currently we are only calling .send(outcome) once ~so finally: blocks aren't actually being executed until GC time~ <- definitely a wrong assumption.
I even suppose that it should be more reliable to send exception into hook wrapper using gen.throw() instead of putting it into outcome container. It ensures that the exception is risen inside the hook wrapper even if explicit call of outcome.get_result() is missed.
Yes, for the error case this is 100% the the way I would expect exceptions to be propagated/handled. Sorry if I didn't restate this but I thought it was implied in your comment when I commented with:
I believe this logic is correct.
which was in reference to:
I do not consider remaining part of code as finalizers. It is just code after calling of inner hook, so I do not see any problem with exceptions just as in ordinary functions.
I'm not sure how else you accomplish this without using generator.throw() :wink:
Further,
I do not think that the problem is similar to async generator.
I would view the situation of nested generators that need to all be unwound as similar to an async generator needing to be unwound from within a coroutine but it's really not that important. The main point was that Pep 533 (which is not async specific) goes over the issue of "generator lifetimes" as something that should be explicit as opposed to handled arbitrary in the future by the GC.
In summary, I think we should make 2 changes:
-
generator.close()should always be called -
generator.throw()should be used to propagate wrapper errors.
Actually @oremanj would mind commenting on this discussion given you opened #244 :smile_cat:
@goodboy, I am confused a bit by your words, but situation becomes clearer if you missed that generator, that follows hook wrapper convention (namely it performs just one yield), does not have a problem with deleter and garbage collector. It executes all its code including finally: blocks and returns from the function. So generator.close() should be optional. Have I missed something in respect to completed generators? Are there any problems with pypy or jython that have different garbage collector strategy?
Another source of confusion is (warning! context is severely narrowed down, see the full comment):
@goodboy
@maxnikulin
If your prefer to skip code after yield...
This might actually be the approach we want instead.
@bluetech asked why I have chosen sending exception instead of original result inside the outcome. That is why, @goodboy, I decided that that you suggest the opposite thing, namely just call gen.close() instead of delivering of the actual exception. If your point is to call gen.close() in addition to sending raised exception to the outer hook wrappers then I completely agree with you.
Concerning gen.throw() instead of boxing the exception into outcome container, it is harder for me to estimate consequences. 3rd party pytest plugins might be broken by such change. And it requires more efforts since documentation has to be updated in pluggy and pytest.
In python-3 it is possible to avoid outcome container completely, generators are allowed to return a value that becomes StopIteration.value attribute. Unfortunately, unlike current pull request, the change is not backward compatible.
To summarize:
- I still believe that there is nothing bad in exceptions that is raised after
yieldand delivering exception to outer hook wrapper throughoutcomeis the most reasonable approach for the minimal change fix - I do not see any problem in non-error case. Generator function runs till it returns, so there should be no work for reference counting garbage collector.
- Thank you for drawing attention to
gen.close(), definitely it should be added. I overlooked the case when finalizer is not executed if the generator having more than oneyield. I believed that_raise_wrapfaildoes all necessary job. I will try to add another unit test for such case.
Unit test is not a problem
diff --git a/testing/test_multicall.py b/testing/test_multicall.py
index d4521cd..af986e9 100644
--- a/testing/test_multicall.py
+++ b/testing/test_multicall.py
@@ -156,15 +156,27 @@ def test_hookwrapper_not_yield():
def test_hookwrapper_too_many_yield():
+ out = []
+
@hookimpl(hookwrapper=True)
def m1():
- yield 1
- yield 2
+ try:
+ yield 1
+ yield 2
+ finally:
+ out.append("cleanup")
with pytest.raises(RuntimeError) as ex:
- MC([m1], {})
+ try:
+ MC([m1], {})
+ finally:
+ out.append("finally")
assert "m1" in str(ex.value)
assert (__file__ + ":") in str(ex.value)
+ assert out == [
+ "cleanup",
+ "finally",
+ ]
@pytest.mark.parametrize("exc", [ValueError, SystemExit])
However raise ... from if gen.close() raises exception is not trivial in python-2:
https://github.com/PythonCharmers/python-future/blob/master/src/future/utils/init.py#L449
have not look in six implementation yet.
I am confused a bit by your words
Yeah, my bad I'm probably not being clear but I also think the details of how errors can be handled in generators is nuanced.
That is why, @goodboy, I decided that that you suggest the opposite thing, namely just call gen.close() instead of delivering of the actual exception.
No, and again my apologies for not being clear, I was trying to say that we should always be calling generator.close() instead of just a single .send() and expecting a StopIteration.
Thank you for drawing attention to gen.close(), definitely it should be added. I overlooked the case when finalizer is not executed if the generator having more than one yield. I believed that _raise_wrapfail does all necessary job. I will try to add another unit test for such case.
Yes this is exactly one reason why. I also don't see a reason to not explicitly close the wrapper when it is finished being used instead of waiting for it to be GCed. If you look at the introduction of generator.close() there is also the mention:
Also, in the CPython implementation of this PEP, the frame object used by the generator should be released whenever its execution is terminated due to an error or normal exit. This will ensure that generators that cannot be resumed do not remain part of an uncollectable reference cycle. This allows other code to potentially use close() in a try/finally or with block (per PEP 343) to ensure that a given generator is properly finalized.
So there may be a memory consideration as well as just adhering to good resource de-allocation practices.
I decided that that you suggest the opposite thing, namely just call gen.close() instead of delivering of the actual exception.
Yeah, again my apologies, that's not what I meant even though I could see it being read that way.
Concerning gen.throw() instead of boxing the exception into outcome container, it is harder for me to estimate consequences. 3rd party pytest plugins might be broken by such change. And it requires more efforts since documentation has to be updated in pluggy and pytest.
pluggy is supposed to evolve on its own which is why we version and release it outside of pytest and friends and this may be a good candidate feature for release in a 1.0. Unfortunately, a bunch of the core devs are on leave atm so when/if they return it would be good to get feedback on this.
If your point is to call gen.close() in addition to sending raised exception to the outer hook wrappers then I completely agree with you.
Yes this my thinking and I think we should be using modern python features over our home brewed _Result if we can.
I still believe that there is nothing bad in exceptions that is raised after yield and delivering exception to outer hook wrapper through outcomeis the most reasonable approach for the minimal change fix
The main problem with this is masking any error that was raised in the hook call that downstream wrappers might be expecting to handle. Being able to distinguish between errors from the hook call and errors from wrappers is likely desirable.
Unit test is not a problem
Huh, interesting. I would have not expected that to work?
[nav] In [1]: def gen():
...: try:
...: yield 10
...: yield 20
...: finally:
...: print('finalizer')
[nav] In [2]: g = gen(); next(g)
Out[2]: 10
[nav] In [3]: try:
...: g.send(None)
...: print('uh oh >1 yield')
...: raise ValueError
...: except StopIteration:
...: print('err: {}'.format(str(err)))
uh oh >1 yield
---------------------------------------------------------------------------
ValueError Traceback (most recent call last)
<ipython-input-3-5f6680eec8f0> in <module>
2 g.send(None)
3 print('uh oh >1 yield')
----> 4 raise ValueError
5 except StopIteration:
6 print('err: {}'.format(str(err)))
ValueError:
When I run this code the finally: is never run. I'm not actually sure if we care since in this case the wrapper is implemented incorrectly anyway?
I have added generator.close() calls. However generator could effectively resist attempts to close it.
However generator could effectively resist attempts to close it.
@maxnikulin of course but this is not something we have control over since it's built into Python :)
I believe the same issue would be present despite generator.close() being called from __del__().
This is looking good but I would like some feedback from other core devs. I also would be partial to finally getting in #59 first so that we can drop some code here before we add more.
My opinion on the current PR is that it's problematic.
In https://github.com/pytest-dev/pluggy/pull/257#issuecomment-619588937 @maxnikulin nicely explains their point of view - that the hookwrappers are analogous to nested function calls, and hence that an exception thrown in the "after yield" section should propagate to the upper wrappers and eventually to the hook caller.
But as mentioned before I somewhat disagree with this view point. From my experience in pytest, I do view the "before-yield" and "after-yield" sections as setup/teardown sections. And specifically, if a setup or teardown code throws, I consider it a bug, and I do not expect it to propagate.
Considering the issue at hand #244, I think that the proper fix is to introduce an explicit result.force_exception(exc) method on _Result, not to allow the wrapper to raise and using what it raised as the exception of the hook. This is by analogy to result.force_result(), which is required if the hook wants to alter the return value of the hook -- if the hookwrapper actually returns, that is discarded.
The idea in #260 would change the perspective to that of @maxnikulin, but in current pluggy I expect that an explicit action on the _Result is needed.
So I mentioned in my previous comment, my preference is to "crash" - in practice it would mean throwing some PluggyHookwrapperException wrapping the hookwrapper's exception and propagating that (so it can't be mistaken with an actual exception thrown by the hook or forced by a hookwrapper -- it is a bug akin to AssertionError).
Regarding the discussion on gen.close(), I think it definitely makes sense to dispose of the generator explicitly rather than rely on GC. I haven't followed all of the details discussed or the diff, but is this something that can be split to a separate PR, without changing the exception propagation behavior?
I should note that this PR is definitely an improvement on the current pluggy, because current pluggy also propagates, but just doesn't do proper teardown, from what I understand. But I do not see this as a fix for #244, and would prefer that pluggy changed to a "crash" behavior as a separate manner.
@bluetech As the person who filed #244, I do see this PR as a fix for #244. :-)
AFAICT, the reason why hooks use this _Result object, instead of following @contextmanager style semantics where the inner result is directly sent or thrown in at the yield, is to work around Python 2.x not allowing return in a generator. _Result.get_result() will throw an exception if the inner hook raised an exception, so in practice there are likely to be lots of hook wrappers that raise exceptions if the inner hook did. And pytest in practice uses at least some hooks in ways that raise exceptions if the user's test failed, so exceptions-in-hooks are not an exotic or rare concern. I got to #244 by way of trying to write a pytest plugin that would suppress certain exceptions thrown by tests.
I think insisting on a new _Result.force_exception() API, and declaring any exception that escapes from a hookwrapper to be a bug in the hookwrapper, would introduce unnecessary churn and backwards compatibility problems due to the existing get_result() semantics. The backcompat issues could maybe be resolved by noticing whether the exception that escapes the hookwrapper is the same one originally in the _Result or not. But just using the exception that escapes in all cases seems strictly better to me, especially since exception chaining means the original failure won't be obscured.
Considering
try:
(yield).get_result()
except KeyError:
raise ValueError
versus
try:
outcome = yield
outcome.get_result()
except KeyError as exc:
new_exc = ValueError()
new_exc.__context__ = exc
outcome.force_exception(new_exc)
does the latter one really look preferable to you?
Thanks for the comments @oremanj!
AFAICT, the reason why hooks use this _Result object, instead of following
@contextmanagerstyle semantics where the inner result is directly sent or thrown in at the yield, is to work around Python 2.x not allowing return in a generator.
Yes, I don't know the history but that's probably why.
Old Tornado code used a hack for this -- instead of return value, they said to raise Return(value) :slightly_smiling_face:
_Result.get_result() will throw an exception if the inner hook raised an exception, so in practice there are likely to be lots of hook wrappers that raise exceptions if the inner hook did.
That's a good point. However, are you aware of any plugins which try to change the exception raised, or even suppress it? Looking at pytest itself, and some corpus of 70 plugins I have checked out, I only found one case in pytest-asyncio, but looks like it was just removed as well.
So because the exception is not modified, it is a question of interpretation whether the exception is propagated, or the exception of the result is used.
The pluggy docs currently only say
"If any hookimpl errors with an exception no further callbacks are invoked and the exception is packaged up and delivered to any wrappers before being re-raised at the hook invocation point"
I'd say this leaves the behavior of a hook raising unspecified, and the feature of altering the exception is currently unsupported.
And pytest in practice uses at least some hooks in ways that raise exceptions if the user's test failed, so exceptions-in-hooks are not an exotic or rare concern.
I'd be interested to see some examples.
But just using the exception that escapes in all cases seems strictly better to me, especially since exception chaining means the original failure won't be obscured.
I think it is not strictly better, because a buggy hookwrapper, which has an accidental IndexError or such, would be indistinguishable from a hookwrapper which explicitly wants to override the exception.
does the latter one really look preferable to you?
Of course the first one looks much nicer. But it is just not analogous with current pluggy which is
result = yield
result.force_result(result.get_result() + 10)
not
return (yield) + 10
I think one way to resolve this question would be:
- Drop Python 2 support (pluggy 1.0, #140)
- Drop
_Result(#260) - Implement & document proper semantics without
_Result-- in this case raising exception in a hookwrapper makes sense to propagate, according to @maxnikulin "nested function calls" POV. - Add back
_Resultfor backward compat, implemented on top of the no-Result code.
However, are you aware of any plugins which try to change the exception raised, or even suppress it?
I've written one -- it was for an internal project at my previous employer, so I can't show it here, but I don't think it's an exotic use case. I wound up needing to assign to result._excinfo directly. This is the same issue that led me to file #244 in the first place.
I'd be interested to see some examples.
pytest_runtest_call is a pytest hook that raises whatever exception the test failed with. This is the most natural thing to hook if you're trying to suppress or modify some exceptions in a test.
I think it is not strictly better, because a buggy hookwrapper, which has an accidental IndexError or such, would be indistinguishable from a hookwrapper which explicitly wants to override the exception.
How is a buggy hookwrapper meaningfully different from a buggy hook that's not a wrapper? Yeah, if your hookwrapper has a bug that results in it throwing an exception, it might not be clear whether that exception was thrown intentionally or not. So you look at the traceback and figure it out. Same as you would do if your non-wrapper hook threw an exception. :-)
Of course the first one looks much nicer. But it is just not analogous with current pluggy
I'd support allowing a return statement to override the result value too. Then both will be analogous. :-) As I mentioned in a comment on #260, this would allow hookwrappers can treat (yield).get_result() as a single piece of syntax which produces an unwrapped value or raises an exception, which can be modified by the wrapper in the obvious way without recourse to specialized APIs.
I think removing _Result entirely is an unnecessary amount of churn just for achieving a fix to #244. Maybe it's desirable for other reasons -- I don't have an opinion on that -- but it would be a shame to see the fix in this PR, which is a great targeted solution to #244, have to wait on a larger rearchitecting like that.
Frankly speaking, I completely missed that hook wrappers could be used for setup and tear down.
So it turns out that hook wrappers could have 2 different variants of usage with distinct expected behavior
- Setup and teardown. I still against "crash" scenario. I believe that all tear down parts still must be invoked however result should not be modified by the hook processing code. All exceptions in teardown code should be reported to the caller somehow. I think that completely swallowing of tear down exception is a bad idea. On the other hand
AssertionErrormight have higher priority than failure of tear down code. - Real wrapper of test function that is able to modify result. I would rather considered some additional checks that report failure despite test function completes successfully. This case fair stack unwind is desired with delivering of exception to outer wrappers of the similar kind.
On the other hand I am tempting to say that tear down code must be in a finally block, so strategy of propagation of exceptions must not affect clean-up parts. From such point of view
setup/teardown wrappers are not a special case (the price is changes of behavior of the existing code).
_Result.force_exception() looks like a reasonable backward compatible solution. By default exception in hook wrapper post-yield code does not affect outcome for outer wrappers however
after all wrappers are finished such exception should be re-thrown. Wrappers that needs to modify outcome could set desired result or exception.
As an alternative, other than True values could be used for the hookwrapper argument to distinguish "setup/teardown" and "real wrapper" cases. Mutually exclusive keword arguments could be a source of confusion.
Concerning docs
and the exception is packaged up and delivered to any wrappers before being re-raised
I did not noticed unspecified behavior and expected that it is delivered to all wrappers called before. Ability to alter the exception is in some contradiction with the statement. On the other hand I would expect consistency in respect to modification of return value and exception.
I have faced the issue playing with pytest. I was trying to achieve a convenient implementation of expect(), a "soft" assertion similar to google tests EXPECT_EQ. It is necessary to convert successful result to failure if expect argument evaluates to false.
To summarize:
- I think that dropping of python-2 support is an independent task
-
_Result.force_exception()could be added in a separated pull request or in this one. - This pull request could be adjusted to avoid implicit propagation of exceptions.
- Current variant is still more clear in respect to which exception becomes the outcome of the call.
btw @maxnikulin I fixed the flake8 errors in #147 so if you rebase (or cherry-pick the relevant commit) from that you should get a clean run.
I also still need to go through the most recent discussion.
Frankly speaking, I completely missed that hook wrappers could be used for setup and tear down.
In practice, I suspect most wrappers that want setup/teardown behavior just don't look at the _Result object that gets sent to them. If you want to do wrapping on the result, then you need to call get_result() at some point, and that will raise the exception if there was one -- there's in fact no way (AFAICT) to get the exception without raising it. This feature of the current semantics of _Result is a lot of why I think we should let hookwrappers propagate an exception just by raising it, no force_exception() needed.
@bluetech wrote:
But as mentioned before I somewhat disagree with this view point. From my experience in pytest, I do view the "before-yield" and "after-yield" sections as setup/teardown sections. And specifically, if a setup or teardown code throws, I consider it a bug, and I do not expect it to propagate.
I have just realized that both hook wrappers examples in pluggy itself and in pytest demonstrate functions that modify outcome (result). I tend to extrapolate that it should be applicable to suppressing or vice versa generating an exception https://pluggy.readthedocs.io/en/latest/#wrappers https://docs.pytest.org/en/latest/writing_plugins.html#hookwrapper-executing-around-other-hooks
If de-facto most wrappers are used for additional setup and tear down then it is worth illustrating such case in the docs.
Exception in tear down code is really a bug but such bug should not spoil exception raised by a test and should not affect outer wrappers. To achieve the former (at least in python-3) exception delivery into the buggy wrapper should be done even in a more aggressive form (gen.throw()). To ensure the latter tear down code bust be below finally: and it is a bug in the wrapper if such recommendation is neglected. It should be reflected in examples in docs just in the @contextmanager docs
https://docs.python.org/3/library/contextlib.html#contextlib.contextmanager
It is harder to achieve reliable exception chaining if exceptions are not delivered to outer wrappers. It require additional code and could be fragile leading to less useful failure reports in pytest.
So I do not see any advantages in swallowing of post-yield exceptions. Please, share you ideas how they should be reported to the caller without lost of their context.
@goodboy wrote:
btw @maxnikulin I fixed the flake8 errors in #147 so if you rebase (or cherry-pick the relevant commit) from that you should get a clean run.
Thank you, but I am afraid a new version of flake8 might be released before we managed to negotiate.
@oremanj wrote:
I think we should let hookwrappers propagate an exception just by raising it, no force_exception() needed.
You are right, _Result.force_exception() adds no value. It does not affect "modify result" vs. "setup/tear down" question.
Rebase is not a problem, I am waiting for a decision if propagation of exception from post-yield code is acceptable.
@maxnikulin feel free to rebase onto master since it will simplify this PR avoiding having to modify the _LegacyMultiCall stuff :smile:
See https://github.com/pytest-dev/pytest-subtests/pull/27/files#diff-d53084e24bba305b8e814d2a0a513397R248 for an example where exception in post-yield code could be useful to mark a pytest as failed one
So we've removed Python 2 support in master, and gearing up to a 1.0.0 release (which pytest 6.0.0 will hopefully depend on). So I'm trying to hookwrap my mind around this again (sorry for the verbosity!).
For the sake of understanding, let's take a simple hookspec, not using firstresult, which has 3 non-wrapper impls and 3 wrapper impls.
Code for the example
from pluggy import HookspecMarker, HookimplMarker, PluginManager
hookspec = HookspecMarker("example")
hookimpl = HookimplMarker("example")
pm = PluginManager("example")
class Hooks:
@hookspec
def myhook(self, arg):
pass
class Plugin1:
@hookimpl
def myhook(self, arg):
print("call1()")
class Plugin2:
@hookimpl
def myhook(self, arg):
print("call2()")
class Plugin3:
@hookimpl()
def myhook(self, arg):
print("call3()")
class Plugin4:
@hookimpl(hookwrapper=True)
def myhook(self, arg):
print("setup4()")
yield
print("teardown4()")
class Plugin5:
@hookimpl(hookwrapper=True)
def myhook(self, arg):
print("setup5()")
yield
print("teardown5()")
class Plugin6:
@hookimpl(hookwrapper=True)
def myhook(self, arg):
print("setup6()")
yield
print("teardown6()")
pm.add_hookspecs(Hooks)
pm.register(Plugin1())
pm.register(Plugin2())
pm.register(Plugin3())
pm.register(Plugin4())
pm.register(Plugin5())
pm.register(Plugin6())
pm.hook.myhook(arg=10)
This prints:
setup6()
setup5()
setup4()
call3()
call2()
call1()
teardown4()
teardown5()
teardown6()
I tried to formulate the current semantics, and this is what I came up with:
results, exc = [], None
try:
setup6(arg)
except BaseException as e:
exc = e
else:
try:
setup5(arg)
except BaseException as e:
exc = e
else:
try:
setup4(arg)
except BaseException as e:
exc = e
else:
try:
results.append(call3(arg))
results.append(call2(arg))
results.append(call1(arg))
except BaseException as e:
exc = e
teardown4(arg, results, exc)
teardown5(arg, results, exc)
teardown6(arg, results, exc)
return results, exc
This PR will change it to this:
results, exc = [], None
try:
setup6(arg)
try:
setup5(arg)
try:
setup4(arg)
try:
results.append(call3(arg))
results.append(call2(arg))
results.append(call1(arg))
except BaseException as e:
exc = e
teardown4(arg, results, exc)
except BaseException as e:
exc = e
teardown5(arg, results, exc)
except BaseException as e:
exc = e
teardown6(arg, results, exc)
except BaseException as e:
exc = e
return results, exc
I'd say the current behavior is the worst of all worlds. It both discards the other teardowns, and propagates the exception as-is.
In particular, I'm convinced by @oremanj's point about the common usage of the
result = (yield).get_result()
pattern without any try/finally, which is very common in pytest, also in its docs, and is currently buggy. We can't break that, and checking that the exception is the same is really ugly.
So my conclusion is that this PR is a good idea. However, I feel that the discrepancy with how a hookwrapper would change the return value (outcome.force_result) vs. how it changes the exception (raise it with regular control flow) would not be ideal. Therefore, I think we should also let hookwrappers return the result(s) instead of using force_result, i.e. these semantics:
results, exc = [], None
try:
setup6(arg)
try:
setup5(arg)
try:
setup4(arg)
try:
results.append(call3(arg))
results.append(call2(arg))
results.append(call1(arg))
except BaseException as e:
exc = e
results = teardown4(arg, results, exc)
except BaseException as e:
exc = e
results = teardown5(arg, results, exc)
except BaseException as e:
exc = e
results = teardown6(arg, results, exc)
except BaseException as e:
exc = e
return results, exc
This can be done with the existing hookwrapper=True already, now that we are Python 3 only. This is also what #260 will do, so the only difference between hookwrapper=True wrappers and new-style wrappers would be that the get_result() in result = (yield).get_result() is basically implied.
WDYT? We can leave the return handling for another PR but would like to know if that sounds good to everyone.
In the meantime @maxnikulin I think you can safely rebase now.
The branch currently have no merge/rebase conflicts with master.
I do not think that propagation of exception will cause real additional problems since currently finalizing part of outer wrappers is not invoked at all in the case of an error in an inner wrapper. Currently teardown call is just missing, with these changes (yield).get_result() will generate exception. Nothing will become worse.
I believe that replacing result by exception is the only straightforward way following least surprise principle. I do not see a clear way to distinguish exceptions generated by a normal hook and by an inner wrapper that is why I expect more confusion with result array and exception from teardown passed simultaneously or from hiding teardown exceptions from outer wrappers.
Recently I have tried to improve the pytest-subtests plugin. I would not mind to have ability to make a list of exceptions the outcome of the test with failed subtests. But it should be handled inside pytest hooks, not in pluggy. The complication is that the list of exceptions could be generated by a single hook(wrapper). We could speculate that generator interface in principle allows list of outcomes from a single wrapper but it should not be considered seriously. It is even possible to allow ordinary hooks generate list of mixed result/exception outcomes (something like StopIteration could stop processing of sequence of regular hooks).
So there are plenty of possibilities to mix results and exceptions in hook call outcome. For me the most logical one is to replace result by exception in teardown handler as an unambiguous sign that the call as a whole has failed (either in a reglar hook or in a wrapper)