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

Enforce pytest version in astropy tests and give useful error message #6457

Closed
taldcroft opened this issue Aug 19, 2017 · 8 comments
Closed
Labels

Comments

@taldcroft
Copy link
Member

Now that astropy is not bundling pytest it seems there is a lot more opportunity for unwanted fun. This morning I changed to my Py2 environment for astropy development and suddenly the tests won't run at all, instead I get a fatal and obscure exception:

  File "/Users/aldcroft/anaconda3/envs/python2/lib/python2.7/site-packages/_pytest/config.py", line 417, in import_plugin
    assert isinstance(modname, str), "module name as string required, got %r" % modname
AssertionError: module name as string required, got u'astropy.tests.pytest_doctestplus'

So my first guess is to upgrade pytest, which then promptly gives me a different and obscure error. After messing around with a bunch of things I discovered #6418, and that suggested trying pytest 3.1. This worked, but in the meantime I lost almost an hour.

So what about having astropy just tell me straight away that pytest 3.1.x is required and nothing else will work.

@MSeifert04
Copy link
Contributor

MSeifert04 commented Aug 19, 2017

I agree - I had the same problem, but I just wasted 10 minutes because I remembered that there were issues with the pytest version.

I wonder if we should place the restrictions in the code (checking for a specific version and then error out with an informative exception) or if we should just add the pytest requirement to the pip-requirements-dev file.

@astrofrog
Copy link
Member

We should also enforce the version in setup.py in install_requires

@mhvk
Copy link
Contributor

mhvk commented Aug 19, 2017

👍 to this, though obviously ideally we are not so strongly dependent on a single version - we may want to make some haste with #6451... especially as in @drdavella we have someone taking charge!

@taldcroft
Copy link
Member Author

I don't have good insight into what happens when I type python setup.py test, but my typical use case is that during development I may jump between several environments without formally installing anything.

As for actual installation, I'm not convinced that there should be hard requirements on pytest to install astropy since astropy installs and runs perfectly well without it.

But anyway this infrastructure stuff is totally out of my depth, so if we can just get something that stops the pain of obscure pytest errors during python setup.py test from a git repo then I'll be happy.

@drdavella
Copy link
Contributor

drdavella commented Aug 21, 2017

As @astrofrog and @MSeifert04 mentioned, I think it should be sufficient to make explicit the requirement on pytest 3.1 in install_requires in setup.py, and in pip-requires-dev.

As for actual installation, I'm not convinced that there should be hard requirements on pytest to install astropy since astropy installs and runs perfectly well without it.

@taldcroft, in a perfect world we could move the pytest requirement from install_requires to tests_require in setup.py. Dependencies expressed in tests_require are installed in the local source tree (e.g. ./.eggs), and not in the target environment. The problem in our case is that when we run python ./setup.py test, under the hood we actually start a subprocess to run pytest. So even though ./setup.py knows where to find the locally installed pytest, our subprocess apparently does not. This means that, for the time being at least, we need to install pytest in the target environment as well.

@taldcroft
Copy link
Member Author

@drdavella - thanks for the explanation!

@bsipocz
Copy link
Member

bsipocz commented Aug 28, 2017

Actually the version requirements were added in #6419, I'll try to pick that PR back again soon.

@bsipocz
Copy link
Member

bsipocz commented May 17, 2018

I think this has been resolved in the meantime, there is no error raised any more. Now astropy relies on pytest-astropy, that strictly requires pytest>=3.1.0. Having an older version installed the machinery @drdavella mentioned above sets into action and a newer pytest is installed into ./.eggs/.

While there is no obvious warning issued, the version requirement is now listed both in docs/install.rst and setup.cfg.

Thus I'm closing this now, but feel free to reopen if needed.

@bsipocz bsipocz closed this as completed May 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants