Repository navigation
Relative vs absolute imports in tests #6452
Description
Activity
For example, let's say we remove
somefunction()fromastropy.somepackagebut forgot to removetest_somefunction()that importssomefunction. But we also have an older version ofastropywithsomefunctionstill in it. Will the test somehow runssomefunctionfrom the older copy and thus erroneously pass? The correct behavior is for the test function to fail withImportError, so we know that we need to remove the test as well.I don't have time to investigate this scenario in detail, so I'll just throw it out here for your consideration. I ran into problems like this in the past but I don't recall enough to give you complete description of it.
It's a good question, and I think the answers to some of these kinds of questions will have to come from some empirical testing.
This is hedging a bit, but one of the reasons I brought it up in #6451 is because the answer to some of these questions might depend on whether we do some restructuring to allow
pytestto run directly.Reacted by P. L. LimWill the test somehow runs somefunction from the older copy and thus erroneously pass?
Of course if you use absolute imports and run
pytest astropy(it won't pass forpython setup.py test) it will test against the first discovered installed astropy version (which could be one that had still somefunction).I regularly use running
pytestdirectly if I want to know if the just installed package passes the tests (or if something went wrong) in my projects. However given that we haveastropy.test()that's not really necessary for astropy.Since astropy bundles tests, it seems to me safer to use relative imports. Then I can be absolutely positively sure that when I run tests (either installed or from the git source) that there is concordance between the tests and the code. With absolute imports it seems possible that when I think I am testing my git source that I am accidentally running against installed astropy. Is that possible in practice?
Reacted by P. L. Lim, Brigitta Sipőcz and Danylo MysakWill the test somehow runs somefunction from the older copy and thus erroneously pass?
I've just tested this, and (at least on Python 3), the answer is no. Check out the following example:
(start in empty directory) $ mkdir -p astropy/table/tests $ touch astropy/__init__.py $ touch astropy/table/__init__.py $ touch astropy/table/tests/__init__.py $ pico astr $ echo "from astropy.table import Table" >> astropy/table/tests/test_table.py $ pytest astropy ============================= test session starts ============================== platform darwin -- Python 3.6.2, pytest-3.1.3, py-1.4.34, pluggy-0.4.0 rootdir: /Users/tom/tmp/test, inifile: plugins: cov-2.5.1, mpl-0.8, xdist-1.18.2 collected 0 items / 1 errors ==================================== ERRORS ==================================== ______________ ERROR collecting astropy/table/tests/test_table.py ______________ ImportError while importing test module '/Users/tom/tmp/test/astropy/table/tests/test_table.py'. Hint: make sure your test modules/packages have valid Python names. Traceback: astropy/table/tests/test_table.py:1: in <module> from astropy.table import Table E ImportError: cannot import name 'Table' !!!!!!!!!!!!!!!!!!! Interrupted: 1 errors during collection !!!!!!!!!!!!!!!!!!!! =========================== 1 error in 0.15 seconds ============================At least with Python 3, I think there is no danger of importing another installation?
EDIT: tested Python 2.7 and the test also fails.
I think it would be helpful if someone could come up with an example where it does matter though!
With absolute imports it seems possible that when I think I am testing my git source that I am accidentally running against installed astropy. Is that possible in practice?
Wouldn't
pip install --editable /astropy/git/sourcesolve this problem? This way installed code is the git source.@astrofrog - I tried a couple of simple cases (just running like normal and with a PYTHONPATH set for a test with an absolute import) and confirm that pytest still imports the correct local astropy.
So if nobody can actually demonstrate this failure mode AND if there is an actual practical benefit, then I would be 👍 with going to absolute imports.
I'm not sure I have seen a clear statement of what we can do with astropy (testing-wise as developers) with absolute imports that is not possible with relative imports. What's the benefit?
If we go with absolute imports, it might be cleaner to do it everywhere rather than just with tests?
Note that PEP8 actually recommends absolute imports unless there is a good reason not to
Also I have a script somewhere that can convert all imports from relative to absolute, so if we do want to do this, that might be easier than doing it by hand!
Reacted by Dan D'Avella and Brigitta SipőczI'm not sure I have seen a clear statement of what we can do with astropy (testing-wise as developers) with absolute imports that is not possible with relative imports. What's the benefit?
This is one reason that I actually hadn't thought of:
Note that PEP8 actually recommends absolute imports unless there is a good reason not to
The other main motivation for using absolute imports in your tests is that it means that the tests have to import your package in the same way that any user code does. The tests can no longer get away with having a privileged position just because they live within the same code base. This gives you some additional confidence that what you are actually testing is the same as what your users are using, and it can help identify packaging issues earlier.
On absolute imports elsewhere: I'd definitely prefer to have
from astropy.extern...overfrom ..extern ..., and generally feel that between submodules an absolute path is clearer. I'm less sure that I would really like to replacefrom .utils ...withfrom astropy.time.utils ..., i.e., imports within a submodule perhaps are clearer using relative imports.Maybe one could have a more general rule to avoid relative imports from a module above oneself? That would cover all tests, and, to me at least, make the rest of the code clearer (and would be easy to search & replace using a script).
That said, I don't want to let this distract from the question for this PR, which was explicitly about tests. I think that is a good idea independently.
Reacted by Tom AldcroftE.g. in
astropy/io/ascii/ecsv.py, the current is:from ...extern import six from . import core, basic from ...table import meta, serialize from ...utils.data_info import serialize_context_asThe suggestion of @mhvk would be the following (after re-arranging lines a bit), which I like. Within the context of a sub-package it makes a clear distinction between external dependences and sub-package modules.
from astropy.extern import six from astropy.table import meta, serialize from astropy.utils.data_info import serialize_context_as from . import core, basicThis is a question that probably deserves an entirely separate thread but @taldcroft's example reminded me to ask it: is there a good reason that we continue to package
sixas an extern? Will the need for this go away with Py2?is there a good reason that we continue to package
sixas an extern?This is a moot discussion once we remove Python 2 support. So, I'd say, just leave it as-is until we remove it completely for Python 3.
Reacted by Dan D'Avella@taldcroft - yes, that is what I had in mind, and I think your example indeed shows it is an improvement.
is there a good reason that we continue to package six as an extern?
and I can't wait the big cleanup session at the meeting when we finally can get rid all of it :)
Reacted by P. L. LimReacted by Tom Aldcroft
This discussion has been moved from #6451.
Right now, it appears that nearly all tests in astropy do something like the following at the top:
In general, it is considered good practice to use absolute imports in unit tests so that the tests actually run against installed code when possible. This is important since it means the tests run against the same code that users will see, and it can help identify packing issues before distributing.
This proposal is primarily motivated by this section of the pytest documentation.
However, there are other considerations (#6451 (comment)) that may prevent this from being practical for astropy. In summary, more investigation/discussion is needed.