Skip to content

Handle python_files correctly in assertion rewrite #2140

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

Merged
merged 4 commits into from
Jun 1, 2017

Conversation

pelme
Copy link
Member

@pelme pelme commented Dec 16, 2016

This PR solves #2121.

Before merging this, pytest-dev/py#99 and a new release of py is required.

@pelme pelme changed the title Failing test for issue #2121 Handle python_files correctly in assertion rewrite Dec 17, 2016
@pelme
Copy link
Member Author

pelme commented Dec 17, 2016

I've updated the PR with a fix and changelog. The fix in py must land before merging this, though!

@nicoddemus
Copy link
Member

Very nice, thanks @pelme!

@nicoddemus
Copy link
Member

We should also add a requirement for py>=1.4.33 to setup.py, otherwise users will get this error when upgrading:

  File "C:\pytest\_pytest\assertion\rewrite.py", line 97, in find_module
    if not self._should_rewrite(name, fn_pypath, state):
  File "C:\pytest\_pytest\assertion\rewrite.py", line 169, in _should_rewrite
    if fn_pypath.fnmatch(pat):
  File "C:\pytest\.env35\lib\site-packages\py\_path\common.py", line 209, in fnmatch
    return FNMatcher(pattern)(self)
  File "C:\pytest\.env35\lib\site-packages\py\_path\common.py", line 402, in __call__
    return py.std.fnmatch.fnmatch(name, pattern)
  File "C:\pytest\.env35\lib\site-packages\py\_apipkg.py", line 125, in __makeattr
    result = importobj(modpath, attrname)
  File "C:\pytest\.env35\lib\site-packages\py\_apipkg.py", line 48, in importobj
    module = __import__(modpath, None, None, ['__doc__'])
  File "<frozen importlib._bootstrap>", line 969, in _find_and_load
  File "<frozen importlib._bootstrap>", line 954, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 892, in _find_spec
  File "<frozen importlib._bootstrap>", line 873, in _find_spec_legacy
  File "C:\pytest\_pytest\assertion\rewrite.py", line 97, in find_module
    if not self._should_rewrite(name, fn_pypath, state):
  File "C:\pytest\_pytest\assertion\rewrite.py", line 169, in _should_rewrite
    if fn_pypath.fnmatch(pat):
  File "C:\pytest\.env35\lib\site-packages\py\_path\common.py", line 209, in fnmatch
    return FNMatcher(pattern)(self)
  File "C:\pytest\.env35\lib\site-packages\py\_path\common.py", line 402, in __call__
    return py.std.fnmatch.fnmatch(name, pattern)
  File "C:\pytest\.env35\lib\site-packages\py\_apipkg.py", line 125, in __makeattr
    result = importobj(modpath, attrname)
  File "C:\pytest\.env35\lib\site-packages\py\_apipkg.py", line 48, in importobj
    module = __import__(modpath, None, None, ['__doc__'])
  File "<frozen importlib._bootstrap>", line 968, in _find_and_load
RecursionError: maximum recursion depth exceeded while calling a Python object

Hmm pinning a requirement for a different version of py could cause any problems that we are not foreseeing here?

@pelme
Copy link
Member Author

pelme commented Dec 27, 2016

I'm will update this PR as soon as 1.4.33 is available, then the builds should pass (they do locally with patched py for me).

I was thinking about copy/pasting the fnmatch code (which is not huge) from py but that did not seem very nice either. :/

@nicoddemus
Copy link
Member

@pelme 1.4.33 has been released for some time now, would you like to continue this PR?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0008%) to 92.235% when pulling c98ad2a on pelme:issue2121 into 3871810 on pytest-dev:master.

@pelme
Copy link
Member Author

pelme commented May 31, 2017

I've rebased the PR on the current master and adapted the changelog to be towncrier friendly! Anything missing?

@nicoddemus
Copy link
Member

Sorry @pelme this went under the radar, I will review/merge it tomorrow!

@pelme
Copy link
Member Author

pelme commented Jun 1, 2017

No worries @nicoddemus, I was slow to update the PR.

I'm very very grateful for all the work you put into pytest and excited to see all the progress happening! 🍻

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me 👍

@nicoddemus
Copy link
Member

Awesome, thanks @pelme! 😁

@nicoddemus nicoddemus merged commit bcbad5b into pytest-dev:master Jun 1, 2017
@RonnyPfannschmidt
Copy link
Member

oh my goodness, i just have seen that right above in the comment we explicitly have a warning about importing fnmatch too late

@nicoddemus
Copy link
Member

Ouch indeed. And we all missed that.

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.

4 participants