Skip to content

Remove unhelpful helper usage from a test file - #16644

Merged
pllim merged 1 commit into
astropy:mainfrom
eerovaher:io-ascii-test-cleanup-example
Jul 1, 2024
Merged

pllim merged 1 commit into
astropy:mainfrom
eerovaher:io-ascii-test-cleanup-example

Conversation

@eerovaher

Copy link
Copy Markdown
Member

Description

io.ascii tests define a few helper functions that apparently provided compatibility with nose, but are not needed anymore (as reported in #15782). This pull request removes their usage from one test file. It also serves as an example how these (now-)useless helpers should be replaced in other files.

  • 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.

`io.ascii` tests define a few helper functions that apparently provided
compatibility with `nose`, but are not needed anymore.
@github-actions

github-actions Bot commented Jul 1, 2024

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.

@pllim

pllim commented Jul 1, 2024

Copy link
Copy Markdown
Member

I see lots of open PRs linked to #15782 . Those probably would be outdated after this one is merged and should be closed?

@eerovaher

Copy link
Copy Markdown
Member Author

All the other pull requests addressing #15782 seem to be abandoned. If there was any reasonable hope that any of them would be finalized then there would have been no need to open this example pull request.

@taldcroft taldcroft 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.

LGTM

@pllim
pllim merged commit c1f97c3 into astropy:main Jul 1, 2024
@pllim

pllim commented Jul 1, 2024

Copy link
Copy Markdown
Member

Thanks, all!

@eerovaher
eerovaher deleted the io-ascii-test-cleanup-example branch July 1, 2024 19:58
@dhomeier

dhomeier commented Jul 2, 2024

Copy link
Copy Markdown
Contributor

I think we had an extensive discussion especially within #15782 in which cases numpy.testing.assert_equal would provide more helpful diagnostics than assert x == y. Would have been a good opportunity to add these, but we should probably keep a reference to it.

@eerovaher

Copy link
Copy Markdown
Member Author

The replacements that are suggested in the issue and applied here are simple and never make the tests worse. It is true that in some cases more extensive edits could improve our tests even more, but the issue has proven to be confusing enough for new contributors (I don't understand why, but empirically it is clear that it is so), therefore it is good to keep instructions and examples simple. More sophisticated follow-up can be done separately.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants