Skip to content

RFC: Introduce pending deprecation warning of test runner#17874

Merged
pllim merged 4 commits into
astropy:mainfrom
pllim:test-runner-pending-depre
Jul 4, 2025
Merged

RFC: Introduce pending deprecation warning of test runner#17874
pllim merged 4 commits into
astropy:mainfrom
pllim:test-runner-pending-depre

Conversation

@pllim

@pllim pllim commented Mar 11, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Description

This pull request is to pave the way for formal deprecation of astropy test runner, as laid out in #16177 . Also a follow-up of #15204 .

xref #16208

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@pllim pllim added API change PRs and issues that change an existing API, possibly requiring a deprecation period Extra CI Run cron CI as part of PR Build all wheels Run all the wheel builds rather than just a selection labels Mar 11, 2025
@pllim pllim added this to the v7.1.0 milestone Mar 11, 2025
@github-actions

github-actions Bot commented Mar 11, 2025

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@github-actions

This comment was marked as outdated.

Sign in to view
pllim
pllim commented Mar 11, 2025
View reviewed changes
Comment thread astropy/tests/helper.py
Comment thread astropy/tests/runner.py
from astropy.utils.exceptions import AstropyDeprecationWarning, AstropyWarning

__all__ = ["TestRunner", "TestRunnerBase", "keyword"]
__all__ = ["TestRunner", "TestRunnerBase"]

@pllim pllim Mar 11, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keyword itself is a decorator, so it is weird to put a @deprecated decorator on a decorator. Not even sure why it is exposed in __all__. I think probably unintentional in the early days of astropy. Much easier to take it off public API. I don't think downstream should be using it on its own anyway.

@neutrinoceros neutrinoceros Mar 17, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so it is weird to put a @deprecated decorator on a decorator

I disagree, there's technically nothing wrong with decorating a decorator. The fact that it feels weird (it does !) isn't sufficient reason to remove public API without a warning. We've had enough problems with removing stuff that was explicitly private, I don't fancy the idea of knowingly removing stuff that's explicitly public :/

@pllim pllim Mar 17, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't like decorator on a decorator. Let's wait for others and see. Thanks!

@neutrinoceros neutrinoceros Jul 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hum, no other voice was raised, so I'm not sure how we can make a decision here. If it helps, I'm willing to let my suggestion ignored (assuming there are no known usage of keyword downstream), but we need to keep an eye out for sudden breakage and be ready to revert this bit to smooth out the transition.

Comment thread astropy/tests/runner.py Outdated
Comment thread astropy/tests/tests/test_run_tests.py
run_tests(pastebin="not_an_option")


def test_unicode_literal_conversion():

@pllim pllim Mar 11, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a leftover from Python 2 days. Now this test makes no sense to me. Definitely unrelated to test runner.

@neutrinoceros neutrinoceros Mar 17, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

@pllim pllim mentioned this pull request Mar 10, 2025
Closed
pllim
pllim commented Mar 11, 2025
View reviewed changes
Comment thread astropy/tests/tests/test_runner.py Outdated
@pllim

pllim commented Mar 11, 2025

Copy link
Copy Markdown
Member Author

linkcheck failure is unrelated.

@bsipocz

This comment was marked as resolved.

Sign in to view
bsipocz

This comment was marked as resolved.

Sign in to view

@neutrinoceros neutrinoceros mentioned this pull request Mar 12, 2025
Merged
1 task
@neutrinoceros

This comment was marked as resolved.

Sign in to view
@neutrinoceros

This comment was marked as resolved.

Sign in to view
@neutrinoceros

This comment was marked as resolved.

Sign in to view
@pllim

pllim commented Mar 12, 2025

Copy link
Copy Markdown
Member Author

I think this is because asdf-astropy pulls in asdf, and unfortunately, asdf itself ships its own pytest plugin within the package (as opposed to separate plugin package): https://github.com/asdf-format/asdf/blob/main/pytest_asdf/plugin.py

Even so, astropy/asdf-astropy#263 was merged 2 days ago. @bsipocz , does the error go away if you use a dev version of asdf-astropy? If not, I can dig more.

Thanks all for your feedback!

bsipocz
bsipocz reviewed Mar 12, 2025
View reviewed changes

@bsipocz bsipocz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, uninstalling asdf resolves the problem, so I'm not blocking this any more.

@bsipocz bsipocz dismissed their stale review March 12, 2025 15:45

Issue resolved

@pllim pllim marked this pull request as ready for review March 12, 2025 17:50
neutrinoceros

This comment was marked as resolved.

Sign in to view

@pllim

This comment was marked as resolved.

Sign in to view
@neutrinoceros

This comment was marked as resolved.

Sign in to view
@pllim

This comment was marked as resolved.

Sign in to view
@saimn

This comment was marked as resolved.

Sign in to view
@pllim

This comment was marked as resolved.

Sign in to view
@saimn

saimn commented Mar 13, 2025

Copy link
Copy Markdown
Contributor

To me putting the deprecation in 7.1 is fine.

@pllim

pllim commented Mar 13, 2025

Copy link
Copy Markdown
Member Author

OK. I will revert the API removal here. See #17883

@pllim pllim force-pushed the test-runner-pending-depre branch from 6d843f5 to 558ddfe Compare March 13, 2025 18:23
@pllim

This comment was marked as resolved.

Sign in to view
@pllim pllim changed the title RFC: Removed deprecated tests command, pending deprecation of runner RFC: Introduce pending deprecation warning of test runner Mar 13, 2025
@pllim pllim modified the milestones: v7.1.0, v8.0.0 Apr 21, 2025
@pllim

This comment was marked as resolved.

Sign in to view
@pllim pllim marked this pull request as draft April 21, 2025 17:45
@neutrinoceros neutrinoceros self-requested a review June 9, 2025 16:42
@pllim
RFC: Removed deprecated tests command, pending deprecation of runner
db6b023 
DOC: Update docs to reflect pending deprecation

TST: Update tests to catch AstropyPendingDeprecationWarning
@pllim pllim force-pushed the test-runner-pending-depre branch from 558ddfe to db6b023 Compare July 2, 2025 18:53
@pllim pllim modified the milestones: v8.0.0, v7.2.0 Jul 2, 2025
@pllim pllim marked this pull request as ready for review July 2, 2025 20:31
@pllim pllim requested review from a team and eteq July 2, 2025 20:31
neutrinoceros
neutrinoceros reviewed Jul 3, 2025
View reviewed changes

@neutrinoceros neutrinoceros left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two remaining comments, one of which I'm willing to let go. The other one should be easy to implement (use pytest.deprecated_call). Otherwise, LGTM

@pllim
TST: Also deprecate test runner call
3b8bb2d 
with AstropyPendingDeprecationWarning.

Update tests, runner docstring, docs.
@pllim pllim force-pushed the test-runner-pending-depre branch from 3ab089c to 3b8bb2d Compare July 3, 2025 17:28
neutrinoceros
neutrinoceros reviewed Jul 3, 2025
View reviewed changes
Comment thread astropy/tests/runner.py
Comment thread astropy/__init__.py
@pllim

pllim commented Jul 4, 2025

Copy link
Copy Markdown
Member Author

@mwcraig approved as well on Slack, so merging. Yeehaw. 🎆

@pllim pllim merged commit 0303742 into astropy:main Jul 4, 2025
97 of 98 checks passed
@pllim pllim deleted the test-runner-pending-depre branch July 4, 2025 02:58
@pllim pllim mentioned this pull request Jul 4, 2025
Merged
3 tasks
pllim added a commit to pllim/astropy that referenced this pull request Nov 3, 2025
@pllim
Officially deprecated things pending
d8341c7 
from astropy#17874
pllim added a commit to pllim/astropy that referenced this pull request Nov 3, 2025
@pllim
Officially deprecated things pending
56f8168 
from astropy#17874
pllim added a commit to pllim/astropy that referenced this pull request Nov 3, 2025
@pllim
Officially deprecated things pending
d622c92 
from astropy#17874
pllim added a commit that referenced this pull request Nov 4, 2025
@pllim
Remove deprecated tests/command.py module and really deprecate test r…
9b0b0f8 
…unner for v8.0 (#17883)

* Remove deprecated tests/command.py module

* Officially deprecated things pending from #17874
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bsipocz bsipocz bsipocz left review comments
@neutrinoceros neutrinoceros neutrinoceros approved these changes
@eteq eteq Awaiting requested review from eteq

Assignees

No one assigned

Labels

API change PRs and issues that change an existing API, possibly requiring a deprecation period Build all wheels Run all the wheel builds rather than just a selection Extra CI Run cron CI as part of PR Ready-for-final-review testing

Projects

None yet

Milestone

v7.2.0

Development

Successfully merging this pull request may close these issues.

4 participants

@pllim @bsipocz @neutrinoceros @saimn