[PATCH 01/14] tests: test set for ivi-surface normal use case with helper client
Jon A. Cruz
jonc at osg.samsung.com
Wed Jun 24 10:00:41 PDT 2015
With those details explained, this looks good.
Reviewed-by: Jon A. Cruz <jonc at 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 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
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
--
Jon A. Cruz - Senior Open Source Developer
Samsung Open Source Group
jonc at osg.samsung.com
More information about the wayland-devel
mailing list