With those details explained, this looks good. Reviewed-by: Jon A. Cruz <[email protected]>
On 06/23/2015 05:56 AM, Pekka Paalanen wrote: > On Mon, 22 Jun 2015 17:28:23 -0700 > "Jon A. Cruz" <[email protected]> wrote: > >> On 06/21/2015 11:33 PM, Nobuhiko Tanibata wrote: >>> From: Nobuhiko Tanibata <[email protected]> >>> >>> 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 <[email protected]> >>> Reviewed-by: Pekka Paalanen <[email protected]> >> >> 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 > [email protected] > http://lists.freedesktop.org/mailman/listinfo/wayland-devel > -- Jon A. Cruz - Senior Open Source Developer Samsung Open Source Group [email protected] _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
