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
Fix CI problems #710
Fix CI problems #710
Conversation
There's one problem that needs fixing upstream (astropy/astropy#9664). |
This looks good except for the one we can't fix yet... |
TESTED_VERSIONS) | ||
|
||
def pytest_configure(config): | ||
config.option.astropy_header = True | ||
except ImportError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just add pytest-astropy-header
as a hard dependency rather than doing this nested try except (it should ideally be picked up from pytest-astropy, but clearly something is not working as it should).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just add pytest-astropy-header as a hard dependency
Yeah, I also thought about this. But the test-log shows that it's installed (corresponding log line from your PR) but not picked up - which makes it probably a lot harder to solve than just adding a hard-dependency.
So I would prefer the nested try-catch for now, it's only astropy-2.x after all and we may even drop support for it once the new astropy LTS is out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather not do a double try/except, but don't have no strong preference to clean it up right now rather than when you remove python2 support anyway.
Should this stay open until the upstream is fixed? Otherwise I would temporarily set the astropy-dev test to allow-fail and put this in. |
I would vote for merging this (CI on master is already broken, this at least fixes some issues). |
@mwcraig I'm going to merge this tomorrow if you won't object. The astropy-dev job is then (temporarily) allowed to fail. |
It seems like we shouldn't use
python:
but instead use thePYTHON_VERSION
variable.I also incremented the NumPy versions (no particular reason except that we were missing NumPy 1.15 and I think that would be more appropriate than NumPy 1.13).
Closes #706
Ref: #707