[PATCH 2/3] tests: remove assert from frame_callback_wait()

[Marek Chalupa]

On 7 July 2014 17:22, Pekka Paalanen <ppaalanen at gmail.com> wrote:

> On Mon, 26 May 2014 16:58:06 +0200
> Marek Chalupa <mchqwerty at gmail.com> wrote:
>
> > This function did dispatching wrapped in assert so when an error came,
> > the test was aborted. Now, with expect_protocol_error, we need
> > this function to abort only when we are not calling expect_protocol_error
> > afterwards. So instead of calling the assert inside of this function,
> > wrap this function into assert in appropriate places.
> > ---
> >  tests/event-test.c                | 6 +++---
> >  tests/weston-test-client-helper.c | 7 +++++--
> >  tests/weston-test-client-helper.h | 2 +-
> >  3 files changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/tests/event-test.c b/tests/event-test.c
> > index 980dfaa..facce49 100644
> > --- a/tests/event-test.c
> > +++ b/tests/event-test.c
> > @@ -395,7 +395,7 @@ TEST(buffer_release)
> >       wl_surface_attach(surface, buf2, 0, 0);
> >       frame_callback_set(surface, &frame);
> >       wl_surface_commit(surface);
> > -     frame_callback_wait(client, &frame);
> > +     assert(frame_callback_wait(client, &frame));
> >       assert(buf1_released == 0);
> >       /* buf2 may or may not be released */
> >       assert(buf3_released == 0);
> > @@ -403,7 +403,7 @@ TEST(buffer_release)
> >       wl_surface_attach(surface, buf3, 0, 0);
> >       frame_callback_set(surface, &frame);
> >       wl_surface_commit(surface);
> > -     frame_callback_wait(client, &frame);
> > +     assert(frame_callback_wait(client, &frame));
> >       assert(buf1_released == 0);
> >       assert(buf2_released == 1);
> >       /* buf3 may or may not be released */
> > @@ -411,7 +411,7 @@ TEST(buffer_release)
> >       wl_surface_attach(surface, client->surface->wl_buffer, 0, 0);
> >       frame_callback_set(surface, &frame);
> >       wl_surface_commit(surface);
> > -     frame_callback_wait(client, &frame);
> > +     assert(frame_callback_wait(client, &frame));
> >       assert(buf1_released == 0);
> >       assert(buf2_released == 1);
> >       assert(buf3_released == 1);
> > diff --git a/tests/weston-test-client-helper.c
> b/tests/weston-test-client-helper.c
> > index fb2e477..f0b1b06 100644
> > --- a/tests/weston-test-client-helper.c
> > +++ b/tests/weston-test-client-helper.c
> > @@ -80,12 +80,15 @@ frame_callback_set(struct wl_surface *surface, int
> *done)
> >       return callback;
> >  }
> >
> > -void
> > +int
> >  frame_callback_wait(struct client *client, int *done)
> >  {
> >       while (!*done) {
> > -             assert(wl_display_dispatch(client->wl_display) >= 0);
> > +             if (wl_display_dispatch(client->wl_display) < 0)
> > +                     return 0;
> >       }
> > +
> > +     return 1;
> >  }
> >
> >  void
> > diff --git a/tests/weston-test-client-helper.h
> b/tests/weston-test-client-helper.h
> > index f154661..18c9e39 100644
> > --- a/tests/weston-test-client-helper.h
> > +++ b/tests/weston-test-client-helper.h
> > @@ -120,7 +120,7 @@ move_client(struct client *client, int x, int y);
> >  struct wl_callback *
> >  frame_callback_set(struct wl_surface *surface, int *done);
> >
> > -void
> > +int
> >  frame_callback_wait(struct client *client, int *done);
> >
>
> Hi,
>
> I think you may have missed the frame_callback_wait() in
> move_client().
>

Yes


>
> If a test is calling frame_callback_wait() without assert() and
> without expect_protocol_error(), and simply returns from the test
> function, do we still get a test failure if there was any error?
>
>
No. There's no way how to fail the test without manually checking for
errors, because the wl_display object is not accessible to the test-runner
and if
the test is not aborted from inside, the test_run function returns
EXIT_SUCCESS.
So if programmer doesn't check for errors at the end of the test, the test
won't fail if there's an error.
Here in libwayland:
http://lists.freedesktop.org/archives/wayland-devel/2014-May/015096.html
I solved it by checking for errors in destroying client (in
client_disconnect function).

Another solution is to return EXIT_FAILURE (or another non-zero value) from
the test_run (
http://cgit.freedesktop.org/wayland/weston/tree/tests/weston-test-runner.c#n55
)
which would force the programmers to call exit(0) upon successful end of
test. This would require to rewrite (add exit(0) at the end of) all the
tests, though.


> Maybe it would be better to leave frame_callback_wait() as is, and
> add a new function frame_callback_wait_nofail() or something that
> would not cause the test to fail if it hits an error from
> wl_display. The change would be smaller to the test code, and
> uncaught uses of the old function would not be silent false-passes.
>
>
Yea, maybe it's better.


>
> Thanks,
> pq
>

Thanks,
Marek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140709/c719a9b0/attachment.html>