TST: re-unify a testing function previously copy-pasted (and patched) accross 4 modules (⏰ wait for #17189) - #17051

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.

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

p.s. If we put it in astropy.tests.helpers, I think we might as well have a changelog entry advertising it.

I think the appropriate place may be astropy/tests/helper.py - this is also where assert_quantity_allclose lives.

seems like the natural place to put it indeed, I just notices that we could also make the following improvement then:

- assert quantity_allclose(a1, a2, rtol=1e-15)
+ assert_quantity_allclose(a1, a2, rtol=1e-15)

I'll push the polished version of the unified function now and I'll add a changelog later

The warning I get on RTD seems unrelated but I can't seem to reproduce it locally. In fact tox -e build_docs appears to be broken for a number o unrelated reasons. We should definitely address this but it's out of scope here.

@mhvk I should have addressed all your comments

Looking through PRs that are milestoned for 7.0, this one I think definitely should go in - it really is just a question where we think the more standardized function should live. I'm all for keeping things simple if that gets it done. @eerovaher - since you were the most critical, any suggestions on how to proceed?

The biggest problem is what I pointed out in #17051 (comment)

Thanks for reviving this discussion and sorry it took me another couple days to circle back to it.
At the moment, however, it is not clear to me what needs be done to move this forward.
Am I right to assume that all remaining concerns would vanish if we kept the newly unified test helper private ?

The first problem is that implementing MixinColumn as a Protocol is conceptually different enough from refactoring our tests that they should not be done in the same pull request. Case in point, fb29bda breaks the documentation build, but reducing code duplication in our tests should not affect our documentation at all.

The second problem is that if you want to implement public test helpers then they need to be very polished or else we will just lock ourselves into a bad API. For example, there should be separate assert_mixin_columns_allclose() and assert_mixin_columns_equal() functions just like there are separate numpy.testing.assert_allclose() and numpy.testing.assert_equal functions.

• I've taken out the Protocol definition to TYP: implement a MixinColumn typing.Protocol #17189¹ and rebased this branch on top of it
• I moved the function to a private name to address concerns about API polishing (which I think would also lead to inflate the scope of this PR beyond reason)

Admittedly this isn't enough to resolve the doc build failure, so I'll need to revisit. But for now, since this isn't critical, let me remove this from the 7.0 milestone. Let's resolve #17189 first.

Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

Hi humans 👋 - this pull request hasn't had any new commits for approximately 2 years. I plan to close this in a month if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.