-
-
Notifications
You must be signed in to change notification settings - Fork 671
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
Run testbed with Wayland #2670
Run testbed with Wayland #2670
Conversation
0927f44
to
6e9e522
Compare
This is the approach I landed on today. Feel free to let me know if this strategy makes sense to you...or doesn't. From here, the failing tests just need to be addressed one way or another. May also benefit from combining the duplicated matrix fields for the two Linux jobs. |
I guess the thing that doesn't make sense is the use of xvfb-run... are you sure it's actually running as Wayland? There's a bunch of functionality that should be tested (like getting an image of the current screen), but that isn't showing up as test failure as I'd expect. Other than that (and the other miscellaneous test failures), the approach looks fine to me. |
I'm relatively confident. If you run
Are you thinking of this one or a different one?
In general, I was surprised this ended up requiring |
That test failure is somewhat expected - the canvas rendering tests are highly platform dependent. I was thinking more of this test - based on the implementation, that shouldn't be able to pass. |
ahh...in that case, pytest appears to be skipping the test.
|
Huh - that's some forward thinking that I don't remember :-) |
2aad2ec
to
d95b5f6
Compare
309a96a
to
3f0d865
Compare
def get_image_data(self): | ||
if "WAYLAND_DISPLAY" in os.environ: | ||
if "WAYLAND_DISPLAY" in os.environ: # pragma: no cover | ||
# Not implemented on wayland due to wayland security policies. | ||
self.interface.factory.not_implemented("Screen.get_image_data() on Wayland") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is left as blanket "no cover" because the probe prevents fetching an image for the screen if running on Wayland. However, I imagine that's because Toga crashes out if this code is ever actually ran in practice....because the Screen.as_image()
API in core
sends the returned None
in to toga.Image
. This may not be the best UX...especially since developers may never test on Wayland before shipping...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed it's not ideal UX; returning a dummy image (blank, same size as the screen?) may be preferable. Up to you whether you roll that into this PR, or log it as a standalone issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created #2673 to capture this.
gtk/tests_backend/app.py
Outdated
def assert_current_window(self, window): | ||
# Gtk 3.24.41 ships with Ubuntu 24.04 where present() works on Wayland | ||
if self.IS_WAYLAND and self.GTK_VERSION < (3, 24, 41): | ||
pytest.skip( | ||
f"Assigning the current window is not supported for " | ||
f"Gtk {'.'.join(map(str, self.GTK_VERSION))} on Wayland" | ||
) | ||
else: | ||
assert self.app.current_window == window |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is likely an older version of Gtk that works on Wayland to limit this with....but Gtk development is too opaque for me to try to track that down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with this. There's so little consistency in Linux testing environments that I'm OK with this being a skip. If someone wants to suggest the limit can be lower, then can submit a PR.
However, given the actual assertion is a trivial comparison, I wonder if it might be better to handle this as a feature flag on the probe (i.e., a supports_current_window_assignment
), rather than an abstracted assertion. Having 5 identical implementations of a trivial comparison, and one that is the same identical implementation with a skip check seems excessive. This would also allow us to continue the tests that perform a current-window check, but in the context of other useful behavior (i.e., we could make a decision on a per-test basis whether we want to skip the test, or ignore the check entirely.=)
Along with allowing for testing Wayland in CI, this should also allow devs to more easily run the testbed tests locally. However, multiple monitor setups where the primary monitor is not the far left monitor will likely still fail. This kinda makes me wonder if all the CI logic to set up the environment to run the testbed tests shouldn't be in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of minor details, but otherwise this looks really good. The --ci
affordance is especially nice - getting a clean test run by default when we know a test has platform issues is a nice addition.
def get_image_data(self): | ||
if "WAYLAND_DISPLAY" in os.environ: | ||
if "WAYLAND_DISPLAY" in os.environ: # pragma: no cover | ||
# Not implemented on wayland due to wayland security policies. | ||
self.interface.factory.not_implemented("Screen.get_image_data() on Wayland") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed it's not ideal UX; returning a dummy image (blank, same size as the screen?) may be preferable. Up to you whether you roll that into this PR, or log it as a standalone issue.
gtk/tests_backend/app.py
Outdated
def assert_current_window(self, window): | ||
# Gtk 3.24.41 ships with Ubuntu 24.04 where present() works on Wayland | ||
if self.IS_WAYLAND and self.GTK_VERSION < (3, 24, 41): | ||
pytest.skip( | ||
f"Assigning the current window is not supported for " | ||
f"Gtk {'.'.join(map(str, self.GTK_VERSION))} on Wayland" | ||
) | ||
else: | ||
assert self.app.current_window == window |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with this. There's so little consistency in Linux testing environments that I'm OK with this being a skip. If someone wants to suggest the limit can be lower, then can submit a PR.
However, given the actual assertion is a trivial comparison, I wonder if it might be better to handle this as a feature flag on the probe (i.e., a supports_current_window_assignment
), rather than an abstracted assertion. Having 5 identical implementations of a trivial comparison, and one that is the same identical implementation with a skip check seems excessive. This would also allow us to continue the tests that perform a current-window check, but in the context of other useful behavior (i.e., we could make a decision on a per-test basis whether we want to skip the test, or ignore the check entirely.=)
gtk/tests_backend/widgets/canvas.py
Outdated
if self.IS_WAYLAND: | ||
return f"{reference}-gtk-wayland" | ||
else: | ||
return f"{reference}-gtk" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to differentiate -x11
from -wayland
in the naming here?
Couple questions I was thinking about:
|
I guess it could be helpful. You don't need to run the actual CI configuration very often, but when you do, it would be nice to have an easy way to replicate it without needing to spelunk through the CI YAML to replicate it.
I've had the same thought myself recently. I've been breaking the dialog tests into their own modules for similar reasons; given the number of mobile vs desktop tests, it makes sense to break them out as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Splitting the mobile/desktop tests into two parts can be a separate PR - in fact, I might end up rolling it into the #2244 landing sequence that I'm in the middle of.
Changes
Notes
weston
,sway
,mutter
, and even a few others todayweston
works well....until you run in headless mode; then Gtk throws errors for no wayland "seat"sway
seems much more limited; for instance, I don't think it supports overlapping windowsmutter
was really the only one that provided a robust testing environmentmutter
's headless mode upsets Gtk...butmutter
is able to run inxvfb-run
unlike theweston
andsway
xvfb-run
...but it seems like a viable testing strategy.PR Checklist: