Repository navigation
Long-term testing improvements and future directions #6451
Description
Activity
Just a question: Is the "absolute import" thing required or optional? I mean in what way does this affect the other questions - more specifically: Would using "relative imports" actually block any progress on the other issues (as far as I can see you only mention "good style" and testing against an already installed version)?
@MSeifert04 Good question: I believe it is actually completely independent of the other issues, but this seemed like a good place to mention it. We could make that change right now without doing anything else. Leaving it alone would not block progress on the other issues.
However, even if we made that change right now I don't think it would actually test against installed code unless we modified the test runner or we were able to run
pytestdirectly. The way thatpytesthandles this is a little bit opaque, though.I'm very much in favour of everything here. Currently, the testing is somewhat of a hatchet job, which includes relying on overriding private attributes. I found it very hard to understand when I had to adjust things for more recent versions of pytest. So, it seems to me the only real question is whether it is possible to do everything with plugins, i.e., whether the test runner can be removed. But I really like that the tasks are nicely independent of each other, with each being an improvement on its own.
On absolute paths: I have no strong opinion, but like the aspect that the test file becomes even more of an example of how to run astropy, including in what & how to import.
update all tests to use absolute imports
I am a bit concerned about this. Relative imports let me do
setup.py developand then test the code in development instead of my installed version. Also, in the case where someone has multiple versions installed, absolute import might introduce ambiguity.should tests ship with packaged code?
Dan and I already discussed this offline. The answer is yes. We want the user to be able to run tests themselves on their installed copy.
The discussion also segway-ed into should we re-organized the tests into:
astropy/ astropy/ cosmology/ io/ ... tests/I would say "no" to that even if it is something recommended or preferred by
pytest. It is a lot of work with little gain.@pllim, we are in agreement on your second and third points.
I am a bit concerned about this. Relative imports let me do setup.py develop and then test the code in development instead of my installed version. Also, in the case where someone has multiple versions installed, absolute import might introduce ambiguity.
Testing in development mode will still work with absolute imports. However, testing with multiple versions installed will not be possible. I would argue that this is not a supported use case for
pytestin general.testing with multiple versions installed will not be possible.
A lot of users have multiple versions installed (on purpose or by accident). So, I think this might be a deal-breaker for absolute imports.
@pllim - your point is a good one (and I realize I hadn't noticed that
setup.py developnow works on python3). But is it really a problem? If one is in the top-level directory, that's wereastropyis imported from....
I see @drdavella already answered similarly, though I don't understand the comment that it changes how one tests with multiple versions, apart from that one better be within the corresponding environment (either by being in the right directory or setting ones
venv).In any case, perhaps we should not get distracted too much by this - after all, above it was suggested that the rest of the plan did not depend on having absolute imports.
If one is in the top-level directory, that's where astropy is imported from.
@mhvk this is actually where I'm unsure. I believe that depending on how
pytestis invoked, it will attempt to use the installed version of the module (including modules that are "installed" in development mode). This is generally desirable, as I mentioned above.A lot of users have multiple versions installed (on purpose or by accident). So, I think this might be a deal-breaker for absolute imports.
@pllim I don't mean to take a hard line on this but we need to ask whether it is really worth going against recommended practices in order to support potentially degenerate use cases.
In any case, perhaps we should not get distracted too much by this - after all, above it was suggested that the rest of the plan did not depend on having absolute imports.
I agree, although this is still a useful discussion.
I need to think about all this a bit, but I think in any case we would want to make sure that
astropy.test()still works to test installed versions.As far as I know, using absolute vs relative imports is irrelevant - if you run
pytest astropy, I don't think there is any risk of it importing an installed version. I use absolute imports in http://github.com/glue-viz/glue and run pytest directly and it all works fine. I propose we leave the relative vs absolute imports out of this issue and discuss this elsewhere if people feel strongly that we should change this.I could have sworn running pytest directly worked already provided one ran
python setup.py build_ext --inplacefirst? (but maybe it doesn't anymore, will check later)I propose we leave the relative vs absolute imports out of this issue and discuss this elsewhere if people feel strongly that we should change this.
@astrofrog I moved the discussion about relative imports to #6452.
I could have sworn running pytest directly worked already provided one ran python setup.py build_ext --inplace first? (but maybe it doesn't anymore, will check later)
That may have worked at one point but it appears that something changed either in
pytestor in the way that we loaded plugins (or both) that now causes failures (see #5402 and #6437). However, if it does still work in your environment that would be an interesting data point.Here's an important point that I forgot to add to the original proposal:
- Regardless of whether the test runner gets moved to astropy-helpers or removed entirely, it would be very useful to move the test helpers in
astropy.tests.helperto astropy-helpers so that other packages can use them without relying on the astropy core.
Reacted by P. L. Lim- Regardless of whether the test runner gets moved to astropy-helpers or removed entirely, it would be very useful to move the test helpers in
@drdavella - the reason the tests stuff isn't in the helpers is because the original intent is that the helpers are only meant to be temporarily installed during the
setup.pyprocess but not installed all the time, so runningastropy.test()wouldn't work after installation.@astrofrog, whoops okay that's good to know. So I guess I'm really asking for another package.
I was away, so coming a bit late to this party.
All sounds good to me, though moving the test runner back to astropy-helpers may go against history as it was moved out of it back to the core once. So my argument is that we need to look into the arguments made at the time whether they will still be a concern in the future: astropy/astropy-helpers#184 and #4020
I've also added this issue to the 3.0 feature planning project, as @drdavella mentioned above v3.0 is the best version to deal with the minimal impact on our downstream it given the big overhaul we already do with the versions.
This issue needs to be updated to reflect recent progress. In particular, #6606 was a big step forward.
I also wanted to mention here another consideration for simplifying the test runner if possible. Currently we are not able to use
tests_requireinsetup.pyto reflect testing dependencies. This is because the packages included intests_requireare installed locally and added to the path thatsetup.pysees when it is run. However, our test runner actually runs in a subprocess created bysetup.py, and the test dependency path is not passed to the subprocess.This has come up in at least one other issue (#6787), and has been mentioned elsewhere (#6811).
cc @pllim
Reacted by P. L. LimIt seems that this issue is mostly done, with the exception of the removal of the test runner, which is a bit controversial. As I recall there were discussions about it elsewhere, e.g. in the APE discussion about removing the helpers: astropy/astropy-APEs#52
If we still need an issue to hold discussions about the test runner itself I suggest to open a separate one, dedicated to that topic only.
Thank you again @drdavella for leading these efforts, our whole infrastructure became much better because of your work.
Purpose
This issue is intended to serve as a forum for discussing future directions and improvements to astropy's testing infrastructure. It attempts to aggregate related testing issues, and I anticipate that new, more tightly scoped, issues may result from this discussion.
Motivation
Test Plugins
It seems like there is general consensus that moving astropy's
pytestplugins into separate packages would be good and useful. This would make them available a la carte to affiliated packages (and even non-affiliated packages). It would help minimize the testing infrastructure we maintain since it aligns with the standardpytestmechanism for plugin distribution. Some work has already been done towards this end (#6384), although more work is required.Invoking Pytest Directly
Equally important, I would argue, is the ability to run tests directly from
pytest, rather than relying exclusively on./setup.py. This currently does not work (#6424), which I would argue is a substantial shortcoming that needs to be addressed. Some work has been done to address this (#6437), but it has become clear that this will require a non-trivial refactoring of the testing infrastructure.One reason I believe this is important is that it is the expected behavior: a project that uses
pytestfor testing should be able to just usepytestfor testing (#5402).Another argument is a little more subtle: I believe that the fact that we do not currently support invoking
pytestdirectly means that we don't use it as it is designed to be used. In addition to being somewhat unsatisfying, I believe this makes our testing code brittle and potentially unstable in the long term. It also makes it difficult to identify testing-related issues since they might be hidden by the way that we run our tests.To address an outstanding issue (#6392), since all
pytestfunctionality is available directly from thepytestcommand line interface, the need to develop a pass-through for our./setup.pyrunner is eliminated. This is good, because after looking into this issue I believe that such a pass-through would be particularly difficult to implement.Other Considerations
With the move to exclusive Python 3 support in v3.0, we have an opportunity to also make potentially substantial improvements to the testing infrastructure. In considering these issues at the same time, we can hopefully minimize the number of times that affiliated packages will have to update the way that they use astropy.
Proposal
Given the motivating assumptions above, here is an outline of the changes that I would like to propose:
pytestplugins to isolate specific functionality as much as possible (see Allow test suite to run directly from pytest #6437)pytestplugins into separate packagespytestplugin mechanism for astropy-specific featurespytest(see Update config file in anticipation of future testing updates #6449, Fix modules in anticipation of future testing updates #6450, and Allow test suite to run directly from pytest #6437)Update all tests to import astropy with absolute importsThis discussion has been moved to Relative vs absolute imports in tests #6452Moving the Plugins
The motivation for moving the plugins has been treated above. I believe this would be fairly straightforward and would require relatively small changes to astropy and affiliates.
The Test Runner
In addition to moving the plugins out of the astropy core, I argue that it would also be beneficial to move the test runner out of the core package as well. Testing is sufficiently general to make it useful to other packages without requiring the core as a hard dependency. The astropy-helpers package seems like a reasonable destination for this functionality, although a separate test package could also be useful. This was proposed in #6362, which should probably be reopened since it is distinct from the work started in #6384.
However, it is worth considering whether the astropy test runner still provides a clear benefit over integrating pytest directly with setuptools. Our test runner is fairly complicated; it is also redundant with some of the functionality provided by astropy-specific
pytestplugins (with respect to argument processing in particular). If it turns out that all of the test runner's functionality could be provided through the plugin mechanism, then I would argue that there is a strong case for doing away with it altogether in an effort to simplify the code base.It is worth noting that either moving the test runner to another package or removing it altogether appears to be necessary in order to allow tests to run from
pytestdirectly. To make a long story short, any test runner that needs to import astropy itself before runningpytest.mainis going against the grain of howpytestintends to be used.In addition to the other benefits of removing the test runner, it would mean that we don't need to separately test
setup.pyoptions since they are provided by a standard interface (#5806).Using Absolute Imports
This discussion has moved to #6452.
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.Questions and Considerations
There are some questions that should be answered before moving forward:
pytestplugin mechanism?pytestas well?import astropy; astropy.test()Tasks
Below is a first pass at a list of discrete tasks that could potentially be broken into individual issues and PRs:
update all tests to use absolute importsSee Relative vs absolute imports in tests #6452pytestplugins so that each plugin performs a unique, independent function./setup.pyandconftest.pyfiles)setup.cfgin anticipation of usingpytestdirectly (Cannot retrieve gzipped tables from CDS using astropy.table #6549)pytestdirectly (Updating pytest minversion in setup.cfg #6550)Depending on whether the test runner can be removed or not:
pytestplugin(s) to replicate test runner functionality