[PATCH weston v2 5/5] tests: add tests for devices handling

[Marek Chalupa]

On Mon, Mar 30, 2015 at 8:22 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:

> On Mon, 30 Mar 2015 06:37:59 -0400
> Marek Chalupa <mchqwerty at gmail.com> wrote:
>
> > Test misc races when adding/releasing devices
> >
> > v2.: use one roundtrip after releasing devices
> >      add touch support
> >
> > Signed-off-by: Marek Chalupa <mchqwerty at gmail.com>
> > ---
> >  Makefile.am          |   7 +-
> >  tests/devices-test.c | 299
> +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 305 insertions(+), 1 deletion(-)
> >  create mode 100644 tests/devices-test.c
>
> > diff --git a/tests/devices-test.c b/tests/devices-test.c
> > new file mode 100644
> > index 0000000..cf41471
> > --- /dev/null
> > +++ b/tests/devices-test.c
>
> > +TEST(device_release_before_destroy)
> > +{
> > +     struct client *cl = client_create(100, 100, 100, 100);
> > +
> > +     /* we can release pointer when we won't be using it anymore.
> > +      * Do it and see what happens if the device is destroyed
> > +      * right after that */
> > +     wl_pointer_release(cl->input->pointer->wl_pointer);
> > +     /* we must free and set to NULL the structures, otherwise
> > +      * seat capabilities will double-free them */
> > +     free(cl->input->pointer);
> > +     cl->input->pointer = NULL;
> > +
> > +     wl_keyboard_release(cl->input->keyboard->wl_keyboard);
> > +     free(cl->input->keyboard);
> > +     cl->input->keyboard = NULL;
> > +
> > +     wl_touch_release(cl->input->touch->wl_touch);
> > +     free(cl->input->touch);
> > +     cl->input->touch = NULL;
> > +
> > +     weston_test_device_release(cl->test->weston_test, "pointer");
> > +     weston_test_device_release(cl->test->weston_test, "keyboard");
> > +     weston_test_device_release(cl->test->weston_test, "touch");
> > +     client_roundtrip(cl);
> > +
> > +     assert(cl->input->caps == 0);
> > +     assert(wl_display_get_error(cl->wl_display) == 0);
>
> Is this display error check needed? Doesn't client_roundtrip()
> guarantee success?
>

Nope, it is not needed, really.


> > +
> > +     /* restore previous state */
> > +     weston_test_device_add(cl->test->weston_test, "pointer");
> > +     weston_test_device_add(cl->test->weston_test, "keyboard");
> > +     weston_test_device_add(cl->test->weston_test, "touch");
> > +     client_roundtrip(cl);
> > +
> > +     assert(cl->input->caps == WL_SEAT_CAPABILITY_ALL);
> > +}
> > +
> > +TEST(device_release_before_destroy_multiple)
> > +{
> > +     int i;
> > +
> > +     /* if weston crashed during this test, then there is
> > +      * some inconsistency */
> > +     for (i = 0; i < 100; ++i) {
> > +             device_release_before_destroy();
>
> Is it intentional to create a hundred clients without destroying the
> old ones? Or is that exactly the point?
>
>
It is intentional to run it so many times, but it is not intentional to run
a hundred clients at a time.
The problem is that currently we have no destroy function for client.
However, the clients do not
run simultaneously but serially, so the effect should be the same as if
we'd destroy them
(after the client finishes its body, it just 'is' and does nothing until
the process exits)


> Maybe it's a good test just like this but needs a comment, otherwise
> someone might "fix" it.
>
> > +     }
> > +}
> > +
> > +/* normal work-flow test */
> > +TEST(device_release_after_destroy)
> > +{
> > +     struct client *cl = client_create(100, 100, 100, 100);
> > +
> > +     weston_test_device_release(cl->test->weston_test, "pointer");
> > +     wl_pointer_release(cl->input->pointer->wl_pointer);
> > +     /* we must free the memory manually, otherwise seat.capabilities
> > +      * will try to free it and will use invalid proxy */
> > +     free(cl->input->pointer);
> > +     cl->input->pointer = NULL;
> > +
> > +     client_roundtrip(cl);
> > +
> > +     weston_test_device_release(cl->test->weston_test, "keyboard");
> > +     wl_keyboard_release(cl->input->keyboard->wl_keyboard);
> > +     free(cl->input->keyboard);
> > +     cl->input->keyboard = NULL;
> > +
> > +     client_roundtrip(cl);
> > +
> > +     weston_test_device_release(cl->test->weston_test, "touch");
> > +     wl_touch_release(cl->input->touch->wl_touch);
> > +     free(cl->input->touch);
> > +     cl->input->touch = NULL;
> > +
> > +     client_roundtrip(cl);
> > +
> > +     assert(cl->input->caps == 0);
> > +     assert(wl_display_get_error(cl->wl_display) == 0);
>
> Is the display error check needed?
>
> > +
> > +     /* restore previous state */
> > +     weston_test_device_add(cl->test->weston_test, "pointer");
> > +     weston_test_device_add(cl->test->weston_test, "keyboard");
> > +     weston_test_device_add(cl->test->weston_test, "touch");
> > +     client_roundtrip(cl);
> > +
> > +     assert(cl->input->caps == WL_SEAT_CAPABILITY_ALL);
> > +}
> > +
> > +TEST(device_release_after_destroy_multiple)
> > +{
> > +     int i;
> > +
> > +     /* if weston crashed during this test, then there is
> > +      * some inconsistency */
> > +     for (i = 0; i < 100; ++i) {
> > +             device_release_after_destroy();
>
> Again hundred simultaneous clients?
>
> > +     }
> > +}
> > +
> > +/* see https://bugzilla.gnome.org/show_bug.cgi?id=745008
> > + * It is a mutter bug, but highly relevant. Weston does not
> > + * suffer from this bug atm, but it is worth of testing. */
> > +TEST(get_device_after_destroy)
> > +{
> > +     struct client *cl = client_create(100, 100, 100, 100);
> > +     struct wl_pointer *wl_pointer;
> > +     struct wl_keyboard *wl_keyboard;
> > +     struct wl_touch *wl_touch;
> > +
> > +     /* There's a race:
> > +      *  1) compositor destroyes device
> > +      *  2) client asks for the device, because it has not got
> > +      *     new capabilities yet
> > +      *  3) compositor gets request with new_id for destroyed device
> > +      *  4) client uses the new_id
> > +      *  5) client gets new capabilities, destroying the objects
> > +      *
> > +      * If compositor just bail out in step 3) and does not create
> > +      * resource, then client gets error in step 4) - even though
> > +      * it followed the protocol (it just didn't know about new
> > +      * capabilities).
> > +      *
> > +      * This test simulates this situation
> > +      */
> > +
> > +     /* connection is buffered, so after calling client_roundtrip(),
> > +      * this whole batch will be delivered to compositor and will
> > +      * exactly simulate our situation */
> > +     weston_test_device_release(cl->test->weston_test, "pointer");
> > +     wl_pointer = wl_seat_get_pointer(cl->input->wl_seat);
> > +     assert(wl_pointer);
> > +
> > +     /* this should be ignored */
> > +     wl_pointer_set_cursor(wl_pointer, 0, NULL, 0, 0);
> > +
> > +     /* this should not be ignored */
> > +     wl_pointer_release(wl_pointer);
> > +     client_roundtrip(cl);
> > +
> > +     weston_test_device_release(cl->test->weston_test, "keyboard");
> > +     wl_keyboard = wl_seat_get_keyboard(cl->input->wl_seat);
> > +     assert(wl_keyboard);
> > +     wl_keyboard_release(wl_keyboard);
> > +     client_roundtrip(cl);
> > +
> > +     weston_test_device_release(cl->test->weston_test, "touch");
> > +     wl_touch = wl_seat_get_touch(cl->input->wl_seat);
> > +     assert(wl_touch);
> > +     wl_touch_release(wl_touch);
> > +     client_roundtrip(cl);
> > +
> > +     assert(wl_display_get_error(cl->wl_display) == 0);
>
> And this check.
>
> > +
> > +     /* get weston to the previous state, so that other tests
> > +      * have the same environment */
>
> This is such an important note, that it could stand on its own. Like in
> the beginning of this whole file as a major doc block.
>
> > +     weston_test_device_add(cl->test->weston_test, "pointer");
> > +     weston_test_device_add(cl->test->weston_test, "keyboard");
> > +     weston_test_device_add(cl->test->weston_test, "touch");
> > +     client_roundtrip(cl);
> > +
> > +     assert(cl->input->caps == WL_SEAT_CAPABILITY_ALL);
> > +}
> > +
> > +TEST(get_device_afer_destroy_multiple)
> > +{
> > +     int i;
> > +
> > +     /* if weston crashed during this test, then there is
> > +      * some inconsistency */
> > +     for (i = 0; i < 100; ++i) {
> > +             get_device_after_destroy();
>
> Again a hundred clients.
>
> > +     }
> > +}
>
> Patches 1, 2 and 4 are R-b me. Patch 3 I commented on IRC, I didn't
> like the hardcoded call to seat_handle_capabilities().
>
>
> Thanks,
> pq
>

Fixed the code according the notes, thanks for the review!

Marek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20150330/8cf433df/attachment.html>