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

Check for '_pytest' in sys.modules before trying to import Astropy's pytest #454

Merged
merged 2 commits into from Nov 15, 2012

Conversation

embray
Copy link
Member

@embray embray commented Nov 2, 2012

Because if '_pytest' is already in sys.modules it's likely that we're already running the py.test command, and so we should use the already imported system pytest.

This came up because when running the system py.test, if it's an older version than the one we're using now strange errors can occur such as:

INTERNALERROR> Traceback (most recent call last):
INTERNALERROR>   File "/usr/stsci/pyssgdev/Python-2.7.3/lib/python2.7/site-packages/_pytest/main.py", line 68, in wrap_session
INTERNALERROR>     config.pluginmanager.do_configure(config)
INTERNALERROR>   File "/usr/stsci/pyssgdev/Python-2.7.3/lib/python2.7/site-packages/_pytest/core.py", line 267, in do_configure
INTERNALERROR>     config.hook.pytest_configure(config=self._config)
INTERNALERROR>   File "/usr/stsci/pyssgdev/Python-2.7.3/lib/python2.7/site-packages/_pytest/core.py", line 419, in __call__
INTERNALERROR>     return self._docall(methods, kwargs)
INTERNALERROR>   File "/usr/stsci/pyssgdev/Python-2.7.3/lib/python2.7/site-packages/_pytest/core.py", line 430, in _docall
INTERNALERROR>     res = mc.execute()
INTERNALERROR>   File "/usr/stsci/pyssgdev/Python-2.7.3/lib/python2.7/site-packages/_pytest/core.py", line 348, in execute
INTERNALERROR>     res = method(**kwargs)
INTERNALERROR>   File "/usr/stsci/pyssgdev/Python-2.7.3/lib/python2.7/site-packages/_pytest/assertion/__init__.py", line 45, in pytest_configure
INTERNALERROR>     _load_modules(mode)
INTERNALERROR>   File "/usr/stsci/pyssgdev/Python-2.7.3/lib/python2.7/site-packages/_pytest/assertion/__init__.py", line 98, in _load_modules
INTERNALERROR>     from _pytest.assertion import reinterpret
INTERNALERROR>   File "/bray/sc1/root/src/astropy/astropy/astropy/extern/pytest.py", line 2447, in load_module
INTERNALERROR>     do_exec(co, module.__dict__)
INTERNALERROR>   File "<string>", line 1, in do_exec_def
INTERNALERROR>   File "_pytest.assertion.reinterpret", line 3, in <module>
INTERNALERROR>     from _pytest.assertion.util import BuiltinAssertionError
INTERNALERROR> ImportError: cannot import name BuiltinAssertionError

This made even me a little frightened. If you set the ASTROPY_USE_SYSTEM_PYTEST environment variable everything works fine, but it's easy to miss that in the docs. I think this pull request makes a reasonable assumption...though I know this has been hashed out at length before, so maybe there's already a known good reason not to do this?

@eteq
Copy link
Member

eteq commented Nov 2, 2012

Hmm... I think this makes sense, but I admit the pytest import structure has gotten me very confused at times. @iguananaut - you might add a note in the docs that indicates that either the ASTROPY_USE_SYSTEM_PYTEST envar or using the py.test command will invoke the system pytest? Or am I misunderstanding how this will work?

@jiffyclub, since you're the original test master, does this seem reasonable to you?

@jiffyclub
Copy link
Contributor

It seems reasonable to me that if the user is somehow manually invoking py.test on their own that we should try to use whatever installation they invoked.

I was initially a little worried that this would end up with people trying to run the tests with versions of py.test that are too old, but the minimum version specified in the setup.cfg file should still catch those.

@embray
Copy link
Member Author

embray commented Nov 2, 2012

Yeah, the py.test command is only installed if one has py.test itself installed. If one is just relying on what's included in Astropy there is no py.test. So using the py.test command line script sort of implies that you actually want to use whatever version of py.test that was installed with (which may not work with the Astropy tests--a disclaimer that we already make).

@astrofrog
Copy link
Member

This seems fine to me.

@eteq
Copy link
Member

eteq commented Nov 7, 2012

@iguananaut - can you update docs/development/testguide.rst to clarify this new behavior (e.g. specifically state that setup.py test and astropy.test() will use the builtin version unless you specify the envar, and py.test ... will always use the system version.

@embray
Copy link
Member Author

embray commented Nov 7, 2012

Will do.

@ghost ghost assigned embray Nov 7, 2012
@eteq
Copy link
Member

eteq commented Nov 12, 2012

Oh, and @iguananaut , feel free to merge once you've done that - this is pretty straightforward, after all.

…e, it's likely that we're already running the py.test command, and so we should use the already imported system pytest.
@embray
Copy link
Member Author

embray commented Nov 14, 2012

Just to note, running py.test still doesn't work for me very well. Lots of tests fail--in particular all the wcs tests fail. Not sure what that's all about. I don't even get a traceback like this since I'm using the latest pytest. But I guess that's a separate issue.

@embray
Copy link
Member Author

embray commented Nov 14, 2012

Actually looks like testguide.rst already states that running py.test uses the system version. So I think the docs were right, but the behavior was wrong. I'm just going to add a note in the changelog.

@mdboom
Copy link
Contributor

mdboom commented Nov 14, 2012

Are the wcs tests failing because you're running from the source tree and not the installed (built) code?

@embray
Copy link
Member Author

embray commented Nov 14, 2012

I dunno, probably. I only ever use setup.py test which works fine.

@eteq
Copy link
Member

eteq commented Nov 15, 2012

I just tested this now, and it looks like the tests that don't require building are all working fine, and the wcs tests pass if I run them in the build dir, so @mdboom's supposition seems likely. So I think this is all running the way it's supposed to.

@iguananaut - if you want you could add a brief note in the docs indicating that invoking via py.test should be done in the build directory for this reason, but it already recommends using setup.py test, which works fine, so whatever. Whatever you prefer is fine - I think this is good to merge either way.

@embray
Copy link
Member Author

embray commented Nov 15, 2012

Actually the wcs tests work for me now with py.test in the source. I think I had some old .pyc files and other crud lying around that was confusing things. I'll just leave it as is because I'm mostly just confused now. I think the docs make sense as written.

embray added a commit that referenced this pull request Nov 15, 2012
Check for '_pytest' in sys.modules before trying to import Astropy's pytest
@embray embray merged commit 0e0de75 into astropy:master Nov 15, 2012
keflavich pushed a commit to keflavich/astropy that referenced this pull request Oct 9, 2013
Check for '_pytest' in sys.modules before trying to import Astropy's pytest
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.

None yet

5 participants