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

[Pekka Paalanen]

On Mon, 22 Jun 2015 17:28:23 -0700
"Jon A. Cruz" <jonc at osg.samsung.com> wrote:

> On 06/21/2015 11:33 PM, Nobuhiko Tanibata wrote:
> > From: Nobuhiko Tanibata <NOBUHIKO_TANIBATA at 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 at xddp.denso.co.jp>
> > Reviewed-by: Pekka Paalanen <pekka.paalanen at 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