GNOME Bugzilla – Bug 736505
Add a test framework and stacking tests
Last modified: 2014-09-12 17:19:49 UTC
As preparation for landing patches that rewrite large chunks of stack.c and stack-tracker.c, here's a test framework to allow testing things like whether when windows are raised, they actually raise, and whether we and the X server agree on whether they are raised.
Created attachment 285954 [details] [review] Remove obsolete mutter-test script mutter-test was a script that was used to run various test on the Metacity source tree (build with different options, etc.) This likely hasn't been run once since the Metacity/Mutter branch point; remove it to avoid confusion with the new test framework in src/tests.
Created attachment 285955 [details] [review] Add meta_wayland_get_[x]wayland_display_name Add private functions for the test framework to use to find out the wayland and x11 display names, so they can set up the environment for children.
Created attachment 285956 [details] [review] Add meta_display_set_alarm_filter() Add a private hook for the test framework to get XSyncAlarmEvent events - this will be used to implement XSyncCounter based synchronization so that the test framework can deterministically wait until Mutter has seen actions performed by an X11 client.
Created attachment 285957 [details] [review] Add meta_ui_window_is_dummy() For reasons related to interaction between the GTK+ CSS code and the frame sync protocol, the dummy GtWwindow that MetaUI creates to track theme properties has to be mapped and have MetaWindow associated with it. Add a private function so that the test framework can filter this out.
Created attachment 285958 [details] [review] Add a test framework and stacking tests Add a basic framework for tests of Mutter handling of client behavior; mutter-test-runner is a Mutter-based compositor that forks off instances of mutter-test-client and sends commands to them based on scripts. The scripts also include assertions. mutter-test-runner always runs in nested-Wayland mode since the separate copy of Xwayland is helpeful in giving a reliably clean X server to test against. Initially the commands and assertions are designed to test the stacking behavior of Mutter, but the framework should be extensible to test other parts of client behavior like focus. The tests are installed according to: https://wiki.gnome.org/Initiatives/GnomeGoals/InstalledTests if --enable-installed-tests is passed to configure. You can run them uninstalled with: cd src && make run-tests (Not in 'make check' to avoid breaking 'make distcheck' if Mutter can't be run nested.)
Review of attachment 285954 [details] [review]: OK.
Review of attachment 285957 [details] [review]: "GtWwindow" in the commit message.
Review of attachment 285955 [details] [review]: ::: src/wayland/meta-wayland.c @@ +452,3 @@ + + +const char * Extra whitespace.
Review of attachment 285958 [details] [review]: "helpeful" in the commit message. ::: src/tests/README @@ +42,3 @@ +new_client <client-id> [wayland|x11] + Starts a client, connecting by either Wayland or X11. The client + will subsequently be known with the given client-id (an arbitraryp "arbitraryp" @@ +59,3 @@ +activate <client-id>/<window-id> + Ask the client to raise and focus the given window. This is currently a no-op + for Wayland, where this capability is not supported in the protocol. It will always be a no-op if I have any say, too. @@ +68,3 @@ +lower <client-id>/<window-id> + Ask the client to raise or lower the given window ID. This is currently a no-op + for Wayland clients. (It's also considered discouraged, but supported, for Same -- it will always be a no-op on Wayland. ::: src/tests/test-client.c @@ +99,3 @@ + * confuses our testing if it happens asynchronously the first + * time a window is painted, by creating an Xlib surface and + * destroying it, we force initialization at a more predictable time. This is a run-on sentence. @@ +298,3 @@ + "client-id", 0, 0, G_OPTION_ARG_STRING, + &client_id, + "Mutter plugin to use", Well that's wrong. ::: src/tests/test-runner.c @@ +129,3 @@ +async_waiter_set_and_wait (AsyncWaiter *waiter) +{ + Display *xdisplay = meta_get_display ()->xdisplay; Indentation here is wrong. @@ +130,3 @@ +{ + Display *xdisplay = meta_get_display ()->xdisplay; + int wait_value = async_waiter_allocate (waiter); async_waiter_next_value sounds more natural to me. @@ +171,3 @@ + + AsyncWaiter *waiter; + Extra commit message. @@ +371,3 @@ + + GSList *windows = meta_display_list_windows (display, + META_LIST_INCLUDE_OVERRIDE_REDIRECT); Do we even read _NET_WM_NAME on O-R windows? @@ +614,3 @@ + if (strcmp (x11_string->str, local_string->str) != 0) + { + char *cmd = g_strdup_printf ("DISPLAY=:1 xwininfo -id %lu", children[n_children-1]); Hardcoded DISPLAY=:1 ? I also assume this is just to print some extra info before dying with an assert fail? @@ +730,3 @@ + return FALSE; + + meta_window_activate (window, 0); Isn't this going to cause a giant warning every time? @@ +826,3 @@ + int argc; + char **argv = NULL; + if (!g_shell_parse_argv (line, &argc, &argv, &error)) Clever :) @@ +1024,3 @@ + return 1; + } + extra whitespace @@ +1052,3 @@ + char *dirname = g_path_get_dirname (argv[0]); + if (g_str_has_prefix (basename, "lt-")) + test_client_path = g_build_filename (dirname, "../mutter-test-client", NULL); yuck. don't you want to run mutter-test-client under libtool too, to make sure that it's bound to the correct libraries?
Review of attachment 285955 [details] [review]: lg after fixes
Review of attachment 285956 [details] [review]: lg
Review of attachment 285957 [details] [review]: I'm unhappy with this, but I'm just unhappy with how the GTK+ decorations stuff is done to begin with, I guess. lg after fixes
(In reply to comment #9) > @@ +59,3 @@ > +activate <client-id>/<window-id> > + Ask the client to raise and focus the given window. This is currently a no-op > + for Wayland, where this capability is not supported in the protocol. > > It will always be a no-op if I have any say, too. To some extent I'm lying about it not being in the protocol - it's just called removing your window and then showing another identical one :-) The canonical case for activation/presentation is a menu item that activates another window of the application - say a "Help" menu item. If the window is not currently showing, it shows the window. If the window *is* currently showing, it should be brought to the top. > @@ +68,3 @@ > +lower <client-id>/<window-id> > + Ask the client to raise or lower the given window ID. This is currently a > no-op > + for Wayland clients. (It's also considered discouraged, but supported, for > > Same -- it will always be a no-op on Wayland. I removed "currently" for this one. Existing X apps definitely exercise this part of Mutter but hopefully app authors can figure out something better when rewriting to Wayland :-) > ::: src/tests/test-runner.c > @@ +129,3 @@ > +async_waiter_set_and_wait (AsyncWaiter *waiter) > +{ > + Display *xdisplay = meta_get_display ()->xdisplay; > > Indentation here is wrong. How? The space in 'meta_get_display ()->xdisplay' is ugly, but consistent with our general rules. And the same construct is used several times above this in the file. > @@ +371,3 @@ > + > + GSList *windows = meta_display_list_windows (display, > + how we do things elsewhere > META_LIST_INCLUDE_OVERRIDE_REDIRECT); > > Do we even read _NET_WM_NAME on O-R windows? Yes. (Keeps things simple here since I can just use the _NET_WM_NAME to distinguish client windows.) > @@ +614,3 @@ > + if (strcmp (x11_string->str, local_string->str) != 0) > + { > + char *cmd = g_strdup_printf ("DISPLAY=:1 xwininfo -id %lu", > children[n_children-1]); > > Hardcoded DISPLAY=:1 ? I also assume this is just to print some extra info > before dying with an assert fail? Whole thing is an accidental leftover for some code I put in to figure out why I was getting a window appearing accidentally. Removed. > @@ +730,3 @@ > + return FALSE; > + > + meta_window_activate (window, 0); > > Isn't this going to cause a giant warning every time? meta_window_activate() has: if (timestamp == 0)how we do things elsewhere timestamp = meta_display_get_current_time_roundtrip (window->display); the warning you are thinking of is directly in the handling of the _NET_ACTIVE_WINDOW message. > @@ +1052,3 @@how we do things elsewhere > + char *dirname = g_path_get_dirname (argv[0]); > + if (g_str_has_prefix (basename, "lt-")) > + test_client_path = g_build_filename (dirname, "../mutter-test-client", > NULL); > > yuck. don't you want to run mutter-test-client under libtool too, to make sure > that it's bound to the correct libraries? Yes it's a bit yuck, but it's the simplest way to make it work installed and uninstalled. The above code *does* run mutter-test-client under libtool since it backs out of the .libs directory and runs the mutter-test-client wrapper. Fixed everything else up - the patches should be ready to push unless you want me to rewrite the 'meta_get_display ()->xdisplay' usages.
It wasn't anything about meta_get_display. It was that function had four spaces instead of two. Four space indents should never appear anywhere in GNU code.
Pushed, fixing the 4-space indent that I couldn't see ;-) Attachment 285954 [details] pushed as 9dd9938 - Remove obsolete mutter-test script Attachment 285955 [details] pushed as 0706de5 - Add meta_wayland_get_[x]wayland_display_name Attachment 285956 [details] pushed as 44ecb1c - Add meta_display_set_alarm_filter() Attachment 285957 [details] pushed as 95d9a95 - Add meta_ui_window_is_dummy() Attachment 285958 [details] pushed as 2f63c39 - Add a test framework and stacking tests