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

sys.modules[None] gets pseudo module when pytest skip condition is string #2103

Closed
jaraco opened this issue Nov 30, 2016 · 4 comments
Closed

Comments

@jaraco
Copy link
Contributor

jaraco commented Nov 30, 2016

In pypa/setuptools#860, the test_sandbox tests started failing when another commit was made that invoked a pytest skip with a string expression, because that causes sys.modules to get a None key in that dict. This behavior can be minimally replicated thus:

$ cat > test_me.py
import sys
import pytest

@pytest.mark.skipif('os.environ.get("foo")')
def test_this():
    assert None not in sys.modules        
$ python -m rwt pytest -- -m pytest
Collecting pytest
  Using cached pytest-3.0.4-py2.py3-none-any.whl
Collecting py>=1.4.29 (from pytest)
  Using cached py-1.4.31-py2.py3-none-any.whl
Installing collected packages: py, pytest
Successfully installed py-1.4.31 pytest-3.0.4
====================================== test session starts =======================================
platform darwin -- Python 3.6.0b4, pytest-3.0.4, py-1.4.31, pluggy-0.4.0
rootdir: /Users/jaraco/issue-nnn, inifile: 
collected 1 items 

test_me.py F

============================================ FAILURES ============================================
___________________________________________ test_this ____________________________________________

    @pytest.mark.skipif('os.environ.get("foo")')
    def test_this():
>       assert None not in sys.modules
E       assert None not in {'builtins': <module 'builtins' (built-in)>, 'sys': <module 'sys' (built-in)>, '_frozen_importlib': <module 'importlib._bootstrap' (frozen)>, '_imp': <module '_imp' (built-in)>, ...}
E        +  where {'builtins': <module 'builtins' (built-in)>, 'sys': <module 'sys' (built-in)>, '_frozen_importlib': <module 'importlib._bootstrap' (frozen)>, '_imp': <module '_imp' (built-in)>, ...} = sys.modules

test_me.py:6: AssertionError
==================================== 1 failed in 0.04 seconds ====================================
$ 
@jaraco
Copy link
Contributor Author

jaraco commented Nov 30, 2016

My feeling is that pytest shouldn't be adding a None key to sys.modules, but if that's a legitimate expectation to be had about sys.modules, then setuptools should be re-written to support this usage. Thoughts?

@nicoddemus
Copy link
Member

nicoddemus commented Nov 30, 2016

This behavior can be minimally replicated thus:

Thanks a lot for taking the time to find a minimal example to replicate the issue!

My feeling is that pytest shouldn't be adding a None key to sys.modules

I also find it weird. Decided to investigate a little, found the code that does that here. I think the intent is to keep the module object alive.

I commented out that line and all tests pass both in py27 and py35, so we might either comment it out or perhaps don't use Source.compile at all but the builtin compile instead.

@nicoddemus
Copy link
Member

More info: that if was introduced in this commit in py in 2010. I think it was an early support for Python 3. The earliest version we support is 3.3 which was released in 2012, so I think the entire if can be scrapped.

Removing it both py27 and py35 pass, as well as @jaraco's test.

nicoddemus added a commit to nicoddemus/pytest that referenced this issue Dec 1, 2016
This code leaves None in sys.modules as a side effect but is no longer needed in the Python 3 versions we support.

Fix pytest-dev#2103
@jaraco
Copy link
Contributor Author

jaraco commented Dec 1, 2016

Excellent. Thanks for this!

nicoddemus added a commit to nicoddemus/py that referenced this issue Dec 1, 2016
This code leaves None in sys.modules as a side effect but is no longer needed in the Python 3 versions we support.

See pytest-dev/pytest#2103 for the original report.
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

No branches or pull requests

2 participants