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

PYTEST_DONT_REWRITE is not checked for plugins #2995

Closed
anntzer opened this Issue Dec 3, 2017 · 8 comments

Comments

Projects
None yet
3 participants
@anntzer
Copy link
Contributor

anntzer commented Dec 3, 2017

Adding PYTEST_DONT_REWRITE to the docstring of a test module suppresses assertion rewrites; however, this does not work for plugins.

Minimal (contrieved) example:
plugin.py

"""PYTEST_DONT_REWRITE"""
print("hello, world")

run:

$ python -c 'import pytest, plugin; pytest.main()' -p plugin
hello, world
===================================== test session starts ======================================
platform linux -- Python 3.6.3, pytest-3.3.0, py-1.5.2, pluggy-0.6.0
rootdir: /tmp/foo, inifile:
collected 0 items                                                                              

======================================= warnings summary =======================================
None
  Module already imported so can not be re-written: plugin

-- Docs: http://doc.pytest.org/en/latest/warnings.html
================================== 1 warnings in 0.00 seconds ==================================
$ pip list
attrs (17.3.0)
pip (9.0.1)
pluggy (0.6.0)
py (1.5.2)
pytest (3.3.0)
setuptools (28.8.0)
six (1.11.0)

Python 3.6 on Arch Linux.

Alternatively, it may also be nice if the warning was only output if there actually is at least one assert in the unrewritable module (or if -O is on which may make that impossible to check, perhaps).

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Dec 11, 2017

(or if -O is on which may make that impossible to check, perhaps).

Actually pytest already checks for that 😉

pytest/_pytest/config.py

Lines 1045 to 1060 in 964c29c

def _warn_about_missing_assertion(self, mode):
try:
assert False
except AssertionError:
pass
else:
if mode == 'plain':
sys.stderr.write("WARNING: ASSERTIONS ARE NOT EXECUTED"
" and FAILING TESTS WILL PASS. Are you"
" using python -O?")
else:
sys.stderr.write("WARNING: assertions not in test modules or"
" plugins will be ignored"
" because assert statements are not executed "
"by the underlying Python interpreter "
"(are you using python -O?)\n")

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Dec 12, 2017

the demo code explicitly imports the plugin before assertion rewriting, the warning is about not even trying (i.e. before we even check the docstring)

if self.is_rewrite_disabled(doc):
is responsible for the actual check on all modules

so the real issue here is that op triggers a warning that in that particular case could be left out since we wouldn't rewrite to begin with

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Dec 12, 2017

i propose we ensure the warning triggers only for modules that dont have a dont rewrite tag

def _warn_already_imported(self, name):
self.config.warn(
'P1',
'Module already imported so cannot be rewritten: %s' % name)
is the code that would need a update, it should be a nice first conribution

@anntzer please verify if my hypothesis is correct

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Dec 12, 2017

so the real issue here is that op triggers a warning that in that particular case could be left out since we wouldn't rewrite to begin with

IIUC, the -p flag is intended to force plugin to be imported as a plugin, so pytest would in that case attempt to rewrite asserts.

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

RonnyPfannschmidt commented Dec 12, 2017

@nicoddemus thing is, the already imported module is marked as dont rewrite - so i beleive its fine to firego thatr warning in that case - in any case it wouldn't happen if the unneeded import was either left out, or the plug in was passed as plugin object to main

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Dec 12, 2017

Ahh true, thanks for the clarification

@anntzer

This comment has been minimized.

Copy link
Contributor Author

anntzer commented Dec 13, 2017

I think something like the following patch should suffice:

diff --git a/_pytest/assertion/rewrite.py b/_pytest/assertion/rewrite.py
index 92deb653..110b189c 100644
--- a/_pytest/assertion/rewrite.py
+++ b/_pytest/assertion/rewrite.py
@@ -180,10 +180,10 @@ class AssertionRewritingHook(object):
         be rewritten on import.
         """
         already_imported = set(names).intersection(set(sys.modules))
-        if already_imported:
-            for name in already_imported:
-                if name not in self._rewritten_names:
-                    self._warn_already_imported(name)
+        for name in already_imported:
+            if (not self.is_rewrite_disabled(sys.modules[name].__doc__ or "")
+                    or name not in self._rewritten_names):
+                self._warn_already_imported(name)
         self._must_rewrite.update(names)
 
     def _warn_already_imported(self, name):

However I would appreciate help as to how to test this feature (do you have any helper for generating a fake module going into sys.modules, for example?). Or feel free to just take over the patch yourself, either way works.

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Dec 13, 2017

@anntzer thanks for posting the patch.

I'm short on time right now so we would appreciate a PR if you have the time

Here's an example of test which uses the testdir fixture to generate a test and module file, run pytest against it and check the output:

def test_pytest_plugins_rewrite_module_names_correctly(self, testdir):
"""Test that we match files correctly when they are marked for rewriting (#2939)."""
contents = {
'conftest.py': """
pytest_plugins = "ham"
""",
'ham.py': "",
'hamster.py': "",
'test_foo.py': """
def test_foo(pytestconfig):
assert pytestconfig.pluginmanager.rewrite_hook.find_module('ham') is not None
assert pytestconfig.pluginmanager.rewrite_hook.find_module('hamster') is None
""",
}
testdir.makepyfile(**contents)
result = testdir.runpytest_subprocess('--assert=rewrite')
assert result.ret == 0

You can use this test as basis, I believe if you add a docstring to the ham module marking it to skip rewrite and import it in conftest.py you will obtain the same result you posted. Then by making an assert like this:

result = testdir.runpytest_subprocess('--assert=rewrite') 
assert 'Module already imported so can not be re-written: ham' not in result.stdout.str()

The test should fail without your patch and pass with it applied.

Let us know if you need further guidance. 👍

@anntzer anntzer referenced this issue Dec 13, 2017

Merged

Respect PYTEST_DONT_REWRITE for plugins too. #3025

4 of 4 tasks complete

mrh1997 added a commit to mrh1997/pytest-headlock that referenced this issue Sep 27, 2018

Pin dependencies and switch over to setuptools
Enforces pytools 3.3.2 now, as older version do not support module level
PYTEST_DONT_REWRITE (pytest-dev/pytest#2995).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.