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

[Pekka Paalanen]

On Mon, 30 Mar 2015 09:13:16 -0400
Marek Chalupa <mchqwerty at gmail.com> wrote:

> 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)

Well, sure, but it still keeps some server state alive, particularly
all protocol objects. The server will try to send events to these
objects. The client is not reading, so at some point the message
buffers might fill up, and the client get kicked. But I'm not sure we
catch the kick-error, because nothing of the client code is ever run
again. But if you try to debug things with WAYLAND_DEBUG=server, you
might get confused, plus also see lots of useless messages.

Anyway, like you said, we don't have a client_destroy() now, so if you
want to fix that, it's another patch.

> > Maybe it's a good test just like this but needs a comment, otherwise
> > someone might "fix" it.
> >
> > > +     }
> > > +}


Thanks,
pq