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

Deprecate astropy test runner in core and downstream #16177

Open
pllim opened this issue Mar 7, 2024 · 11 comments
Open

Deprecate astropy test runner in core and downstream #16177

pllim opened this issue Mar 7, 2024 · 11 comments

Comments

@pllim
Copy link
Member

pllim commented Mar 7, 2024

This is what some people call "an epic" in JIRA-land.

A long, long time ago, in the same galaxy, astropy existed when Python was youngish, pytest was younger (remember when we bundled pytest as a single file?) , CI on GitHub was almost non-existent (ask Tom R about Jenkins). There was a need to be able to test installed versions of astropy. And thus, the test runner was created. For many years, it served us well.

class TestRunner(TestRunnerBase):
"""
A test runner for astropy tests.
"""

test = TestRunner.make_test_runner_in(__path__[0])

We even advertised it in package-template (https://github.com/astropy/package-template/blob/36ed48db74c9a0bfcf214e14a1a3fb1fdaae0c6c/%7B%7B%20cookiecutter.package_name%20%7D%7D/%7B%7B%20cookiecutter.module_name%20%7D%7D/_%7B%7B%20cookiecutter._parent_project%20%7D%7D_init.py#L13), so downstream packages may have similar runners too (whether they realized it is there or not is another issue).

Now in 2024, we have multi-architecture CI and wheels. The need for such a test runner has diminished greatly. In fact, having it might even hinder progress like these:

Additionally, it is not aging well now that pytest is very complicated and there are plenty of pytest plugins available:

All the while, it was never feature complete:

The plea to get rid of the test runner went as far back as 2018 (#8153 (comment)), "Friends, I beg of you, let's please consider axing the test runner entirely."

Proposal

  1. Deprecate astropy.test() function and astropy.tests.runner module.
  2. Open issues in affected downstream packages to warn them to check whether they inherited this test runner from package template (or added it intentionally) and tell them to move away from using it. Alternative is to invoke pytest directly (or tox, whatever).
  3. After a deprecation period, remove the deprecated stuff from astropy. If downstream packages have not caught up, they need to pin to an older release of astropy or have a broken test runner.

Impact: High!
Benefit: Less technical debt to carry forward.
Cost: Some crying maintainers.

Counter proposal

We keep everything as-is, but we have to start testing astropy.test() in our CI.

@namurphy
Copy link
Contributor

namurphy commented Mar 7, 2024

I did a search for "astropy.test()" in Python files across GitHub and got 76 results. A similar search for "astropy.tests.runner" in Python files across GitHub got 132 results. (Checking on this was waaaay more fun than timing Sphinx builds.)

@MridulS
Copy link
Contributor

MridulS commented Mar 7, 2024

Thanks for the write up @pllim! I wonder who is the audience for the deprecation? Can we remove astropy.test() directly and only deprecate TestRunner? Assuming the hypothesis that an end user doesn't really need to run astropy.test() today.

There are a bunch of packages which use the TestRunner code downstream but from a quick glance it seems like there isn't much magic happening in the downstream packages, it just directly imports TestRunner and then exposes a test function in the package namespace, so the change hopefully wouldn't be that painful!

I did a search for "astropy.test()" in Python files across GitHub and got 76 results.

If we remove all the forks on github, the only active place astropy.test() seemed to be used is in https://github.com/spack/spack/blob/31de670bd26beca979ebd75dcb0ce90c535a78c4/var/spack/repos/builtin/packages/py-astropy/package.py#L126 and they do support running tests directly with pytest.

@pllim
Copy link
Member Author

pllim commented Mar 7, 2024

Thanks for the searches, y'all!

We advertise astropy.test() in every RC testing (e.g., https://github.com/astropy/astropy/wiki/v6.0-RC-Testing), so I think deprecation is safer bet than straight removal. But whether astropy.test() is considered "public API" or not perhaps could be debated, especially since the public API APE is still WIP:

@pllim
Copy link
Member Author

pllim commented Mar 7, 2024

@mhvk
Copy link
Contributor

mhvk commented Mar 7, 2024

I agree we can stop using the test runner (and deprecate the TestRunner class). The main thing really would seem to have good instructions for how people can test astropy without it. The nice thing about our current test instructions at https://github.com/astropy/astropy/wiki/v6.0-RC-Testing is that we do not have to worry at all about what virtual environment people have, etc. But I think one can replace the second line with just a tox incantation, no?

@saimn
Copy link
Contributor

saimn commented Mar 8, 2024

The nice thing about our current test instructions at https://github.com/astropy/astropy/wiki/v6.0-RC-Testing is that we do not have to worry at all about what virtual environment people have, etc. But I think one can replace the second line with just a tox incantation, no?

As long as tests are installed, one can use pytest --pyargs astropy.
Should be quite similar to astropy.test() without a local clone.

@saimn
Copy link
Contributor

saimn commented Mar 8, 2024

A search for make_test_runner_in without forks gives 84 results, seems to give a good list of packages importing the TestRunner (i.e. using the template).

@pllim
Copy link
Member Author

pllim commented Mar 8, 2024

As long as tests are installed

But how, if the wheels no longer come with tests?

@mhvk
Copy link
Contributor

mhvk commented Mar 8, 2024

Presumably pip install astropy[test] would pull in astropy-tests too!

@pllim
Copy link
Member Author

pllim commented Mar 8, 2024

Oh, we talking about moving the tests out of this repo too? 😱

@Cadair
Copy link
Member

Cadair commented Mar 8, 2024

SunPy 🔥'ed the test runner a long time ago, it's very easy to just call pytest.main and get the same effect: https://github.com/sunpy/sunpy/blob/main/sunpy/tests/self_test.py#L73

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants