From addbd3161e37edffebb2c0f6527da49b0515d1a1 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Thu, 14 Jan 2021 16:51:18 +0200 Subject: [PATCH 1/4] nose,fixtures: use the public item API for adding finalizers --- src/_pytest/fixtures.py | 10 +++------- src/_pytest/nose.py | 2 +- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 5bdee3096..43a40a864 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -543,10 +543,8 @@ class FixtureRequest: self._addfinalizer(finalizer, scope=self.scope) def _addfinalizer(self, finalizer: Callable[[], object], scope) -> None: - colitem = self._getscopeitem(scope) - self._pyfuncitem.session._setupstate.addfinalizer( - finalizer=finalizer, colitem=colitem - ) + item = self._getscopeitem(scope) + item.addfinalizer(finalizer) def applymarker(self, marker: Union[str, MarkDecorator]) -> None: """Apply a marker to a single test function invocation. @@ -694,9 +692,7 @@ class FixtureRequest: self, fixturedef: "FixtureDef[object]", subrequest: "SubRequest" ) -> None: # If fixture function failed it might have registered finalizers. - self.session._setupstate.addfinalizer( - functools.partial(fixturedef.finish, request=subrequest), subrequest.node - ) + subrequest.node.addfinalizer(lambda: fixturedef.finish(request=subrequest)) def _check_scope( self, diff --git a/src/_pytest/nose.py b/src/_pytest/nose.py index de91af85a..5bba030a5 100644 --- a/src/_pytest/nose.py +++ b/src/_pytest/nose.py @@ -13,7 +13,7 @@ def pytest_runtest_setup(item) -> None: # Call module level setup if there is no object level one. call_optional(item.parent.obj, "setup") # XXX This implies we only call teardown when setup worked. - item.session._setupstate.addfinalizer((lambda: teardown_nose(item)), item) + item.addfinalizer(lambda: teardown_nose(item)) def teardown_nose(item) -> None: From 096bae6c68840c19bdc97cdfdf99b96dbcb2c427 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Thu, 14 Jan 2021 17:05:01 +0200 Subject: [PATCH 2/4] unittest: add clarifying comment on unittest.SkipTest -> pytest.skip code I was tempted to remove it, until I figured out why it was there. --- src/_pytest/unittest.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/_pytest/unittest.py b/src/_pytest/unittest.py index cc616578b..6a90188ca 100644 --- a/src/_pytest/unittest.py +++ b/src/_pytest/unittest.py @@ -343,6 +343,10 @@ def pytest_runtest_makereport(item: Item, call: CallInfo[None]) -> None: except AttributeError: pass + # Convert unittest.SkipTest to pytest.skip. + # This is actually only needed for nose, which reuses unittest.SkipTest for + # its own nose.SkipTest. For unittest TestCases, SkipTest is already + # handled internally, and doesn't reach here. unittest = sys.modules.get("unittest") if ( unittest @@ -350,7 +354,6 @@ def pytest_runtest_makereport(item: Item, call: CallInfo[None]) -> None: and isinstance(call.excinfo.value, unittest.SkipTest) # type: ignore[attr-defined] ): excinfo = call.excinfo - # Let's substitute the excinfo with a pytest.skip one. call2 = CallInfo[None].from_call( lambda: pytest.skip(str(excinfo.value)), call.when ) From 3dde519f53b4480a3b30d58a1c71ca4505ae5ff7 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Thu, 14 Jan 2021 17:42:38 +0200 Subject: [PATCH 3/4] nose: type annotate with some resulting refactoring --- src/_pytest/nose.py | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/src/_pytest/nose.py b/src/_pytest/nose.py index 5bba030a5..16d5224e9 100644 --- a/src/_pytest/nose.py +++ b/src/_pytest/nose.py @@ -1,33 +1,35 @@ """Run testsuites written for nose.""" -from _pytest import python -from _pytest import unittest from _pytest.config import hookimpl from _pytest.fixtures import getfixturemarker from _pytest.nodes import Item +from _pytest.python import Function +from _pytest.unittest import TestCaseFunction @hookimpl(trylast=True) -def pytest_runtest_setup(item) -> None: - if is_potential_nosetest(item): - if not call_optional(item.obj, "setup"): - # Call module level setup if there is no object level one. - call_optional(item.parent.obj, "setup") - # XXX This implies we only call teardown when setup worked. - item.addfinalizer(lambda: teardown_nose(item)) +def pytest_runtest_setup(item: Item) -> None: + if not isinstance(item, Function): + return + # Don't do nose style setup/teardown on direct unittest style classes. + if isinstance(item, TestCaseFunction): + return + # Capture the narrowed type of item for the teardown closure, + # see https://github.com/python/mypy/issues/2608 + func = item -def teardown_nose(item) -> None: - if is_potential_nosetest(item): - if not call_optional(item.obj, "teardown"): - call_optional(item.parent.obj, "teardown") + if not call_optional(func.obj, "setup"): + # Call module level setup if there is no object level one. + assert func.parent is not None + call_optional(func.parent.obj, "setup") # type: ignore[attr-defined] + def teardown_nose() -> None: + if not call_optional(func.obj, "teardown"): + assert func.parent is not None + call_optional(func.parent.obj, "teardown") # type: ignore[attr-defined] -def is_potential_nosetest(item: Item) -> bool: - # Extra check needed since we do not do nose style setup/teardown - # on direct unittest style classes. - return isinstance(item, python.Function) and not isinstance( - item, unittest.TestCaseFunction - ) + # XXX This implies we only call teardown when setup worked. + func.addfinalizer(teardown_nose) def call_optional(obj: object, name: str) -> bool: From 7f989203ed58119bf63e026cbb99df274c7700d6 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Thu, 14 Jan 2021 11:58:59 +0200 Subject: [PATCH 4/4] Improve way in which skip location is fixed up when skipped by mark When `pytest.skip()` is called inside a test function, the skip location should be reported as the line that made the call, however when `pytest.skip()` is called by the `pytest.mark.skip` and similar mechanisms, the location should be reported at the item's location, because the exact location is some irrelevant internal code. Currently the item-location case is implemented by the caller setting a boolean key on the item's store and the `skipping` plugin checking it and fixing up the location if needed. This is really roundabout IMO and breaks encapsulation. Instead, allow the caller to specify directly on the skip exception whether to use the item's location or not. For now, this is entirely private. --- src/_pytest/outcomes.py | 5 +++++ src/_pytest/reports.py | 7 ++++++- src/_pytest/skipping.py | 18 +----------------- src/_pytest/unittest.py | 6 ++---- 4 files changed, 14 insertions(+), 22 deletions(-) diff --git a/src/_pytest/outcomes.py b/src/_pytest/outcomes.py index 8f6203fd7..756b4098b 100644 --- a/src/_pytest/outcomes.py +++ b/src/_pytest/outcomes.py @@ -58,9 +58,14 @@ class Skipped(OutcomeException): msg: Optional[str] = None, pytrace: bool = True, allow_module_level: bool = False, + *, + _use_item_location: bool = False, ) -> None: OutcomeException.__init__(self, msg=msg, pytrace=pytrace) self.allow_module_level = allow_module_level + # If true, the skip location is reported as the item's location, + # instead of the place that raises the exception/calls skip(). + self._use_item_location = _use_item_location class Failed(OutcomeException): diff --git a/src/_pytest/reports.py b/src/_pytest/reports.py index d2d7115b2..303f731dd 100644 --- a/src/_pytest/reports.py +++ b/src/_pytest/reports.py @@ -324,7 +324,12 @@ class TestReport(BaseReport): elif isinstance(excinfo.value, skip.Exception): outcome = "skipped" r = excinfo._getreprcrash() - longrepr = (str(r.path), r.lineno, r.message) + if excinfo.value._use_item_location: + filename, line = item.reportinfo()[:2] + assert line is not None + longrepr = str(filename), line + 1, r.message + else: + longrepr = (str(r.path), r.lineno, r.message) else: outcome = "failed" if call.when == "call": diff --git a/src/_pytest/skipping.py b/src/_pytest/skipping.py index c7afef5db..1ad312919 100644 --- a/src/_pytest/skipping.py +++ b/src/_pytest/skipping.py @@ -230,8 +230,6 @@ def evaluate_xfail_marks(item: Item) -> Optional[Xfail]: return None -# Whether skipped due to skip or skipif marks. -skipped_by_mark_key = StoreKey[bool]() # Saves the xfail mark evaluation. Can be refreshed during call if None. xfailed_key = StoreKey[Optional[Xfail]]() @@ -239,9 +237,8 @@ xfailed_key = StoreKey[Optional[Xfail]]() @hookimpl(tryfirst=True) def pytest_runtest_setup(item: Item) -> None: skipped = evaluate_skip_marks(item) - item._store[skipped_by_mark_key] = skipped is not None if skipped: - skip(skipped.reason) + raise skip.Exception(skipped.reason, _use_item_location=True) item._store[xfailed_key] = xfailed = evaluate_xfail_marks(item) if xfailed and not item.config.option.runxfail and not xfailed.run: @@ -292,19 +289,6 @@ def pytest_runtest_makereport(item: Item, call: CallInfo[None]): rep.outcome = "passed" rep.wasxfail = xfailed.reason - if ( - item._store.get(skipped_by_mark_key, True) - and rep.skipped - and type(rep.longrepr) is tuple - ): - # Skipped by mark.skipif; change the location of the failure - # to point to the item definition, otherwise it will display - # the location of where the skip exception was raised within pytest. - _, _, reason = rep.longrepr - filename, line = item.reportinfo()[:2] - assert line is not None - rep.longrepr = str(filename), line + 1, reason - def pytest_report_teststatus(report: BaseReport) -> Optional[Tuple[str, str, str]]: if hasattr(report, "wasxfail"): diff --git a/src/_pytest/unittest.py b/src/_pytest/unittest.py index 6a90188ca..719eb4e88 100644 --- a/src/_pytest/unittest.py +++ b/src/_pytest/unittest.py @@ -29,7 +29,6 @@ from _pytest.python import Class from _pytest.python import Function from _pytest.python import PyCollector from _pytest.runner import CallInfo -from _pytest.skipping import skipped_by_mark_key if TYPE_CHECKING: import unittest @@ -150,7 +149,7 @@ def _make_xunit_fixture( def fixture(self, request: FixtureRequest) -> Generator[None, None, None]: if _is_skipped(self): reason = self.__unittest_skip_why__ - pytest.skip(reason) + raise pytest.skip.Exception(reason, _use_item_location=True) if setup is not None: try: if pass_self: @@ -256,9 +255,8 @@ class TestCaseFunction(Function): def addSkip(self, testcase: "unittest.TestCase", reason: str) -> None: try: - skip(reason) + raise pytest.skip.Exception(reason, _use_item_location=True) except skip.Exception: - self._store[skipped_by_mark_key] = True self._addexcinfo(sys.exc_info()) def addExpectedFailure(