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

Running ipytest.main multiple times triggers rewrite warnings for plugins #5432

Closed
chmp opened this issue Jun 10, 2019 · 7 comments

Comments

Projects
None yet
3 participants
@chmp
Copy link

commented Jun 10, 2019

When running ipytest.main multiple times inside the same python process, the rewrite warning ("Module already imported so cannot be rewritten:" ...) is trigged for plugins. A minimal example is:

pip install pytest pytest_asyncio
import pytest
pytest.main()
pytest.main()

My real use case is: I'm using ipytest (I'm also the author) to run tests inside jupyter notebooks with pytest. Internally, ipytest calls pytest.main. Hence, the rewrite warnings are triggered. The pytest_asyncio plugin is just an example, previously I observed the same behavior with the nbval plugin.

I'm running pytest==4.2.6on OSX 10.13.6. The packages are:

Package            Version  Location                                
------------------ -------- ----------------------------------------
aiohttp            3.5.4    
appnope            0.1.0    
async-timeout      3.0.1    
atomicwrites       1.3.0    
attrs              19.1.0   
backcall           0.1.0    
bleach             3.1.0    
certifi            2019.3.9 
chardet            3.0.4    
commonmark         0.9.0    
decorator          4.4.0    
defusedxml         0.6.0    
entrypoints        0.3      
future             0.17.1   
idna               2.8      
importlib-metadata 0.17     
ipykernel          5.1.1    
ipytest            0.6.0    
ipython            7.5.0    
ipython-genutils   0.2.0    
ipywidgets         7.4.2    
jedi               0.13.3   
Jinja2             2.10.1   
jsonschema         3.0.1    
jupyter            1.0.0    
jupyter-client     5.2.4    
jupyter-console    6.0.0    
jupyter-core       4.4.0    
keyring            19.0.2   
MarkupSafe         1.1.1    
mistletoe          0.7.2    
mistune            0.8.4    
more-itertools     7.0.0    
multidict          4.5.2    
nbconvert          5.5.0    
nbformat           4.4.0    
notebook           5.7.8    
packaging          19.0     
pandocfilters      1.4.2    
parso              0.4.0    
pexpect            4.7.0    
pickleshare        0.7.5    
pip                19.1.1   
pluggy             0.12.0   
prometheus-client  0.7.0    
prompt-toolkit     2.0.9    
ptyprocess         0.6.0    
py                 1.8.0    
Pygments           2.4.2    
pyparsing          2.4.0    
pyrsistent         0.15.2   
pytest             4.6.2    
pytest-asyncio     0.10.0   
python-dateutil    2.8.0    
pyzmq              18.0.1   
qtconsole          4.5.1    
requests           2.22.0   
Send2Trash         1.5.0    
setuptools         41.0.1   
six                1.12.0   
terminado          0.8.2    
testpath           0.4.2    
tornado            6.0.2    
traitlets          4.3.2    
urllib3            1.25.3   
wcwidth            0.1.7    
webencodings       0.5.1    
wheel              0.33.4   
widgetsnbextension 3.4.2    
yarl               1.3.0    
zipp               0.5.1    

Checklist:

  • a detailed description of the bug or suggestion
  • output of pip list from the virtual environment you are using
  • pytest and operating system versions
  • minimal example if possible

@chmp chmp changed the title Running `ipytest.main` multiple times triggers rewrite warnings for plugins Running ipytest.main multiple times triggers rewrite warnings for plugins Jun 10, 2019

@asottile

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

This usage pattern is at least not recommend which probably puts this into not supported / wontfix territory?

Calling pytest.main() will result in importing your tests and any modules that they import. Due to the caching mechanism of python’s import system, making subsequent calls to pytest.main() from the same process will not reflect changes to those files between the calls. For this reason, making multiple calls to pytest.main() from the same process (in order to re-run tests, for example) is not recommended.

Tracing this, what happens is as follows:

  • pytest.main()
  • pytest installs the assertion import hook
  • it marks the specific modules for rewriting
    • this checks if they have already been imported and produces a warnings
    • it subtracts out modules that have already been marked for import and imported
  • the tests run
  • pytest uninstalls the assertion import hook
  • pytest.main()
  • pytest installs a new assertion import hook
    • note this doesn't know anything about the previous rewritten imports
  • it marks the specific modules for rewriting
    • in this case, they have already been imported, and the the hook doesn't know about the already-rewritten modules so it produces a warning

The only way I could think of "fixing" this would be to attach some information to the actual modules themselves to identify that pytest rewrote them. It seems there's ~kind of this information already in the __loader__ attribute

(Pdb) mod = sys.modules['pytest_asyncio']
(Pdb) mod.plugin
<module 'pytest_asyncio.plugin' from '/tmp/t/venv/lib/python3.6/site-packages/pytest_asyncio/plugin.py'>
(Pdb) mod.plugin.__loader__
<_pytest.assertion.rewrite.AssertionRewritingHook object at 0x7f057a6eb400>
@asottile

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

this gets it closer:

diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py
index ce698f368..38b0b43de 100644
--- a/src/_pytest/assertion/rewrite.py
+++ b/src/_pytest/assertion/rewrite.py
@@ -260,6 +260,10 @@ class AssertionRewritingHook:
         from _pytest.warning_types import PytestAssertRewriteWarning
         from _pytest.warnings import _issue_warning_captured
 
+        # we already imported this (multiple invocations of pytest.main()?)
+        if isinstance(sys.modules[name].__loader__, type(self)):
+            return
+
         _issue_warning_captured(
             PytestAssertRewriteWarning(
                 "Module already imported so cannot be rewritten: %s" % name

but it seems that pytest currently doesn't know how to rewrite "packages" (by packages, I mean directories containing __init__.py files) and so the warning for pytest_asycio continues:

$ python t.py t2.py
============================= test session starts ==============================
platform linux -- Python 3.6.7, pytest-4.6.1.dev84+g40c5a9d9f, py-1.8.0, pluggy-0.12.0
rootdir: /tmp/t
plugins: asyncio-0.10.0
collected 1 item                                                               

t2.py .                                                                  [100%]

=========================== 1 passed in 0.01 seconds ===========================
============================= test session starts ==============================
platform linux -- Python 3.6.7, pytest-4.6.1.dev84+g40c5a9d9f, py-1.8.0, pluggy-0.12.0
rootdir: /tmp/t
plugins: asyncio-0.10.0
collected 1 item                                                               

t2.py .                                                                  [100%]

=============================== warnings summary ===============================
pytest/src/_pytest/config/__init__.py:768
pytest/src/_pytest/config/__init__.py:768
  /tmp/t/pytest/src/_pytest/config/__init__.py:768: PytestAssertRewriteWarning: Module already imported so cannot be rewritten: pytest_asyncio
    self._mark_plugins_for_rewrite(hook)

-- Docs: https://docs.pytest.org/en/latest/warnings.html
===================== 1 passed, 2 warnings in 0.01 seconds =====================
@chmp

This comment has been minimized.

Copy link
Author

commented Jun 11, 2019

Thanks a lot for the detailed analysis. I figured / feared that this issue would maybe fall into the won't fix category. In that case, I will investigate how to either supress the warnings (however #4439 seems to be a blocker) or how to re-expose the information of the previous assert rewrite hook between calls.

I just wanted to confirm this is expected behavior, before I go down that route. So either way, I'm good, whatever the official position is.

@chmp

This comment has been minimized.

Copy link
Author

commented Jun 11, 2019

Quick update: thanks to your detailed trace of what happens, I figure out one solution: by monkey patching _pytest.assertion.install_importhook to keep the original import hook around, I can ensure that the hook still remembers that the modules were rewritten (see below).

I will for now implement a quick fix on my side. Please just close this issue, if this is a wont-fix for you.

import sys
from _pytest import assertion
from _pytest.assertion import rewrite, AssertionState


def custom_install_importhook(config):
    """Try to install the rewrite hook, raise SystemError if it fails."""
    config._assertstate = AssertionState(config, "rewrite")
    
    installed = any(
        isinstance(hook, rewrite.AssertionRewritingHook) for hook in sys.meta_path
    )
    
    if not installed:
        config._assertstate.hook = hook = rewrite.AssertionRewritingHook(config)
        sys.meta_path.insert(0, hook)
        config._assertstate.trace("installed rewrite import hook")
    
    else:
        hook, = [hook for hook in sys.meta_path if isinstance(hook, rewrite.AssertionRewritingHook)]
    
    config._assertstate.hook = hook
    
    return hook

assertion.install_importhook = custom_install_importhook
@asottile

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

I created #5433 -- fixing that I think it's easy to make that warning suppressed with the patch I wrote above

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Jun 15, 2019

Thanks everyone.

@asottile should we close this and track #5433 only, or would you like to keep this open?

@asottile

This comment has been minimized.

Copy link
Member

commented Jun 15, 2019

I think we should track both, the other necessarily blocks this one however

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.