Made tests uninstallable#189
Conversation
|
For some context, that change was made in #165 to test changes related to #124. The issue was (as I recall) that in order to test it, a change had to be made to before anything was imported. When running the pytests when they are nested within the same directories, the package will be imported before the necessary changes could be made. It is definitely possible that there were better ways to work around this, or that pytest has changed such that this is no longer an issue. Also, a judgement could be made that testing that functionality is not important enough to justify making the tests harder to access and reference. To be clear, I am not opposed to this (and I am also no longer an active user of this package), but I wanted to provide more info. |
|
@theavey Thanks for the context! What are thoughts on making the tests not an installable package, but just part of the CI process and for developers vs putting it in the repo to be installed via pip and by end users? |
|
Yes, I agree (see also #182 (comment)). Thanks @theavey for providing the explanation and thanks @whitead for looking into the problem. |
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
According to the discussion, we don't want to make tests part of the package. Instead, we just don't install it.
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
I think that tests is still a package because tests/__init__.py is still present. Can we keep it that way but just not include it in setup.py? Then we could also keep using relative imports inside the tests, which is less error-prone than absolute ones.
tests/test_cbook.py
Outdated
| import gromacs.setup | ||
|
|
||
| from .datafiles import datafile | ||
| from datafiles import datafile |
There was a problem hiding this comment.
Do relative imports not work anymore because it's not a package anymore? Otherwise, adding the
from __future__ import absolute_importand keeping the relative import would be nicer.
I look at it and think "Eww... not a relative import".
There was a problem hiding this comment.
That was an accident, fixed it. I have kept it as a package and removed it from setup.py
3 participants
Based on @orbeckst's recommendation, made tests part of the package itself. Addresses #182. I tried to modify Travis CI file, but we'll see if it works.