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

[Pekka Paalanen]

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?

> +
> +	/* 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?

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