Skip to content

TEST: ensure Masked tests run also without matplotlib installed#16843

Merged
pllim merged 1 commit intoastropy:mainfrom
mhvk:mask-test-fix
Aug 17, 2024
Merged

TEST: ensure Masked tests run also without matplotlib installed#16843
pllim merged 1 commit intoastropy:mainfrom
mhvk:mask-test-fix

Conversation

@mhvk
Copy link
Contributor

@mhvk mhvk commented Aug 17, 2024

This pull request is a subtle bug introduced in #16820, where an import of MATPLOTLIB_LT_3_8 in a masked test file causes the tests to be skipped altogether if matplotlib is not installed.

In review, should make sure that the masked tests are all run also in the minimal dependencies case.

@github-actions
Copy link
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.

Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Ops, sorry about that!

@pllim pllim merged commit c50aea4 into astropy:main Aug 17, 2024
@mhvk
Copy link
Contributor Author

mhvk commented Aug 17, 2024

No worries, it was not at all obvious this would cause the tests to be skipped, rather than given an error!

@neutrinoceros
Copy link
Contributor

This appears to crash on daily cron (specifically on pyinstaller)
https://github.com/astropy/astropy/actions/runs/10446362063/job/28923664532

pass


# For grep'ing: test requires MATPLOTLIB_LT_3_8
Copy link
Contributor

Choose a reason for hiding this comment

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

didn't you mean "test requires not MATPLOTLIB_LT_3_8" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops...

@mhvk
Copy link
Contributor Author

mhvk commented Aug 19, 2024

This appears to crash on daily cron (specifically on pyinstaller) https://github.com/astropy/astropy/actions/runs/10446362063/job/28923664532

What? How is this possible. Does it evaluate it even if not HAS_PLT?

@neutrinoceros
Copy link
Contributor

Yes I think this condition is evaluated: matplotlib is indeed installed on this job, but somehow minversion just crashes with

importlib/metadata/__init__.py:548: in from_name
    ???
E   importlib.metadata.PackageNotFoundError: No package metadata was found for matplotlib

I never heard of pyinstaller before so I don't know off hand how this is happening but it's oddly specific to that env

@mhvk
Copy link
Contributor Author

mhvk commented Aug 19, 2024

It's weird that the previous version in which matplotlib was imported did work. Maybe easiest is just to revert to that...

@pllim
Copy link
Member

pllim commented Aug 19, 2024

Argh, sorry to cause so much trouble.

@pllim
Copy link
Member

pllim commented Aug 19, 2024

Hopefully #16854 is finally the correct fix? 🤞

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.

3 participants