Allow test suite to run directly from pytest - #6437

Hi there @drdavella 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labelled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

I noticed the following issue with this pull request:

• Changelog entry section (v2.0.2) inconsistent with milestone (v2.0.3)

Would it be possible to fix this? Thanks!

I've added 2.0.2 as anticipatory milestone, however this seems to be a big overhaul, so I'm not sure whether it should indeed go into the bugfix rather the next major. If it goes into the bugfix we definitely need to make sure it doesn't break anything for affiliates.

@bsipocz that's perfectly fine, just let me know if/when you need me to update the change log.

I will try and review this in more detail once I'm back from holidays (in a week or so), but if possible I think it would be nice to have a way to define the plugins to use in setup.cfg instead of setup.py (for affiliated packages we try and make sure as much as possible is defined in setup.cfg so that they can all share the same setup.py file).

Plugin discovery actually requires astropy to be installed in development mode. Previously the test command automatically built the package, but this is no longer sufficient.

Not sure if CircleCI failure is related. I am attaching the log here because CircleCI made me download it anyway (400+ KB) so might as well:
build_2661_step_12_container_0.txt

Should really test this against an affiliated package (http://www.astropy.org/affiliated/). For example, photutils (since if that test break, you can just hunt down Larry down the hall to figure things out).

If this breaks compatibility with affiliated packages, here are my thoughts on how to go forward (FWIW):

• Keep the test runner as-is (i.e., reject this PR).
• However, using the way you have broken things down into plugins here, actually go ahead and make separate plugin packages.
• Close this PR (unmerged) and issue a new PR for Astropy to use the new plugin packages.
• Then deprecate Astropy's built-in stuff and encourage affiliated package maintainers to switch over.
• Remove deprecated stuff after sufficient future release (as per Addendum to APE 2 astropy-APEs#20). At that point, all affiliated packages (and whoever uses astropy-helpers will have to switch to using the new separate pytest plugins).

I would suggest to test it against multiple affiliated or otherwise related packages. In addition to photutils, e.g. gammapy and sunpy can be a good testbed as they are already quite complicated ones.

Also if we go ahead with this, I would suggest to send out an e-mail to astropy-dev as well as astropy-affiliated-maintainers list to ask maintainers to do some testing before we merge this in.

It sounds like something we shouldn't rush to merge, and definitely agree about emailing astropy-dev (maybe a decision could be taken when the core maintainers meet in september)

I do personally like the idea of just releasing separate individual pytest plugin packages as @pllim suggest.

@pllim, I think that sounds like a reasonable approach. Do you think it makes sense in the meantime to submit a separate PR just to reorganize the plugins within astropy without changing the way they are loaded? In my view this just builds on the work that @Cadair did in #6384.

I should point out that I believe it will not be possible to have the plugins loaded in both astropy/conftest.py as they are currently, and also in setup.py, so we will need to be careful about what the intermediate version looks like. It's possible that running tests directly from pytest will continue to be broken until we completely deprecate the current testing structure.

This probably isn't the right place to have this discussion but I want to mention a related train of thought:

• In addition to moving the plugins out of astropy and into a separate package, it would be useful to have astropy's test runner machinery be independent of astropy itself so that other packages (e.g. asdf) can use it without having a hard dependency on the astropy core.
• However, I'm wondering whether astropy's test runner still provides a clear benefit over running pytest directly from setup.py test using pytest-runner. There is a lot of fairly complicated and potentially redundant machinery in the astropy test runner. I think we're not really running pytest the way it's intended to be used, which causes some weird issues and possibly decreases stability in the long term. I'd like to discuss (in a separate forum) whether we can provide the functionality we need simply through the plugin mechanism and deprecate the runner altogether.

Incidentally, I believe the 'quirks' I mentioned were actually real issues that should be addressed in a separate PR. In particular, I think the file astropy/extern/configobj.py needs to be deleted, and the way that cosmology parameters are exposed in astropy/cosmology/parameters.py does not appear to work under certain circumstances. These issues were exposed when running the tests directly from pytest.

@astrofrog, that's a good idea to make use of setup.cfg. Should I add a prototype to this PR or should I wait until we sort out the overall plan for these changes?

@astrofrog, @bsipocz, @pllim, I've been looking at testing a lot recently (in case you couldn't tell) and have a lot of related thoughts/questions/issues that I'd like to discuss. Is astropy-dev the right place for these? Should I open an issue here? Should I think about writing an APE?

@astrofrog, @bsipocz, @pllim, I've been looking at testing a lot recently (in case you couldn't tell) and have a lot of related thoughts/questions/issues that I'd like to discuss. Is astropy-dev the right place for these? Should I open an issue here? Should I think about writing an APE?

either of those would work, though I slightly prefer github or an APE rather than astropy-dev. Will you be around during the coordination meeting (either locally or remotely)? I think this is a topic that would hugely benefit of a discussion slot there.

I think it would make sense to have an in-person or hangouts discussion, and once there is some consensus, an APE could be written to formalize this (since it would be a big change).

Also just a quick note - regardless of what we do with the pytest plugins, the main motivation for python setup.py test is that running pytest only works if separately compiling astropy beforehand with e.g. python setup.py build_ext --inplace - unless pytest now has a mechanism for that too?

@drdavella

It's possible that running tests directly from pytest will continue to be broken until we completely deprecate the current testing structure

Yes, it is entirely possible and it is not a deal-breaker as this is a long-known issue anyway, so no one is going to cry over something that never really worked being not working still.

it would be useful to have astropy's test runner machinery be independent of astropy itself

I agree with this completely. It was actually one of the GSoC 2017 projects proposed, but unfortunately not accepted.

using pytest-runner

Personally, I am not opposed to complete overhaul if it is deemed necessary and someone has the time to do it. I believe a lot of these were written back in Astropy's infancy, when these pytest features that are available now were not available back then. But I agree with others above that such a big change should be discussed first before being worked on.

should I wait until we sort out the overall plan for these changes?
Is astropy-dev the right place for these? Should I open an issue here? Should I think about writing an APE?

In-person discussions during coordination meeting would be very useful indeed. For the rest of the community, APE seems appropriate (although I sometimes dislike how slow things go over there).

the 'quirks'

If they are actually self-contained bugs that can be fixed without overhauling our pytest infrastructure here, then please open separate PRs (one for configobj and one for cosmology).

While we're at this topic, though unrelated to this particular PR specifically, I find that some astropy runner (setup.py test) features are at odds with expected pytest behaviors:

1. Passing in --args="--lf" to only rerun failed test from last run does not seem to work. (Or is it just me?)
2. When using together with --args="--html=report.html" (with the pytest-html plugin), the report is stored at /tmp/stsynphot-test-1ahee8se/lib.linux-x86_64-3.5/report.html, and promptly deleted upon completion together with the other results. While there are ways around this one (by providing absolute path to somewhere else), it still seems undesirable.

I am not saying you should fix these as well, but perhaps they support your case for re-implementation of Astropy test runner above.

@astrofrog I'll do some research (unfortunately setuptools documentation is fairly thin) but I think it's true that running ./setup.py test automatically treats build_ext as a dependency, and I would expect that to be the case even when using pytest-runner. Maybe I'm wrong about that though. For users who run pytest directly, I think it will be necessary to manually rebuild extensions, but think that is the case for any package with C extensions that is tested using pytest.

[edit]
@astrofrog after re-reading I see that I missed your point entirely. However, I do think it's the case that even without the astropy test runner, if we use pytest-runner to enable ./setup.py test it should automatically handle the build_ext target as a dependency. Users who want to call pytest directly should be aware that extensions need to be rebuilt manually, and we can add documentation saying as much. In any case, it doesn't seem like this will affect most users/developers though, is that correct?

[another edit]
It also may be possible to write a pytest plugin that will automatically rebuild extensions.

I agree that this probably shouldn't be integrated immediately and that we need to have more discussion about it. I hope to be able to attend the coordination meeting but am waiting for funding to be approved.

In the meantime, I thought I would share the results of some testing against affiliated packages:

• sunpy appears to work out of the box (after getting through some unrelated build issues)
  • Invoking pytest directly appears to work with sunpy as well (after making a very minor tweak to norecursedirs in setup.cfg). The tests all run but I observed a lot of failures in this case and didn't spend time tracking down the issue. However, I have noticed that in general test collection appears to be different when using pytest versus ./setup.py test, and I think the failures I observed might be related.
• gammapy works after a minor modification to gammapy/conftest.py, which simply reflects the updated layout of the astropy.tests package.
  • Invoking pytest directly also works (and all tests pass) after making some similar modifications to setup.cfg
• photutils required some similar changes to photutils/conftest.py but seemed to work after that (although the doctest-rst tests failed, which needs to be investigated)
  • Invoking pytest directly also works after minor modifications to setup.cfg

All-in-all, it seems like fairly minor changes could get most packages working with these changes. However, there are some issues that need to be investigated first. In particular, it looks like the doctest-rst plugin might not work as expected for affiliated packages.

@pllim, thanks for the log from CircleCI. It's not obvious to me either whether the failure is related, but I'll try to see if I can reproduce it locally.

Superseded by #6606