Skip to content
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

Fix internal assert failure regression in 5.3.4 #6518

Merged
merged 1 commit into from
Jan 20, 2020

Conversation

bluetech
Copy link
Member

@bluetech bluetech commented Jan 20, 2020

This reverts commit 930a158 (pr #6511).

Fixes #6517.

The assert is failing on a real test suite. Basically, I was sure the assert was safe because the _node_location_to_relpath cache that the value is passed into is only designed to work with py.path.locals, not strs, and code-wise if given an str, it would fail. Or so I thought. Turns out several less-than-nice things combined to mislead me:

  1. The py.path.local function bestrelpath is wrapped in a giant try...except AttributeError which masks any wrong types.
  2. py.path.local has an __eq__ implementation that can compare true with an str: py.path.local('/foo') == '/foo' is true.
  3. The cache seems to always be warmed up with a py.path.local, so the bestrelpath code actually does run with py.path.locals. When an str comes in which compares true with a previous py.path.local, it seems like everything's fine.

Anyway, can look into these things later 😨 , but for now, better to just revert the offending commit to fix people's tests.

@bluetech bluetech mentioned this pull request Jan 20, 2020
4 tasks
@nicoddemus
Copy link
Member

Awesome @bluetech, thanks for the lightning fast response!

And ouch, indeed py.path.local comparing with str is misleading... good thing that pathlib doesn't do that.

I managed to reproduce the issue with this:

from nose.tools import raises

@raises(RuntimeError)
def test_fail_without_tcp():
    raise RuntimeError

I don't think we need to release this in a hurry, few test suites will be affected (only those mixing nose and pytest). Perhaps we should keep your original patch and just fix the assertion? The test above passes for me when I apply this:

diff --git a/src/_pytest/nodes.py b/src/_pytest/nodes.py
index fc951d2bc..64363662a 100644
--- a/src/_pytest/nodes.py
+++ b/src/_pytest/nodes.py
@@ -462,7 +462,7 @@ class Item(Node):
     @cached_property
     def location(self) -> Tuple[str, Optional[int], str]:
         location = self.reportinfo()
-        assert isinstance(location[0], py.path.local), location[0]
+        assert isinstance(location[0], (py.path.local, str)), location[0]
         fspath = self.session._node_location_to_relpath(location[0])
         assert type(location[2]) is str
         return (fspath, location[1], location[2])

(It is a shame we don't have a test for that nose feature in the first place actually)

@bluetech
Copy link
Member Author

I prefer to revert the entire commit, because all of the changes related to this cache and the type of reportinfo() need a deeper look now. So I prefer to have a clean master reintroduce the changes with more consideration.

I'll add the regression test to this PR, thanks!

This reverts commit 930a158.

Regression test from Bruno Oliveira.
@bluetech bluetech force-pushed the fix-py-typed-fixes-regression branch from 4301bfe to fb99b5c Compare January 20, 2020 21:45
@bluetech
Copy link
Member Author

I added the regression test. Verified that it crashes before/passes after. Will merge once CI passes.

@nicoddemus
Copy link
Member

Awesome, thanks again. 👍

@bluetech bluetech merged commit a52f791 into pytest-dev:master Jan 20, 2020
@bluetech bluetech deleted the fix-py-typed-fixes-regression branch January 20, 2020 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5.3.4 causing internal errors
2 participants