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

Assertion rewrite should match files completely and avoid rewriting setup.py #2939

Closed
nicoddemus opened this issue Nov 22, 2017 · 7 comments · Fixed by #2940
Closed

Assertion rewrite should match files completely and avoid rewriting setup.py #2939

nicoddemus opened this issue Nov 22, 2017 · 7 comments · Fixed by #2940
Labels
topic: rewrite related to the assertion rewrite mechanism type: bug problem that needs to be addressed

Comments

@nicoddemus
Copy link
Member

Quoting @hroncok in pypa/setuptools#1170:

  • _pytest.config._mark_plugins_for_rewrite should not register setup when it sees setup.py
  • _pytest.assertion.rewrite._should_rewrite should probably match things that startswith mark + '.', not just mark (although I'm not very sure here)
@nicoddemus nicoddemus added topic: rewrite related to the assertion rewrite mechanism type: bug problem that needs to be addressed labels Nov 22, 2017
@hroncok
Copy link
Member

hroncok commented Nov 22, 2017

Ad _should_rewrite: when I think about it, it should either be exact match or it should start with the thing plus dot.

@nicoddemus
Copy link
Member Author

Heh exactly what I came up with. 😁

I will open a PR shortly.

@nicoddemus
Copy link
Member Author

diff --git a/_pytest/assertion/rewrite.py b/_pytest/assertion/rewrite.py
index d48b664..27026b0 100644
--- a/_pytest/assertion/rewrite.py
+++ b/_pytest/assertion/rewrite.py
@@ -168,7 +168,7 @@ class AssertionRewritingHook(object):
                 return True

         for marked in self._must_rewrite:
-            if name.startswith(marked):
+            if name == marked or name.startswith(marked + '.'):
                 state.trace("matched marked file %r (from %r)" % (name, marked))
                 return True

😉

I'm finishing up the test.

@hroncok
Copy link
Member

hroncok commented Nov 22, 2017

Note that blacklisting setup.py from _pytest.config._mark_plugins_for_rewrite is not needed anymore to fix this particular problem, but I'd still rather do it, because having it there can bring other problems some day.

nicoddemus added a commit to nicoddemus/pytest that referenced this issue Nov 22, 2017
@nicoddemus
Copy link
Member Author

Hmm not sure about setup.py now, I've seen code in the wild which used setup.py, although usually inside packages so this probably wouldn't be an issue. Let's see if others have an opinion on this.

@hroncok
Copy link
Member

hroncok commented Nov 22, 2017

Ok, not blacklist it, but reconsider the code that produces it as it's result for sdist installed packages. (I cannot reopen, not my issue)

@nicoddemus
Copy link
Member Author

nicoddemus commented Nov 22, 2017

I see, it makes sense to ignore setup.py files that are declared in the root of the metadata. Created #2941 for that. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: rewrite related to the assertion rewrite mechanism type: bug problem that needs to be addressed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants