-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
refactor callinfo to simplify ctor magic #4299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor callinfo to simplify ctor magic #4299
Conversation
CI failures appear to be unrelated. |
Will squash/rebase later today @nicoddemus PS this is roughly what i want to enable for nodes AS well |
How is #4292 meant to look with this? diff --git i/src/_pytest/runner.py w/src/_pytest/runner.py
index 0d6e3dc3..045eb72d 100644
--- i/src/_pytest/runner.py
+++ w/src/_pytest/runner.py
@@ -15,6 +15,7 @@
from .reports import CollectReport
from .reports import TestReport
from _pytest._code.code import ExceptionInfo
+from _pytest.outcomes import Exit
from _pytest.outcomes import skip
from _pytest.outcomes import Skipped
from _pytest.outcomes import TEST_OUTCOME
@@ -190,10 +191,13 @@ def check_interactive_exception(call, report):
def call_runtest_hook(item, when, **kwds):
hookname = "pytest_runtest_" + when
ihook = getattr(item.ihook, hookname)
+ reraise = (Exit,)
+ if not item.config.getvalue("usepdb"):
+ reraise += (KeyboardInterrupt,)
return CallInfo.from_call(
lambda: ihook(item=item, **kwds),
when=when,
- reraise=KeyboardInterrupt if not item.config.getvalue("usepdb") else (),
+ reraise=reraise,
) ? Or would I think this tights Exit and KeyboardInterrupt just a bit more (as it currently stands)?! |
@blueyed correct - the signaling should be layered differently |
How then? (please be more verbose) |
@blueyed my own investigation on that is not finished as the layering and the structure of calls, hooks and nodes is intertwined with this detail issue and its not clear how to layer and integrate them - i took a while to think if something comes to mind, but right no good solution seems to come to it |
But you would like to keep this PR as is? (but IMHO given the above diff it does not really make it any better than https://github.com/pytest-dev/pytest/pull/4292/files#diff-4d6d922c3b97a9eae157f8f653627b13 would have done it) |
thanks for the node, i got sidetracked ^^ |
c434f67
to
5e6553c
Compare
pushed my squash/rebase - i might have invalidated the repr while in call test and should take a look at removing that again - will do more after i fetched the kid from daycare |
Codecov Report
@@ Coverage Diff @@
## features #4299 +/- ##
============================================
+ Coverage 95.81% 95.83% +0.02%
============================================
Files 111 111
Lines 25034 25038 +4
Branches 2462 2463 +1
============================================
+ Hits 23987 23996 +9
+ Misses 737 735 -2
+ Partials 310 307 -3
Continue to review full report at Codecov.
|
5e6553c
to
1b07320
Compare
yup, had to take it out again |
testing/test_runner.py
Outdated
assert ci.when == "123" | ||
assert ci.result == 0 | ||
assert "result" in repr(ci) | ||
assert repr(ci) == "<CallInfo when='123' result: 0>" | ||
ci = runner.CallInfo.from_call(lambda: 0 / 0, "123") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test should not be changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
1b07320
to
13380e3
Compare
appveyor is codecov related |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@RonnyPfannschmidt this is conflicting with the recent merge for |
will do so either in the late afternoon or tommorow |
13380e3
to
847eace
Compare
No description provided.