Skip to content

tests: extend QtAgg figureoptions coverage (follow-up to #30128)#30587

Open
YQ157 wants to merge 9 commits into
matplotlib:mainfrom
YQ157:fix-legend-test-coverage
Open

tests: extend QtAgg figureoptions coverage (follow-up to #30128)#30587
YQ157 wants to merge 9 commits into
matplotlib:mainfrom
YQ157:fix-legend-test-coverage

Conversation

@YQ157

@YQ157 YQ157 commented Sep 19, 2025

Copy link
Copy Markdown

This PR extends the test suite for the QtAgg figureoptions dialog,
addressing coverage gaps reported on PR #30128 (Codecov) and related to issue #11109.

What’s included:

  • Parameterized legend scenarios (legend present/absent, visible/invisible)
  • Date axis handling
  • Lines labeled "nolegend" (skipped from legend)
  • Mappables without array data
  • Preserving existing legend when regenerate=False
  • Stronger assertions for redraw and navigation stack updates

The underlying implementation was originally proposed in #30128.
This PR only adds tests (with minimal supporting changes), to ensure more complete coverage
and to make it easier for maintainers to review and merge the fix.

Refs #30128, #11109


ℹ️ Note: This is my first open-source contribution.
It directly addresses the same problem described in issue #11109, which was marked as a good first issue.

@YQ157 YQ157 force-pushed the fix-legend-test-coverage branch 2 times, most recently from 85ae698 to a1434f3 Compare September 19, 2025 16:19
@YQ157 YQ157 closed this Sep 19, 2025
@YQ157 YQ157 reopened this Sep 19, 2025
@YQ157

YQ157 commented Sep 20, 2025

Copy link
Copy Markdown
Author

Only ubuntu-24.04-arm (Python 3.12) failed with test_webagg timeout. Other platforms passed. Could a maintainer please re-run the ARM job?

@YQ157

YQ157 commented Sep 20, 2025

Copy link
Copy Markdown
Author

Hi maintainers,

The only failing check is "Python 3.12 on ubuntu-24.04-arm", where test_webagg appears to time out after 120 s.
This seems to be an environment issue on ARM64 + Python 3.12 runners rather than caused by this PR.
Could you please override this job or let me know if I should skip the test?
Thanks for your help!

@YQ157

YQ157 commented Sep 22, 2025

Copy link
Copy Markdown
Author

All checks have passed ✅

This is not only my first contribution to Matplotlib, but also my first contribution to an open-source project.
I’m really excited to see it ready, and I’m looking forward to your review/merge.

Thanks a lot!

@QuLogic

QuLogic commented Sep 22, 2025

Copy link
Copy Markdown
Member

I'm a bit confused by the PR title; there are more files changed here than just tests. Also, please don't reformat entire files; that makes it difficult to see what your actual changes are.

@YQ157 YQ157 changed the title tests: broaden QtAgg figureoptions test coverage tests: extend QtAgg figureoptions coverage (follow-up to #30128) Sep 23, 2025
@YQ157

YQ157 commented Sep 23, 2025

Copy link
Copy Markdown
Author

I'm a bit confused by the PR title; there are more files changed here than just tests. Also, please don't reformat entire files; that makes it difficult to see what your actual changes are.

Thanks for the feedback!

I’ve reverted the accidental reformatting, so the diff should now be clean.
To clarify: the changes in figureoptions.py come from PR #30128 and are not new in this PR.
My contribution here is only the additions in test_backend_qt.py, which extend coverage for the QtAgg figureoptions dialog.
There are no new implementation changes beyond what #30128 proposed.

@rcomer

rcomer commented Sep 23, 2025

Copy link
Copy Markdown
Member

If this PR is meant to go on top of #30128 then I think we need to bring that to a conclusion before reviewing this one.

Add a second callback invocation with a 4-tuple mappable
(label, cmap, low, high) to exercise the `elif len(...) == 4`
branch in `figure_edit`. Use fresh copies of the input blocks
since the callback mutates them.
@YQ157

YQ157 commented Sep 23, 2025

Copy link
Copy Markdown
Author

If this PR is meant to go on top of #30128 then I think we need to bring that to a conclusion before reviewing this one.

Thanks for the clarification!

You are correct — this PR is based on #30128, and its purpose is to add the missing tests needed for its CI checks to pass.
I realize this creates a dependency loop, so I’m happy to follow whichever workflow works best for the maintainers to resolve both PRs.

Please let me know how I can help.

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

Projects

Status: Needs review

Development

Successfully merging this pull request may close these issues.

4 participants