Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Don't test napari main #277

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tlambert03
Copy link
Member

I'm afraid it's a bit too hard to maintain running tests against napari main on each PR. napari certainly needs to be able to be flexible in writing its own tests, but unfortunately it too often breaks here. this PR no longer tests against napari main, but does still test against an old version (v0.4.19.post1) and a new version (v0.5.6)

Copy link

codecov bot commented Jan 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.28%. Comparing base (7850e53) to head (0aa6284).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #277      +/-   ##
==========================================
+ Coverage   87.14%   87.28%   +0.14%     
==========================================
  Files          47       47              
  Lines        3554     3554              
==========================================
+ Hits         3097     3102       +5     
+ Misses        457      452       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@psobolewskiPhD
Copy link
Contributor

psobolewskiPhD commented Jan 27, 2025

@tlambert03 So it's something about the pyvista action that is different from the combo of
tlambert03/setup-qt-libs@v1
aganders3/headless-gui@v2
When I try those two instead (https://github.com/psobolewskiPhD/workflows/blob/patch-1/.github/workflows/test-dependents.yml) then the tests pass:
https://github.com/psobolewskiPhD/superqt/actions/runs/12981609361/job/36200274303?pr=1

(ignore the 0.4.19 failure)

@psobolewskiPhD
Copy link
Contributor

I compared the apt installs and they are the same, so it's something in the headless-gui part.
pyvista uses :99 and the aganders3/headless-gui starts with 0 and looks for a free one?
https://github.com/aganders3/headless-gui/blob/fbd407c863dc613326c425c219d85507cd886a5f/src/start-xvfb.bash#L9-L21

The resolution difference is not relevant, I tested that...

@jni
Copy link

jni commented Jan 27, 2025

I'm afraid it's a bit too hard to maintain running tests against napari main on each PR. napari certainly needs to be able to be flexible in writing its own tests, but unfortunately it too often breaks here.

Can you point us to where things break/broke? And/or how we could ensure compatibility on our end to reduce the burden here? We absolutely don't want to be an extra burden on superqt, but it's (super) important to us to stay compatible...

@tlambert03
Copy link
Member Author

Can you point us to where things break/broke?

it's actually still broken on this PR, which I guess means it was already broken with the latest release from last week. See the failing tests here. There's something about the check_white_scale_bar test that doesn't work here. I haven't looked yet... maybe it's expecting a specific size of the offscreen buffer?

I've been just continuing to add more ignores to this line here, but it happens somewhat frequently and i was hoping to reduce that frequency.

if anyone from napari wants to set up the workflows here to use your specific reusable workflows, that would be welcome.

@jni
Copy link

jni commented Jan 27, 2025

See the failing tests here.

Got it, thanks. Seeing the test checking for [1, 1, 1, 255] is extremely suspicious right off the bat. 😅 Did this maybe depend on some on-screen interpolation??? We'll investigate on our end.

I've been just continuing to add more ignores to this line here,

oh god 😂

if anyone from napari wants to set up the workflows here to use your specific reusable workflows, that would be welcome.

I think that is probably the best solution, because otherwise you're just pushing the problem to releases, which is better but still a pain to work through, as we are seeing. I'll try to make some time for it tomorrow. 🙏

@tlambert03
Copy link
Member Author

yeah, not sure... it's conceivable it depends on the screen size settings in aganders3/headless-gui: https://github.com/aganders3/headless-gui/blob/fbd407c863dc613326c425c219d85507cd886a5f/src/start-xvfb.bash#L24-L26

@psobolewskiPhD
Copy link
Contributor

I tested setting a smaller size -- same as pyvista. The test passes:
https://github.com/psobolewskiPhD/superqt/actions/runs/12981902437/job/36200978172#step:9:97

@tlambert03
Copy link
Member Author

by the way, @jni ... it came up with @psobolewskiPhD elsewhere, but you may be interested to know that https://github.com/pyvista/setup-headless-display-action is a pretty nice one-stop-shop that would let you replace three different actions:

  • my setupqt-libs action used here
  • setup of windows opengl used here
  • aganders3 xvfb action used here ... and I particularly like that I don't need to pass the actual run command into with: (which also means you don't need to specify the shell)

@tlambert03
Copy link
Member Author

thanks @psobolewskiPhD!

@psobolewskiPhD
Copy link
Contributor

I guess the ideal outcome could/would be to sort out how to get the pyvista action working with napari tests and then upstream the change?

@tlambert03
Copy link
Member Author

yeah, or even just use it directly yourself? :)

@tlambert03
Copy link
Member Author

actually @psobolewskiPhD I don't think it's the screen size... what you use in your patch is the same thing they use

@psobolewskiPhD
Copy link
Contributor

psobolewskiPhD commented Jan 27, 2025

Sorry I wasn't clear -- napari actions use the larger default from the headless-gui action. I switched to the smaller default of the pyvista action in my patch to see if that would break stuff.
It didn't so the only other difference I see is DISPLAY 0 vs 99.0.

Edit: the issue with us using the pyvista action would be the failing test though 🤣

@tlambert03
Copy link
Member Author

napari actions use the larger default from the headless-gui action

ahh... ok, that makes, confirms that the tests depends on the specific screen size

@tlambert03
Copy link
Member Author

Edit: the issue with us using the pyvista action would be the failing test though 🤣

true 😂 ... i guess the ultimate fix is to have tests that don't depend as critically on the size of the display? i know, it's hard stuff

@psobolewskiPhD
Copy link
Contributor

psobolewskiPhD commented Jan 27, 2025

napari actions use the larger default from the headless-gui action

ahh... ok, that makes, confirms that the tests depends on the specific screen size

Does it? I ran the test with my patch with both sizes and both pass with the headless-gui action.
First with the headless-gui default (1280x1024x24) then the pyvista default (1024x768x24).
🤷

@tlambert03
Copy link
Member Author

tlambert03 commented Jan 27, 2025

Does it? I ran the test with my patch with both sizes and both pass with the headless-gui action.
First with the headless-gui default (1280x1024x24) then the pyvista default (1024x768x24)

ah, well no. if it doesn't matter what screen size you put in, then no, that indeed argues something different 😂.

I don't know. in summary: going back to the original proposal. i'm basically happy to use any setup here that you want me to if someone from the napari team wants to maintain superqt's testing of napari. I just want something that I don't have to update as often as I have been

@Czaki
Copy link
Contributor

Czaki commented Jan 27, 2025

So it means that I should work again on napari/napari#6336

@tlambert03
Copy link
Member Author

yep, if that's your preferred strategy. I had been hoping to be able to use the general pyapp-kit reusable workflows, but napari is complex and fluid enough that I'm happy to use anything that napari developers want to maintain :) 👍

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

Successfully merging this pull request may close these issues.

4 participants