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

No pytest import from non-test code #1266

Merged
merged 2 commits into from Feb 1, 2018

Conversation

Projects
None yet
1 participant
@cdeil
Member

cdeil commented Feb 1, 2018

@leajouvin reported on Slack the following error:
https://gist.github.com/cdeil/e80093d61034831c810c1486dccff0a8

She is trying to import gammapy.catalog, which imports

from astropy.tests.helper import ignore_warnings

and fails because she has a broken pytest installation (the py dependency of pytest is missing).

We should never import code that imports pytest from normal Gammapy code. I checked this:

$ ack -l astropy.tests gammapy | egrep -v tests
gammapy/conftest.py
gammapy/_astropy_init.py
gammapy/catalog/gammacat.py
gammapy/catalog/fermi.py
gammapy/catalog/hess.py

The import pytest in astropy.tests.helper was added a year ago:
astropy/astropy@1196fdb#diff-5a4193c12a96fa7bfa06b29a41e95120R18
and looking at http://astropy.readthedocs.io/en/latest/testhelpers.html, I think this is something we can rely on and use from Gammapy tests if needed, but really shouldn't import from normal Gammapy code.

So the fix here is to see if we still need ignore_warnings in gammapy/catalog to avoid users seeing warnings because of small formatting issues with the catalog files we read that we can never fix, and if yes, just write in a few lines our own ignore_warnings.


In addition, this will also trigger an import of pytest:

from ..conftest import PYTEST_HEADER_MODULES

There the list of models should be moved to normal Gammapy code or config. Either separated, or done the other way around, import that version from conftest.py for the tests.


Finally, we might want to add a build on travis-ci that runs checks via python instead of pytest in an env without pytest installed, that would have triggered this issue. I'm not sure if this is really needed though, I'd say this is low priority.


Assigning this issue to myself, but if anyone wants to take on one or several of the three tasks listed above, PRs welcome any time.

@cdeil cdeil added the bug label Jan 25, 2018

@cdeil cdeil added this to the 0.7 milestone Jan 25, 2018

@cdeil cdeil self-assigned this Jan 25, 2018

@cdeil

This comment has been minimized.

Member

cdeil commented Feb 1, 2018

I added commits here that address this issue. Let's see if python -m gammapy info like that works on travis-ci and/or appveyor.

@cdeil

This comment has been minimized.

Member

cdeil commented Feb 1, 2018

Running python -m gammapy info in CI doesn't work, because in many CI builds it isn't installed:

I'll revert that now.

@cdeil cdeil merged commit 6cf2364 into gammapy:master Feb 1, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment