[PATCH weston] ivi-shell: Added tests for screen-remove-layer API

[Teyfel, Michael (ADITG/ESB)]

Hi Pekka,

thank you for reviewing and also your remarks. I adjusted my patch accordingly and resent it to ML.


Best regards

Michael Teyfel
Engineering Software Base (ADITG/ESB)

Tel. +49 5121 49 6932

-----Original Message-----
From: Pekka Paalanen [mailto:ppaalanen at gmail.com] 
Sent: Mittwoch, 26. Juli 2017 11:18
To: Teyfel, Michael (ADITG/ESB)
Cc: wayland-devel at lists.freedesktop.org; Ucan, Emre (ADITG/ESB); Friedrich, Eugen (ADITG/ESB)
Subject: Re: [PATCH weston] ivi-shell: Added tests for screen-remove-layer API

On Tue, 25 Jul 2017 15:59:06 +0200
Michael Teyfel <mteyfel at de.adit-jv.com> wrote:

> Two cases are tested: success and fail case of the screen-remove-layer API.
> 
> Signed-off-by: Michael Teyfel <mteyfel at de.adit-jv.com>
> ---
>  tests/ivi_layout-internal-test.c | 62 
> ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)

Hi Michael,

thanks for these, they look mostly good, just some polishing required.
Comments inline.

> 
> diff --git a/tests/ivi_layout-internal-test.c 
> b/tests/ivi_layout-internal-test.c
> index 37a2356..64380ec 100644
> --- a/tests/ivi_layout-internal-test.c
> +++ b/tests/ivi_layout-internal-test.c
> @@ -341,6 +341,66 @@ test_layer_source_rectangle(struct test_context 
> *ctx)  }
>  
>  static void
> +test_screen_remove_layer(struct test_context *ctx) {
> +	const struct ivi_layout_interface *lyt = ctx->layout_interface;
> +	struct ivi_layout_layer *ivilayer;
> +	struct weston_output *output;
> +	struct ivi_layout_layer **array;
> +	int32_t length = 0;
> +
> +	ivilayer = lyt->layer_create_with_dimension(IVI_TEST_LAYER_ID(0), 200, 300);
> +	iassert(ivilayer != NULL);
> +
> +	if (wl_list_empty(&ctx->compositor->output_list))
> +		return;

This leaks the ivilayer, possibly affecting also further tests. Move the check before the layer creation.

> +
> +	output = wl_container_of(ctx->compositor->output_list.next, output, 
> +link);
> +
> +	iassert(lyt->screen_add_layer(output, ivilayer) == IVI_SUCCEEDED);
> +	lyt->commit_changes();
> +
> +	iassert(lyt->get_layers_on_screen(output, &length, &array) == IVI_SUCCEEDED);
> +	iassert(length == 1);
> +	iassert(array[0] == ivilayer);
> +
> +	iassert(lyt->screen_remove_layer(output ,ivilayer) == 
> +IVI_SUCCEEDED);

Space on the wrong side of comma.

> +	lyt->commit_changes();
> +

'array' gets leaked.

> +	iassert(lyt->get_layers_on_screen(output, &length, &array) == IVI_SUCCEEDED);
> +	iassert(length == 0);

'array' would get leaked if there were any layers, but since we fail the test in that case, it doesn't matter.

> +
> +	lyt->layer_destroy(ivilayer);
> +}
> +
> +static void
> +test_screen_bad_remove_layer(struct test_context *ctx) {
> +	const struct ivi_layout_interface *lyt = ctx->layout_interface;
> +	struct ivi_layout_layer *ivilayer;
> +	struct weston_output *output;
> +
> +	ivilayer = lyt->layer_create_with_dimension(IVI_TEST_LAYER_ID(0), 200, 300);
> +	iassert(ivilayer != NULL);
> +
> +	if (wl_list_empty(&ctx->compositor->output_list))
> +		return;

The same leak of ivilayer as before.

> +
> +	output = wl_container_of(ctx->compositor->output_list.next, output, 
> +link);
> +
> +	iassert(lyt->screen_remove_layer(NULL, ivilayer) == IVI_FAILED);
> +	lyt->commit_changes();
> +
> +	iassert(lyt->screen_remove_layer(output, NULL) == IVI_FAILED);
> +	lyt->commit_changes();
> +
> +	iassert(lyt->screen_remove_layer(NULL, NULL) == IVI_FAILED);
> +	lyt->commit_changes();
> +
> +	lyt->layer_destroy(ivilayer);
> +}
> +
> +static void
>  test_layer_bad_remove(struct test_context *ctx)  {
>  	const struct ivi_layout_interface *lyt = ctx->layout_interface; @@ 
> -951,6 +1011,8 @@ run_internal_tests(void *data)
>  	test_layer_position(ctx);
>  	test_layer_destination_rectangle(ctx);
>  	test_layer_source_rectangle(ctx);
> +	test_screen_remove_layer(ctx);
> +	test_screen_bad_remove_layer(ctx);

These two would be better grouped with the other screen tests, I think.

>  	test_layer_bad_remove(ctx);
>  	test_layer_bad_visibility(ctx);
>  	test_layer_bad_opacity(ctx);


Thanks,
pq