tests: improve test for `nose.raises` #6521

blueyed · 2020-01-21T10:31:50Z

This should probably get transferred into a pytest.fail really, but
tests/documents the current behavior.

blueyed · 2020-01-21T11:32:31Z

testing/test_nose.py

+        [
+            "test_raises.py::test_raises_runtimeerror PASSED*",
+            "test_raises.py::test_raises_baseexception_not_caught FAILED*",
+            "test_raises.py::test_raises_baseexception_caught PASSED*",


Without the cast here it would display "test_raises.py::test_raises_runtimeerror <- /…/test_raises0/test_raises.py PASSED` here.

Hmm sorry, what "cast" you mean?

@nicoddemus https://github.com/blueyed/pytest/blob/156a41a855d28ae9f6d83545af69b51fb26f199e/src/_pytest/terminal.py#L747-L752

Since location fails to make the path relative it is absolute, and then triggers that code.

nicoddemus · 2020-01-21T11:35:32Z

LGTM but @bluetech specifically said he would like to revisit his original commit instead of reverting it, so let's wait for his review.

blueyed · 2020-01-21T11:42:57Z

src/_pytest/nodes.py

-        fspath = self.session._node_location_to_relpath(location[0])
+        fspath = location[0]
+        if not isinstance(fspath, py.path.local):
+            fspath = py.path.local(fspath)


This might be triggered via

pytest/src/_pytest/python.py

Lines 288 to 292 in 156a41a

# nose compatibility

fspath = sys.modules[obj.__module__].__file__

if fspath.endswith(".pyc"):

fspath = fspath[:-1]

lineno = compat_co_firstlineno

.

Thanks I was wondering this myself because a cursory look at nose.py did not bring anything to light.

bluetech · 2020-01-21T11:59:36Z

Thanks @blueyed for hardening the test! The test improvements looks good to me, the rest I can't quite assess ATM.

From my point of view I've come up with similar / the same like he did initially, except for that I've noticed (and IIRC mentioned in the review) that it might not be 100% correct, what we've seen now.

You did -- the revert PR message explains why I was sure the assert was safe, but I'll be more careful with these. Amusingly, this is just the sort of bugs that types help prevent, and we'll get there eventually, but the road can be bumpy :)

One thing I'm not sure about in general regarding this part of the code is: are custom Items a thing? i.e. are there plugins which create subclasses of Item? I see that at least the internal doctest plugin does this, so I assume yes. And if so, is it considered to be public API? Is reportinfo public API (not allowed to break unscrupulously)?

blueyed · 2020-01-21T12:09:54Z

One thing I'm not sure about in general regarding this part of the code is: are custom Items a thing? i.e. are there plugins which create subclasses of Item? I see that at least the internal doctest plugin does this, so I assume yes. And if so, is it considered to be public API? Is reportinfo public API (not allowed to break unscrupulously)?

Yes, I would say it is public, and therefore we need to handle strs returned from it, as does pytest itself internally still.
With your revert I've left out the adjustment to the test (test_itemreport_reportinfo) that you had to adjust, which kind of showed the issue already.
With the hardened test -vv triggers similar code.

bluetech · 2020-01-21T12:36:54Z

Yes, I would say it is public, and therefore we need to handle strs returned from it, as does pytest itself internally still.

OK. I think it will be better to use the same signature for all reportinfo functions, then. Here are some suggestions (untested...):

diff --git a/src/_pytest/doctest.py b/src/_pytest/doctest.py
index d7ca888cc..ffc1890d4 100644
--- a/src/_pytest/doctest.py
+++ b/src/_pytest/doctest.py
@@ -308,7 +308,7 @@ class DoctestItem(pytest.Item):
         else:
             return super().repr_failure(excinfo)
 
-    def reportinfo(self) -> Tuple[py.path.local, int, str]:
+    def reportinfo(self) -> Tuple[Union[str, py.path.local], int, str]:
         return self.fspath, self.dtest.lineno, "[doctest] %s" % self.name
 
 
diff --git a/src/_pytest/nodes.py b/src/_pytest/nodes.py
index 080f079cd..ab976efae 100644
--- a/src/_pytest/nodes.py
+++ b/src/_pytest/nodes.py
@@ -462,9 +462,10 @@ class Item(Node):
     @cached_property
     def location(self) -> Tuple[str, Optional[int], str]:
         location = self.reportinfo()
-        fspath = location[0]
-        if not isinstance(fspath, py.path.local):
-            fspath = py.path.local(fspath)
-        fspath = self.session._node_location_to_relpath(fspath)
+        if isinstance(location[0], py.path.local):
+            fspath = location[0]
+        else:
+            fspath = py.path.local(location[0])
+        relfspath = self.session._node_location_to_relpath(fspath)
         assert type(location[2]) is str
-        return (fspath, location[1], location[2])
+        return (relfspath, location[1], location[2])
diff --git a/src/_pytest/python.py b/src/_pytest/python.py
index 3d2916c83..82dca3bcc 100644
--- a/src/_pytest/python.py
+++ b/src/_pytest/python.py
@@ -369,7 +369,12 @@ class PyCollector(PyobjMixin, nodes.Collector):
                 if not isinstance(res, list):
                     res = [res]
                 values.extend(res)
-        values.sort(key=lambda item: item.reportinfo()[:2])
+
+        def sort_key(item):
+            fspath, lineno, _ = item.reportinfo()
+            return (str(fspath), lineno)
+
+        values.sort(key=sort_key)
         return values
 
     def _makeitem(self, name, obj):

blueyed · 2020-01-21T13:10:06Z

@bluetech

I think it will be better to use the same signature for all reportinfo functions, then.

Why? If it is known to return a str only that should be reflected in the signature, no?

bluetech · 2020-01-21T14:11:33Z

I think it adds to the confusion. When all the subclasses publicize the same interface that the base class dictates, there is less chance of errors creeping in when the types are interchanged. But you can leave it as just py.path.local if you disagree.

blueyed · 2020-01-21T14:17:28Z

@bluetech
I think it makes it clearer in general - especially with regard to removing the py.path.local instances in the long run it is good to know where only strs are returned by now already.
So I'd rather leave it as-is (i.e. more strict / describing the actual return values).

blueyed · 2020-01-23T09:44:04Z

@bluetech are you OK with it then?

This should probably get transferred into a `pytest.fail` really, but tests/documents the current behavior.

…ency"" Without changes to test_itemreport_reportinfo. This reverts commit fb99b5c. Conflicts: testing/test_nose.py

bluetech

LGTM with a couple of suggestions.

I also think we should fold this change in here as well:

diff --git a/src/_pytest/python.py b/src/_pytest/python.py
index 3d2916c83..82dca3bcc 100644
--- a/src/_pytest/python.py
+++ b/src/_pytest/python.py
@@ -369,7 +369,12 @@ class PyCollector(PyobjMixin, nodes.Collector):
                 if not isinstance(res, list):
                     res = [res]
                 values.extend(res)
-        values.sort(key=lambda item: item.reportinfo()[:2])
+
+        def sort_key(item):
+            fspath, lineno, _ = item.reportinfo()
+            return (str(fspath), lineno)
+
+        values.sort(key=sort_key)
         return values
 
     def _makeitem(self, name, obj):

The idea is that since item.reportinfo()[1] is Union[str, py.path.local], sorting by it can compare str with py.path.local. While this does the right thing, I find it distasteful, so I prefer that the str(fspath) is explicit.

src/_pytest/config/findpaths.py

src/_pytest/nodes.py

Ref: pytest-dev#6521 (review)

bluetech

Thanks, LGTM now, if CI passes.

blueyed force-pushed the harden-nose-raises branch from c86cfa7 to 0087db6 Compare January 21, 2020 10:38

blueyed commented Jan 21, 2020

View reviewed changes

blueyed requested a review from bluetech January 21, 2020 11:32

This comment has been minimized.

Sign in to view

blueyed commented Jan 21, 2020

View reviewed changes

blueyed force-pushed the harden-nose-raises branch from b9c303a to 8cd591f Compare January 21, 2020 11:53

This comment has been minimized.

Sign in to view

blueyed force-pushed the harden-nose-raises branch from 87091dd to 20d8e04 Compare January 21, 2020 12:25

This comment has been minimized.

Sign in to view

blueyed added 4 commits January 23, 2020 10:45

tests: improve test for nose.raises

8fa57c8

This should probably get transferred into a `pytest.fail` really, but tests/documents the current behavior.

Revert "Revert "Fix type errors after adding types to the py depend…

ef112fd

…ency"" Without changes to test_itemreport_reportinfo. This reverts commit fb99b5c. Conflicts: testing/test_nose.py

Node.location: handle str with _node_location_to_relpath

1350c60

typing: PyobjMixin.reportinfo, getfslineno

9c7b3c5

blueyed force-pushed the harden-nose-raises branch from 20d8e04 to 9c7b3c5 Compare January 23, 2020 09:45

bluetech reviewed Jan 23, 2020

View reviewed changes

src/_pytest/config/findpaths.py Show resolved Hide resolved

src/_pytest/nodes.py Outdated Show resolved Hide resolved

bluetech added 2 commits January 23, 2020 12:54

PyCollector.collect: use explicit cast to str

0b6258a

Ref: pytest-dev#6521 (review)

Rewrite Item.location to be clearer with regard to types

9dcdea5

bluetech approved these changes Jan 23, 2020

View reviewed changes

blueyed merged commit 6b13379 into pytest-dev:master Jan 23, 2020

blueyed deleted the harden-nose-raises branch January 23, 2020 12:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: improve test for `nose.raises` #6521

tests: improve test for `nose.raises` #6521

blueyed commented Jan 21, 2020

blueyed Jan 21, 2020

nicoddemus Jan 21, 2020

blueyed Jan 21, 2020

nicoddemus commented Jan 21, 2020

This comment has been minimized.

This comment has been minimized.

blueyed Jan 21, 2020

nicoddemus Jan 21, 2020

bluetech commented Jan 21, 2020 •

edited

Loading

blueyed commented Jan 21, 2020

This comment has been minimized.

bluetech commented Jan 21, 2020

This comment has been minimized.

blueyed commented Jan 21, 2020 •

edited

Loading

bluetech commented Jan 21, 2020

blueyed commented Jan 21, 2020

blueyed commented Jan 23, 2020

bluetech left a comment

bluetech left a comment

	# nose compatibility
	fspath = sys.modules[obj.__module__].__file__
	if fspath.endswith(".pyc"):
	fspath = fspath[:-1]
	lineno = compat_co_firstlineno

tests: improve test for nose.raises #6521

tests: improve test for nose.raises #6521

Conversation

blueyed commented Jan 21, 2020

blueyed Jan 21, 2020

Choose a reason for hiding this comment

nicoddemus Jan 21, 2020

Choose a reason for hiding this comment

blueyed Jan 21, 2020

Choose a reason for hiding this comment

nicoddemus commented Jan 21, 2020

This comment has been minimized.

This comment has been minimized.

blueyed Jan 21, 2020

Choose a reason for hiding this comment

nicoddemus Jan 21, 2020

Choose a reason for hiding this comment

bluetech commented Jan 21, 2020 • edited Loading

blueyed commented Jan 21, 2020

This comment has been minimized.

bluetech commented Jan 21, 2020

This comment has been minimized.

blueyed commented Jan 21, 2020 • edited Loading

bluetech commented Jan 21, 2020

blueyed commented Jan 21, 2020

blueyed commented Jan 23, 2020

bluetech left a comment

Choose a reason for hiding this comment

bluetech left a comment

Choose a reason for hiding this comment

tests: improve test for `nose.raises` #6521

tests: improve test for `nose.raises` #6521

bluetech commented Jan 21, 2020 •

edited

Loading

blueyed commented Jan 21, 2020 •

edited

Loading