[01/14] tests: test set for ivi-surface normal use case with helper client

Submitted by Nobuhiko Tanibata on June 22, 2015, 6:33 a.m.

Details

Message ID 1434954797-27125-1-git-send-email-nobuhiko_tanibata@xddp.denso.co.jp
State Accepted
Commit 23d95824a508b0724dde77587147706a7f724a79
Headers show

Not browsing as part of any series.

Commit Message

Nobuhiko Tanibata June 22, 2015, 6:33 a.m.
From: Nobuhiko Tanibata <NOBUHIKO_TANIBATA@xddp.denso.co.jp>

These tests are implemented on test suite framework, which provides
helper client.
Following features are tested for ivi-surface
- orientation
- dimension
- position
- destination rectangle
- source rectangle

Signed-off-by: Nobuhiko Tanibata <NOBUHIKO_TANIBATA@xddp.denso.co.jp>
Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
---
 tests/ivi_layout-test-plugin.c | 219 ++++++++++++++++++++++++++++++++++++++---
 tests/ivi_layout-test.c        |   5 +
 2 files changed, 211 insertions(+), 13 deletions(-)

Patch hide | download patch | download mbox

diff --git a/tests/ivi_layout-test-plugin.c b/tests/ivi_layout-test-plugin.c
index b4abfbf..1d0ce02 100644
--- a/tests/ivi_layout-test-plugin.c
+++ b/tests/ivi_layout-test-plugin.c
@@ -303,16 +303,16 @@  RUNNER_TEST(surface_create_p1)
 	uint32_t ivi_id;
 
 	ivisurf[0] = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
-	runner_assert_or_return(ivisurf[0]);
+	runner_assert(ivisurf[0]);
 
 	ivisurf[1] = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(1));
-	runner_assert_or_return(ivisurf[1]);
+	runner_assert(ivisurf[1]);
 
 	ivi_id = ctl->get_id_of_surface(ivisurf[0]);
-	runner_assert_or_return(ivi_id == IVI_TEST_SURFACE_ID(0));
+	runner_assert(ivi_id == IVI_TEST_SURFACE_ID(0));
 
 	ivi_id = ctl->get_id_of_surface(ivisurf[1]);
-	runner_assert_or_return(ivi_id == IVI_TEST_SURFACE_ID(1));
+	runner_assert(ivi_id == IVI_TEST_SURFACE_ID(1));
 }
 
 RUNNER_TEST(surface_create_p2)
@@ -322,7 +322,7 @@  RUNNER_TEST(surface_create_p2)
 
 	/* the ivi_surface was destroyed by the client */
 	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
-	runner_assert_or_return(ivisurf == NULL);
+	runner_assert(ivisurf == NULL);
 }
 
 RUNNER_TEST(surface_visibility)
@@ -334,18 +334,18 @@  RUNNER_TEST(surface_visibility)
 	const struct ivi_layout_surface_properties *prop;
 
 	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
-	runner_assert_or_return(ivisurf);
+	runner_assert(ivisurf);
 
 	ret = ctl->surface_set_visibility(ivisurf, true);
-	runner_assert_or_return(ret == IVI_SUCCEEDED);
+	runner_assert(ret == IVI_SUCCEEDED);
 
 	ctl->commit_changes();
 
 	visibility = ctl->surface_get_visibility(ivisurf);
-	runner_assert_or_return(visibility == true);
+	runner_assert(visibility == true);
 
 	prop = ctl->get_properties_of_surface(ivisurf);
-	runner_assert_or_return(prop->visibility == true);
+	runner_assert(prop->visibility == true);
 }
 
 RUNNER_TEST(surface_opacity)
@@ -357,16 +357,209 @@  RUNNER_TEST(surface_opacity)
 	const struct ivi_layout_surface_properties *prop;
 
 	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
-	runner_assert_or_return(ivisurf);
+	runner_assert(ivisurf);
+
+	runner_assert(ctl->surface_get_opacity(ivisurf) ==
+		      wl_fixed_from_double(1.0));
 
 	ret = ctl->surface_set_opacity(ivisurf, wl_fixed_from_double(0.5));
-	runner_assert_or_return(ret == IVI_SUCCEEDED);
+	runner_assert(ret == IVI_SUCCEEDED);
+
+	runner_assert(ctl->surface_get_opacity(ivisurf) ==
+		      wl_fixed_from_double(1.0));
 
 	ctl->commit_changes();
 
 	opacity = ctl->surface_get_opacity(ivisurf);
-	runner_assert_or_return(opacity == wl_fixed_from_double(0.5));
+	runner_assert(opacity == wl_fixed_from_double(0.5));
+
+	prop = ctl->get_properties_of_surface(ivisurf);
+	runner_assert(prop->opacity == wl_fixed_from_double(0.5));
+}
+
+RUNNER_TEST(surface_orientation)
+{
+	const struct ivi_controller_interface *ctl = ctx->controller_interface;
+	struct ivi_layout_surface *ivisurf;
+	const struct ivi_layout_surface_properties *prop;
+
+	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
+	runner_assert(ivisurf != NULL);
+
+	runner_assert(ctl->surface_get_orientation(ivisurf) ==
+		      WL_OUTPUT_TRANSFORM_NORMAL);
+
+	runner_assert(ctl->surface_set_orientation(
+		      ivisurf, WL_OUTPUT_TRANSFORM_90) == IVI_SUCCEEDED);
+
+	runner_assert(ctl->surface_get_orientation(ivisurf) ==
+		      WL_OUTPUT_TRANSFORM_NORMAL);
+
+	ctl->commit_changes();
+
+	runner_assert(ctl->surface_get_orientation(
+		      ivisurf) == WL_OUTPUT_TRANSFORM_90);
+
+	prop = ctl->get_properties_of_surface(ivisurf);
+	runner_assert_or_return(prop);
+	runner_assert(prop->orientation == WL_OUTPUT_TRANSFORM_90);
+}
+
+RUNNER_TEST(surface_dimension)
+{
+	const struct ivi_controller_interface *ctl = ctx->controller_interface;
+	struct ivi_layout_surface *ivisurf;
+	const struct ivi_layout_surface_properties *prop;
+	int32_t dest_width;
+	int32_t dest_height;
+
+	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
+	runner_assert(ivisurf != NULL);
+
+	runner_assert(ctl->surface_get_dimension(
+		      ivisurf, &dest_width, &dest_height) == IVI_SUCCEEDED);
+	runner_assert(dest_width == 1);
+	runner_assert(dest_height == 1);
+
+	runner_assert(IVI_SUCCEEDED ==
+		      ctl->surface_set_dimension(ivisurf, 200, 300));
+
+	runner_assert(ctl->surface_get_dimension(
+		      ivisurf, &dest_width, &dest_height) == IVI_SUCCEEDED);
+	runner_assert(dest_width == 1);
+	runner_assert(dest_height == 1);
+
+	ctl->commit_changes();
+
+	runner_assert(ctl->surface_get_dimension(
+		      ivisurf, &dest_width, &dest_height) == IVI_SUCCEEDED);
+	runner_assert(dest_width == 200);
+	runner_assert(dest_height == 300);
+
+	prop = ctl->get_properties_of_surface(ivisurf);
+	runner_assert_or_return(prop);
+	runner_assert(prop->dest_width == 200);
+	runner_assert(prop->dest_height == 300);
+}
+
+RUNNER_TEST(surface_position)
+{
+	const struct ivi_controller_interface *ctl = ctx->controller_interface;
+	struct ivi_layout_surface *ivisurf;
+	const struct ivi_layout_surface_properties *prop;
+	int32_t dest_x;
+	int32_t dest_y;
+
+	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
+	runner_assert(ivisurf != NULL);
+
+	runner_assert(ctl->surface_get_position(
+		      ivisurf, &dest_x, &dest_y) == IVI_SUCCEEDED);
+	runner_assert(dest_x == 0);
+	runner_assert(dest_y == 0);
+
+	runner_assert(ctl->surface_set_position(
+		      ivisurf, 20, 30) == IVI_SUCCEEDED);
+
+	runner_assert(ctl->surface_get_position(
+		      ivisurf, &dest_x, &dest_y) == IVI_SUCCEEDED);
+	runner_assert(dest_x == 0);
+	runner_assert(dest_y == 0);
+
+	ctl->commit_changes();
+
+	runner_assert(ctl->surface_get_position(
+		      ivisurf, &dest_x, &dest_y) == IVI_SUCCEEDED);
+	runner_assert(dest_x == 20);
+	runner_assert(dest_y == 30);
+
+	prop = ctl->get_properties_of_surface(ivisurf);
+	runner_assert_or_return(prop);
+	runner_assert(prop->dest_x == 20);
+	runner_assert(prop->dest_y == 30);
+}
+
+RUNNER_TEST(surface_destination_rectangle)
+{
+	const struct ivi_controller_interface *ctl = ctx->controller_interface;
+	struct ivi_layout_surface *ivisurf;
+	const struct ivi_layout_surface_properties *prop;
+	int32_t dest_width;
+	int32_t dest_height;
+	int32_t dest_x;
+	int32_t dest_y;
+
+	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
+	runner_assert(ivisurf != NULL);
+
+	prop = ctl->get_properties_of_surface(ivisurf);
+	runner_assert_or_return(prop);
+	runner_assert(prop->dest_width == 1);
+	runner_assert(prop->dest_height == 1);
+	runner_assert(prop->dest_x == 0);
+	runner_assert(prop->dest_y == 0);
+
+	runner_assert(ctl->surface_set_destination_rectangle(
+		      ivisurf, 20, 30, 200, 300) == IVI_SUCCEEDED);
+
+	prop = ctl->get_properties_of_surface(ivisurf);
+	runner_assert_or_return(prop);
+	runner_assert(prop->dest_width == 1);
+	runner_assert(prop->dest_height == 1);
+	runner_assert(prop->dest_x == 0);
+	runner_assert(prop->dest_y == 0);
+
+	ctl->commit_changes();
+
+	runner_assert(ctl->surface_get_dimension(
+		      ivisurf, &dest_width, &dest_height) == IVI_SUCCEEDED);
+	runner_assert(dest_width == 200);
+	runner_assert(dest_height == 300);
+
+	runner_assert(ctl->surface_get_position(ivisurf, &dest_x, &dest_y) == IVI_SUCCEEDED);
+	runner_assert(dest_x == 20);
+	runner_assert(dest_y == 30);
+
+	prop = ctl->get_properties_of_surface(ivisurf);
+	runner_assert_or_return(prop);
+	runner_assert(prop->dest_width == 200);
+	runner_assert(prop->dest_height == 300);
+	runner_assert(prop->dest_x == 20);
+	runner_assert(prop->dest_y == 30);
+}
+
+RUNNER_TEST(surface_source_rectangle)
+{
+	const struct ivi_controller_interface *ctl = ctx->controller_interface;
+	struct ivi_layout_surface *ivisurf;
+	const struct ivi_layout_surface_properties *prop;
+
+	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
+	runner_assert(ivisurf != NULL);
+
+	prop = ctl->get_properties_of_surface(ivisurf);
+	runner_assert_or_return(prop);
+	runner_assert(prop->source_width == 0);
+	runner_assert(prop->source_height == 0);
+	runner_assert(prop->source_x == 0);
+	runner_assert(prop->source_y == 0);
+
+	runner_assert(ctl->surface_set_source_rectangle(
+		      ivisurf, 20, 30, 200, 300) == IVI_SUCCEEDED);
+
+	prop = ctl->get_properties_of_surface(ivisurf);
+	runner_assert_or_return(prop);
+	runner_assert(prop->source_width == 0);
+	runner_assert(prop->source_height == 0);
+	runner_assert(prop->source_x == 0);
+	runner_assert(prop->source_y == 0);
+
+	ctl->commit_changes();
 
 	prop = ctl->get_properties_of_surface(ivisurf);
-	runner_assert_or_return(prop->opacity == wl_fixed_from_double(0.5));
+	runner_assert_or_return(prop);
+	runner_assert(prop->source_width == 200);
+	runner_assert(prop->source_height == 300);
+	runner_assert(prop->source_x == 20);
+	runner_assert(prop->source_y == 30);
 }
diff --git a/tests/ivi_layout-test.c b/tests/ivi_layout-test.c
index d6401c4..ba0387b 100644
--- a/tests/ivi_layout-test.c
+++ b/tests/ivi_layout-test.c
@@ -190,6 +190,11 @@  ivi_window_destroy(struct ivi_window *wnd)
 const char * const basic_test_names[] = {
 	"surface_visibility",
 	"surface_opacity",
+	"surface_orientation",
+	"surface_dimension",
+	"surface_position",
+	"surface_destination_rectangle",
+	"surface_source_rectangle",
 };
 
 TEST_P(ivi_layout_runner, basic_test_names)

Comments

On 06/21/2015 11:33 PM, Nobuhiko Tanibata wrote:
> From: Nobuhiko Tanibata <NOBUHIKO_TANIBATA@xddp.denso.co.jp>
> 
> These tests are implemented on test suite framework, which provides
> helper client.
> Following features are tested for ivi-surface
> - orientation
> - dimension
> - position
> - destination rectangle
> - source rectangle
> 
> Signed-off-by: Nobuhiko Tanibata <NOBUHIKO_TANIBATA@xddp.denso.co.jp>
> Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>

These generally look good, but there are a few 'gotcha' places were
conversion from runner_assert_or_return() calls to runner_assert() ones.

Several places check to see if ivisurf is valid. If the value is not
good then none of the subsequent testing should be attempted, thus the
code should keep things to runner_assert_or_return();

In most of the cases the code probably still warrant the *_or_return()
calls. Aside from attempting to test an invalid surface (if
runner_assert(ivisurf) fails), much of the testing in not valid if it
continues.

For example, after an attempt to set a property fails, then any further
testing of the state would be invalid.

Removing the _or_return() part should only be in those places where
failures in the middle of a test function don't actually affect
subsequent code in that test.

> ---
>  tests/ivi_layout-test-plugin.c | 219 ++++++++++++++++++++++++++++++++++++++---
>  tests/ivi_layout-test.c        |   5 +
>  2 files changed, 211 insertions(+), 13 deletions(-)
> 
> diff --git a/tests/ivi_layout-test-plugin.c b/tests/ivi_layout-test-plugin.c
> index b4abfbf..1d0ce02 100644
> --- a/tests/ivi_layout-test-plugin.c
> +++ b/tests/ivi_layout-test-plugin.c
> @@ -303,16 +303,16 @@ RUNNER_TEST(surface_create_p1)
>  	uint32_t ivi_id;
>  
>  	ivisurf[0] = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
> -	runner_assert_or_return(ivisurf[0]);
> +	runner_assert(ivisurf[0]);
>  
>  	ivisurf[1] = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(1));
> -	runner_assert_or_return(ivisurf[1]);
> +	runner_assert(ivisurf[1]);
>  
>  	ivi_id = ctl->get_id_of_surface(ivisurf[0]);
> -	runner_assert_or_return(ivi_id == IVI_TEST_SURFACE_ID(0));
> +	runner_assert(ivi_id == IVI_TEST_SURFACE_ID(0));
>  
>  	ivi_id = ctl->get_id_of_surface(ivisurf[1]);
> -	runner_assert_or_return(ivi_id == IVI_TEST_SURFACE_ID(1));
> +	runner_assert(ivi_id == IVI_TEST_SURFACE_ID(1));
>  }
>  RUNNER_TEST(surface_create_p2)
> @@ -322,7 +322,7 @@ RUNNER_TEST(surface_create_p2)
>  
>  	/* the ivi_surface was destroyed by the client */
>  	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
> -	runner_assert_or_return(ivisurf == NULL);
> +	runner_assert(ivisurf == NULL);
>  }
>  
>  RUNNER_TEST(surface_visibility)
> @@ -334,18 +334,18 @@ RUNNER_TEST(surface_visibility)
>  	const struct ivi_layout_surface_properties *prop;
>  
>  	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
> -	runner_assert_or_return(ivisurf);
> +	runner_assert(ivisurf);
>  
>  	ret = ctl->surface_set_visibility(ivisurf, true);
> -	runner_assert_or_return(ret == IVI_SUCCEEDED);
> +	runner_assert(ret == IVI_SUCCEEDED);
>  
>  	ctl->commit_changes();
>  
>  	visibility = ctl->surface_get_visibility(ivisurf);
> -	runner_assert_or_return(visibility == true);
> +	runner_assert(visibility == true);
>  
>  	prop = ctl->get_properties_of_surface(ivisurf);
> -	runner_assert_or_return(prop->visibility == true);
> +	runner_assert(prop->visibility == true);
>  }
>  
>  RUNNER_TEST(surface_opacity)
> @@ -357,16 +357,209 @@ RUNNER_TEST(surface_opacity)
>  	const struct ivi_layout_surface_properties *prop;
>  
>  	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
> -	runner_assert_or_return(ivisurf);
> +	runner_assert(ivisurf);
> +
> +	runner_assert(ctl->surface_get_opacity(ivisurf) ==
> +		      wl_fixed_from_double(1.0));
>  
>  	ret = ctl->surface_set_opacity(ivisurf, wl_fixed_from_double(0.5));
> -	runner_assert_or_return(ret == IVI_SUCCEEDED);
> +	runner_assert(ret == IVI_SUCCEEDED);
> +
> +	runner_assert(ctl->surface_get_opacity(ivisurf) ==
> +		      wl_fixed_from_double(1.0));
>  
>  	ctl->commit_changes();
>  
>  	opacity = ctl->surface_get_opacity(ivisurf);
> -	runner_assert_or_return(opacity == wl_fixed_from_double(0.5));
> +	runner_assert(opacity == wl_fixed_from_double(0.5));
> +
> +	prop = ctl->get_properties_of_surface(ivisurf);
> +	runner_assert(prop->opacity == wl_fixed_from_double(0.5));
> +}
> +
> +RUNNER_TEST(surface_orientation)
> +{
> +	const struct ivi_controller_interface *ctl = ctx->controller_interface;
> +	struct ivi_layout_surface *ivisurf;
> +	const struct ivi_layout_surface_properties *prop;
> +
> +	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
> +	runner_assert(ivisurf != NULL);

The use of an explicit "!= NULL" is not consistent with other places in
the code that merely checks "(ivisurf)".

> +
> +	runner_assert(ctl->surface_get_orientation(ivisurf) ==
> +		      WL_OUTPUT_TRANSFORM_NORMAL);
> +
> +	runner_assert(ctl->surface_set_orientation(
> +		      ivisurf, WL_OUTPUT_TRANSFORM_90) == IVI_SUCCEEDED);
> +
> +	runner_assert(ctl->surface_get_orientation(ivisurf) ==
> +		      WL_OUTPUT_TRANSFORM_NORMAL);
> +
> +	ctl->commit_changes();
> +
> +	runner_assert(ctl->surface_get_orientation(
> +		      ivisurf) == WL_OUTPUT_TRANSFORM_90);
> +
> +	prop = ctl->get_properties_of_surface(ivisurf);
> +	runner_assert_or_return(prop);
> +	runner_assert(prop->orientation == WL_OUTPUT_TRANSFORM_90);
> +}
> +
> +RUNNER_TEST(surface_dimension)
> +{
> +	const struct ivi_controller_interface *ctl = ctx->controller_interface;
> +	struct ivi_layout_surface *ivisurf;
> +	const struct ivi_layout_surface_properties *prop;
> +	int32_t dest_width;
> +	int32_t dest_height;
> +
> +	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
> +	runner_assert(ivisurf != NULL);

The use of an explicit "!= NULL" is not consistent with other places in
the code that merely checks "(ivisurf)".

> +
> +	runner_assert(ctl->surface_get_dimension(
> +		      ivisurf, &dest_width, &dest_height) == IVI_SUCCEEDED);
> +	runner_assert(dest_width == 1);
> +	runner_assert(dest_height == 1);
> +
> +	runner_assert(IVI_SUCCEEDED ==
> +		      ctl->surface_set_dimension(ivisurf, 200, 300));
> +
> +	runner_assert(ctl->surface_get_dimension(
> +		      ivisurf, &dest_width, &dest_height) == IVI_SUCCEEDED);
> +	runner_assert(dest_width == 1);
> +	runner_assert(dest_height == 1);
> +
> +	ctl->commit_changes();
> +
> +	runner_assert(ctl->surface_get_dimension(
> +		      ivisurf, &dest_width, &dest_height) == IVI_SUCCEEDED);
> +	runner_assert(dest_width == 200);
> +	runner_assert(dest_height == 300);
> +
> +	prop = ctl->get_properties_of_surface(ivisurf);
> +	runner_assert_or_return(prop);
> +	runner_assert(prop->dest_width == 200);
> +	runner_assert(prop->dest_height == 300);
> +}
> +
> +RUNNER_TEST(surface_position)
> +{
> +	const struct ivi_controller_interface *ctl = ctx->controller_interface;
> +	struct ivi_layout_surface *ivisurf;
> +	const struct ivi_layout_surface_properties *prop;
> +	int32_t dest_x;
> +	int32_t dest_y;
> +
> +	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
> +	runner_assert(ivisurf != NULL);

The use of an explicit "!= NULL" is not consistent with other places in
the code that merely checks "(ivisurf)".

> +
> +	runner_assert(ctl->surface_get_position(
> +		      ivisurf, &dest_x, &dest_y) == IVI_SUCCEEDED);
> +	runner_assert(dest_x == 0);
> +	runner_assert(dest_y == 0);
> +
> +	runner_assert(ctl->surface_set_position(
> +		      ivisurf, 20, 30) == IVI_SUCCEEDED);
> +
> +	runner_assert(ctl->surface_get_position(
> +		      ivisurf, &dest_x, &dest_y) == IVI_SUCCEEDED);
> +	runner_assert(dest_x == 0);
> +	runner_assert(dest_y == 0);
> +
> +	ctl->commit_changes();
> +
> +	runner_assert(ctl->surface_get_position(
> +		      ivisurf, &dest_x, &dest_y) == IVI_SUCCEEDED);
> +	runner_assert(dest_x == 20);
> +	runner_assert(dest_y == 30);
> +
> +	prop = ctl->get_properties_of_surface(ivisurf);
> +	runner_assert_or_return(prop);
> +	runner_assert(prop->dest_x == 20);
> +	runner_assert(prop->dest_y == 30);
> +}
> +
> +RUNNER_TEST(surface_destination_rectangle)
> +{
> +	const struct ivi_controller_interface *ctl = ctx->controller_interface;
> +	struct ivi_layout_surface *ivisurf;
> +	const struct ivi_layout_surface_properties *prop;
> +	int32_t dest_width;
> +	int32_t dest_height;
> +	int32_t dest_x;
> +	int32_t dest_y;
> +
> +	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
> +	runner_assert(ivisurf != NULL);

The use of an explicit "!= NULL" is not consistent with other places in
the code that merely checks "(ivisurf)".

> +
> +	prop = ctl->get_properties_of_surface(ivisurf);
> +	runner_assert_or_return(prop);
> +	runner_assert(prop->dest_width == 1);
> +	runner_assert(prop->dest_height == 1);
> +	runner_assert(prop->dest_x == 0);
> +	runner_assert(prop->dest_y == 0);
> +
> +	runner_assert(ctl->surface_set_destination_rectangle(
> +		      ivisurf, 20, 30, 200, 300) == IVI_SUCCEEDED);
> +
> +	prop = ctl->get_properties_of_surface(ivisurf);
> +	runner_assert_or_return(prop);
> +	runner_assert(prop->dest_width == 1);
> +	runner_assert(prop->dest_height == 1);
> +	runner_assert(prop->dest_x == 0);
> +	runner_assert(prop->dest_y == 0);
> +
> +	ctl->commit_changes();
> +
> +	runner_assert(ctl->surface_get_dimension(
> +		      ivisurf, &dest_width, &dest_height) == IVI_SUCCEEDED);
> +	runner_assert(dest_width == 200);
> +	runner_assert(dest_height == 300);
> +
> +	runner_assert(ctl->surface_get_position(ivisurf, &dest_x, &dest_y) == IVI_SUCCEEDED);
> +	runner_assert(dest_x == 20);
> +	runner_assert(dest_y == 30);
> +
> +	prop = ctl->get_properties_of_surface(ivisurf);
> +	runner_assert_or_return(prop);
> +	runner_assert(prop->dest_width == 200);
> +	runner_assert(prop->dest_height == 300);
> +	runner_assert(prop->dest_x == 20);
> +	runner_assert(prop->dest_y == 30);
> +}
> +
> +RUNNER_TEST(surface_source_rectangle)
> +{
> +	const struct ivi_controller_interface *ctl = ctx->controller_interface;
> +	struct ivi_layout_surface *ivisurf;
> +	const struct ivi_layout_surface_properties *prop;
> +
> +	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
> +	runner_assert(ivisurf != NULL);

The use of an explicit "!= NULL" is not consistent with other places in
the code that merely checks "(ivisurf)".

> +
> +	prop = ctl->get_properties_of_surface(ivisurf);
> +	runner_assert_or_return(prop);
> +	runner_assert(prop->source_width == 0);
> +	runner_assert(prop->source_height == 0);
> +	runner_assert(prop->source_x == 0);
> +	runner_assert(prop->source_y == 0);
> +
> +	runner_assert(ctl->surface_set_source_rectangle(
> +		      ivisurf, 20, 30, 200, 300) == IVI_SUCCEEDED);
> +
> +	prop = ctl->get_properties_of_surface(ivisurf);
> +	runner_assert_or_return(prop);
> +	runner_assert(prop->source_width == 0);
> +	runner_assert(prop->source_height == 0);
> +	runner_assert(prop->source_x == 0);
> +	runner_assert(prop->source_y == 0);
> +
> +	ctl->commit_changes();
>  
>  	prop = ctl->get_properties_of_surface(ivisurf);
> -	runner_assert_or_return(prop->opacity == wl_fixed_from_double(0.5));
> +	runner_assert_or_return(prop);
> +	runner_assert(prop->source_width == 200);
> +	runner_assert(prop->source_height == 300);
> +	runner_assert(prop->source_x == 20);
> +	runner_assert(prop->source_y == 30);
>  }
> diff --git a/tests/ivi_layout-test.c b/tests/ivi_layout-test.c
> index d6401c4..ba0387b 100644
> --- a/tests/ivi_layout-test.c
> +++ b/tests/ivi_layout-test.c
> @@ -190,6 +190,11 @@ ivi_window_destroy(struct ivi_window *wnd)
>  const char * const basic_test_names[] = {
>  	"surface_visibility",
>  	"surface_opacity",
> +	"surface_orientation",
> +	"surface_dimension",
> +	"surface_position",
> +	"surface_destination_rectangle",
> +	"surface_source_rectangle",
>  };
>  
>  TEST_P(ivi_layout_runner, basic_test_names)
>
On Mon, 22 Jun 2015 17:28:23 -0700
"Jon A. Cruz" <jonc@osg.samsung.com> wrote:

> On 06/21/2015 11:33 PM, Nobuhiko Tanibata wrote:
> > From: Nobuhiko Tanibata <NOBUHIKO_TANIBATA@xddp.denso.co.jp>
> > 
> > These tests are implemented on test suite framework, which provides
> > helper client.
> > Following features are tested for ivi-surface
> > - orientation
> > - dimension
> > - position
> > - destination rectangle
> > - source rectangle
> > 
> > Signed-off-by: Nobuhiko Tanibata <NOBUHIKO_TANIBATA@xddp.denso.co.jp>
> > Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
> 
> These generally look good, but there are a few 'gotcha' places were
> conversion from runner_assert_or_return() calls to runner_assert() ones.
> 
> Several places check to see if ivisurf is valid. If the value is not
> good then none of the subsequent testing should be attempted, thus the
> code should keep things to runner_assert_or_return();
> 
> In most of the cases the code probably still warrant the *_or_return()
> calls. Aside from attempting to test an invalid surface (if
> runner_assert(ivisurf) fails), much of the testing in not valid if it
> continues.
> 
> For example, after an attempt to set a property fails, then any further
> testing of the state would be invalid.
> 
> Removing the _or_return() part should only be in those places where
> failures in the middle of a test function don't actually affect
> subsequent code in that test.

Hi,

thanks for taking a look.

That's a bit tricky. My guideline here has been that assert_or_return()
should avoid crashing the test (the compositor), because a crash would
cause a lot of also unrelated tests to be skipped. In all other cases
I'd favour the non-return flavour.

The ivi-layout API is different to Weston conventions in that it
handles most NULL arguments as a no-op with a logged complaint. You can
argue whether that's good or bad, but most NULL arguments wouldn't lead
to a crash here.

Many functions here have several things being tested in each function,
each thing having one or more checks. We have to choose between failing
a check will not run any following things at all vs. one failure
causing other failures to appear. Personally I'd prefer to see the
failures than to skip tests based on that "we know it will fail if this
failed". Rationale:
- simpler code, much less conditional paths
- unrelated checks don't get skipped for no reason
- less prone to accidentally skip clean-up, which would cause spurious
  failures in unrelated tests

Isn't this in line with your new framework? assert_or_return() is like
ZUC_FATAL and non-return is ZUC_FAIL?

I suppose your new testing framework might help with ensuring we don't
at least skip the clean-ups?

I know I criticized the use of "return" in your macros, I've done the
same "mistake" here myself. So maybe your framework could use a
ZUC_PROPAGATE_FATAL or such macro that checks the status and retuns
again if it's FATAL. It might be the best we can do with the macros,
since longjmp() is much much worse.

Does this explanation satisfy you?

There could be places where the flavour of runner_assert really is
wrong, though.

> 
> > ---
> >  tests/ivi_layout-test-plugin.c | 219 ++++++++++++++++++++++++++++++++++++++---
> >  tests/ivi_layout-test.c        |   5 +
> >  2 files changed, 211 insertions(+), 13 deletions(-)
> > 
> > diff --git a/tests/ivi_layout-test-plugin.c b/tests/ivi_layout-test-plugin.c
> > index b4abfbf..1d0ce02 100644
> > --- a/tests/ivi_layout-test-plugin.c
> > +++ b/tests/ivi_layout-test-plugin.c
> > @@ -303,16 +303,16 @@ RUNNER_TEST(surface_create_p1)
> >  	uint32_t ivi_id;
> >  
> >  	ivisurf[0] = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
> > -	runner_assert_or_return(ivisurf[0]);
> > +	runner_assert(ivisurf[0]);
> >  
> >  	ivisurf[1] = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(1));
> > -	runner_assert_or_return(ivisurf[1]);
> > +	runner_assert(ivisurf[1]);
> >  
> >  	ivi_id = ctl->get_id_of_surface(ivisurf[0]);
> > -	runner_assert_or_return(ivi_id == IVI_TEST_SURFACE_ID(0));
> > +	runner_assert(ivi_id == IVI_TEST_SURFACE_ID(0));
> >  
> >  	ivi_id = ctl->get_id_of_surface(ivisurf[1]);
> > -	runner_assert_or_return(ivi_id == IVI_TEST_SURFACE_ID(1));
> > +	runner_assert(ivi_id == IVI_TEST_SURFACE_ID(1));
> >  }
> >  RUNNER_TEST(surface_create_p2)
> > @@ -322,7 +322,7 @@ RUNNER_TEST(surface_create_p2)
> >  
> >  	/* the ivi_surface was destroyed by the client */
> >  	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
> > -	runner_assert_or_return(ivisurf == NULL);
> > +	runner_assert(ivisurf == NULL);
> >  }
> >  
> >  RUNNER_TEST(surface_visibility)
> > @@ -334,18 +334,18 @@ RUNNER_TEST(surface_visibility)
> >  	const struct ivi_layout_surface_properties *prop;
> >  
> >  	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
> > -	runner_assert_or_return(ivisurf);
> > +	runner_assert(ivisurf);
> >  
> >  	ret = ctl->surface_set_visibility(ivisurf, true);
> > -	runner_assert_or_return(ret == IVI_SUCCEEDED);
> > +	runner_assert(ret == IVI_SUCCEEDED);
> >  
> >  	ctl->commit_changes();
> >  
> >  	visibility = ctl->surface_get_visibility(ivisurf);
> > -	runner_assert_or_return(visibility == true);
> > +	runner_assert(visibility == true);
> >  
> >  	prop = ctl->get_properties_of_surface(ivisurf);
> > -	runner_assert_or_return(prop->visibility == true);
> > +	runner_assert(prop->visibility == true);
> >  }
> >  
> >  RUNNER_TEST(surface_opacity)
> > @@ -357,16 +357,209 @@ RUNNER_TEST(surface_opacity)
> >  	const struct ivi_layout_surface_properties *prop;
> >  
> >  	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
> > -	runner_assert_or_return(ivisurf);
> > +	runner_assert(ivisurf);
> > +
> > +	runner_assert(ctl->surface_get_opacity(ivisurf) ==
> > +		      wl_fixed_from_double(1.0));
> >  
> >  	ret = ctl->surface_set_opacity(ivisurf, wl_fixed_from_double(0.5));
> > -	runner_assert_or_return(ret == IVI_SUCCEEDED);
> > +	runner_assert(ret == IVI_SUCCEEDED);
> > +
> > +	runner_assert(ctl->surface_get_opacity(ivisurf) ==
> > +		      wl_fixed_from_double(1.0));
> >  
> >  	ctl->commit_changes();
> >  
> >  	opacity = ctl->surface_get_opacity(ivisurf);
> > -	runner_assert_or_return(opacity == wl_fixed_from_double(0.5));
> > +	runner_assert(opacity == wl_fixed_from_double(0.5));
> > +
> > +	prop = ctl->get_properties_of_surface(ivisurf);
> > +	runner_assert(prop->opacity == wl_fixed_from_double(0.5));
> > +}
> > +
> > +RUNNER_TEST(surface_orientation)
> > +{
> > +	const struct ivi_controller_interface *ctl = ctx->controller_interface;
> > +	struct ivi_layout_surface *ivisurf;
> > +	const struct ivi_layout_surface_properties *prop;
> > +
> > +	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
> > +	runner_assert(ivisurf != NULL);
> 
> The use of an explicit "!= NULL" is not consistent with other places in
> the code that merely checks "(ivisurf)".

Yeah, all those would be nice to clean up, but could be a follow-up too.


Thanks,
pq
With those details explained, this looks good.

Reviewed-by: Jon A. Cruz <jonc@osg.samsung.com>


On 06/23/2015 05:56 AM, Pekka Paalanen wrote:
> On Mon, 22 Jun 2015 17:28:23 -0700
> "Jon A. Cruz" <jonc@osg.samsung.com> wrote:
> 
>> On 06/21/2015 11:33 PM, Nobuhiko Tanibata wrote:
>>> From: Nobuhiko Tanibata <NOBUHIKO_TANIBATA@xddp.denso.co.jp>
>>>
>>> These tests are implemented on test suite framework, which provides
>>> helper client.
>>> Following features are tested for ivi-surface
>>> - orientation
>>> - dimension
>>> - position
>>> - destination rectangle
>>> - source rectangle
>>>
>>> Signed-off-by: Nobuhiko Tanibata <NOBUHIKO_TANIBATA@xddp.denso.co.jp>
>>> Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
>>
>> These generally look good, but there are a few 'gotcha' places were
>> conversion from runner_assert_or_return() calls to runner_assert() ones.
>>
>> Several places check to see if ivisurf is valid. If the value is not
>> good then none of the subsequent testing should be attempted, thus the
>> code should keep things to runner_assert_or_return();
>>
>> In most of the cases the code probably still warrant the *_or_return()
>> calls. Aside from attempting to test an invalid surface (if
>> runner_assert(ivisurf) fails), much of the testing in not valid if it
>> continues.
>>
>> For example, after an attempt to set a property fails, then any further
>> testing of the state would be invalid.
>>
>> Removing the _or_return() part should only be in those places where
>> failures in the middle of a test function don't actually affect
>> subsequent code in that test.
> 
> Hi,
> 
> thanks for taking a look.
> 
> That's a bit tricky. My guideline here has been that assert_or_return()
> should avoid crashing the test (the compositor), because a crash would
> cause a lot of also unrelated tests to be skipped. In all other cases
> I'd favour the non-return flavour.
> 
> The ivi-layout API is different to Weston conventions in that it
> handles most NULL arguments as a no-op with a logged complaint. You can
> argue whether that's good or bad, but most NULL arguments wouldn't lead
> to a crash here.
> 
> Many functions here have several things being tested in each function,
> each thing having one or more checks. We have to choose between failing
> a check will not run any following things at all vs. one failure
> causing other failures to appear. Personally I'd prefer to see the
> failures than to skip tests based on that "we know it will fail if this
> failed". Rationale:
> - simpler code, much less conditional paths
> - unrelated checks don't get skipped for no reason
> - less prone to accidentally skip clean-up, which would cause spurious
>   failures in unrelated tests
> 
> Isn't this in line with your new framework? assert_or_return() is like
> ZUC_FATAL and non-return is ZUC_FAIL?
> 
> I suppose your new testing framework might help with ensuring we don't
> at least skip the clean-ups?
> 
> I know I criticized the use of "return" in your macros, I've done the
> same "mistake" here myself. So maybe your framework could use a
> ZUC_PROPAGATE_FATAL or such macro that checks the status and retuns
> again if it's FATAL. It might be the best we can do with the macros,
> since longjmp() is much much worse.
> 
> Does this explanation satisfy you?
> 
> There could be places where the flavour of runner_assert really is
> wrong, though.
> 
>>
>>> ---
>>>  tests/ivi_layout-test-plugin.c | 219 ++++++++++++++++++++++++++++++++++++++---
>>>  tests/ivi_layout-test.c        |   5 +
>>>  2 files changed, 211 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/tests/ivi_layout-test-plugin.c b/tests/ivi_layout-test-plugin.c
>>> index b4abfbf..1d0ce02 100644
>>> --- a/tests/ivi_layout-test-plugin.c
>>> +++ b/tests/ivi_layout-test-plugin.c
>>> @@ -303,16 +303,16 @@ RUNNER_TEST(surface_create_p1)
>>>  	uint32_t ivi_id;
>>>  
>>>  	ivisurf[0] = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
>>> -	runner_assert_or_return(ivisurf[0]);
>>> +	runner_assert(ivisurf[0]);
>>>  
>>>  	ivisurf[1] = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(1));
>>> -	runner_assert_or_return(ivisurf[1]);
>>> +	runner_assert(ivisurf[1]);
>>>  
>>>  	ivi_id = ctl->get_id_of_surface(ivisurf[0]);
>>> -	runner_assert_or_return(ivi_id == IVI_TEST_SURFACE_ID(0));
>>> +	runner_assert(ivi_id == IVI_TEST_SURFACE_ID(0));
>>>  
>>>  	ivi_id = ctl->get_id_of_surface(ivisurf[1]);
>>> -	runner_assert_or_return(ivi_id == IVI_TEST_SURFACE_ID(1));
>>> +	runner_assert(ivi_id == IVI_TEST_SURFACE_ID(1));
>>>  }
>>>  RUNNER_TEST(surface_create_p2)
>>> @@ -322,7 +322,7 @@ RUNNER_TEST(surface_create_p2)
>>>  
>>>  	/* the ivi_surface was destroyed by the client */
>>>  	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
>>> -	runner_assert_or_return(ivisurf == NULL);
>>> +	runner_assert(ivisurf == NULL);
>>>  }
>>>  
>>>  RUNNER_TEST(surface_visibility)
>>> @@ -334,18 +334,18 @@ RUNNER_TEST(surface_visibility)
>>>  	const struct ivi_layout_surface_properties *prop;
>>>  
>>>  	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
>>> -	runner_assert_or_return(ivisurf);
>>> +	runner_assert(ivisurf);
>>>  
>>>  	ret = ctl->surface_set_visibility(ivisurf, true);
>>> -	runner_assert_or_return(ret == IVI_SUCCEEDED);
>>> +	runner_assert(ret == IVI_SUCCEEDED);
>>>  
>>>  	ctl->commit_changes();
>>>  
>>>  	visibility = ctl->surface_get_visibility(ivisurf);
>>> -	runner_assert_or_return(visibility == true);
>>> +	runner_assert(visibility == true);
>>>  
>>>  	prop = ctl->get_properties_of_surface(ivisurf);
>>> -	runner_assert_or_return(prop->visibility == true);
>>> +	runner_assert(prop->visibility == true);
>>>  }
>>>  
>>>  RUNNER_TEST(surface_opacity)
>>> @@ -357,16 +357,209 @@ RUNNER_TEST(surface_opacity)
>>>  	const struct ivi_layout_surface_properties *prop;
>>>  
>>>  	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
>>> -	runner_assert_or_return(ivisurf);
>>> +	runner_assert(ivisurf);
>>> +
>>> +	runner_assert(ctl->surface_get_opacity(ivisurf) ==
>>> +		      wl_fixed_from_double(1.0));
>>>  
>>>  	ret = ctl->surface_set_opacity(ivisurf, wl_fixed_from_double(0.5));
>>> -	runner_assert_or_return(ret == IVI_SUCCEEDED);
>>> +	runner_assert(ret == IVI_SUCCEEDED);
>>> +
>>> +	runner_assert(ctl->surface_get_opacity(ivisurf) ==
>>> +		      wl_fixed_from_double(1.0));
>>>  
>>>  	ctl->commit_changes();
>>>  
>>>  	opacity = ctl->surface_get_opacity(ivisurf);
>>> -	runner_assert_or_return(opacity == wl_fixed_from_double(0.5));
>>> +	runner_assert(opacity == wl_fixed_from_double(0.5));
>>> +
>>> +	prop = ctl->get_properties_of_surface(ivisurf);
>>> +	runner_assert(prop->opacity == wl_fixed_from_double(0.5));
>>> +}
>>> +
>>> +RUNNER_TEST(surface_orientation)
>>> +{
>>> +	const struct ivi_controller_interface *ctl = ctx->controller_interface;
>>> +	struct ivi_layout_surface *ivisurf;
>>> +	const struct ivi_layout_surface_properties *prop;
>>> +
>>> +	ivisurf = ctl->get_surface_from_id(IVI_TEST_SURFACE_ID(0));
>>> +	runner_assert(ivisurf != NULL);
>>
>> The use of an explicit "!= NULL" is not consistent with other places in
>> the code that merely checks "(ivisurf)".
> 
> Yeah, all those would be nice to clean up, but could be a follow-up too.
> 
> 
> Thanks,
> pq
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>