[PATCH] tests: fix bad-buffer-test

[Marek Chalupa]

On 10 April 2014 08:10, Pekka Paalanen <ppaalanen at gmail.com> wrote:

> On Wed,  9 Apr 2014 16:44:53 +0200
> Marek Chalupa <mchqwerty at gmail.com> wrote:
>
> > bad-buffer-test is FAIL_TEST and every assert() (or even SIGSEGV signal)
> > make it pass. It shouldn't be so for example when assert() is invoked
> > when a client couldn't connect to display.
> >
> > Make sure that only relevant asserts make the test pass
> > and the other make it fail (by returning 0)
>
> Hi,
>
> yes, the FAIL_TEST is fundamentally broken. When expecting a failure, we
> always expect a particular kind of failure to be the success condition.
>
> Could you describe more, what exactly you were hitting that prompted
> for this patch?
>

When you run the binary as it is, without Weston running, then it
(obviously) can not connect to
display, but the test passes.


>  > ---
> >  tests/bad-buffer-test.c | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/tests/bad-buffer-test.c b/tests/bad-buffer-test.c
> > index 6eae313..4c5eb64 100644
> > --- a/tests/bad-buffer-test.c
> > +++ b/tests/bad-buffer-test.c
> > @@ -25,6 +25,8 @@
> >
> >  #include <unistd.h>
> >  #include <sys/types.h>
> > +#include <err.h>
> > +#include <signal.h>
> >
> >  #include "../shared/os-compatibility.h"
> >  #include "weston-test-client-helper.h"
> > @@ -58,12 +60,30 @@ create_bad_shm_buffer(struct client *client, int
> width, int height)
> >       return buffer;
> >  }
> >
> > +static void sighandler(int signum)
> > +{
> > +     /* this means failure */
> > +     errx(0, "Got %s", (signum == SIGABRT) ? "SIGABRT" : "SIGSEGV");
>
> The manual says: "These functions are nonstandard BSD extensions."
> Can we use those?
>

Sorry, I'm used to use these and did it automatically. It's not necessary,
I will use standard functions.


>
> >
> >       client = client_create(46, 76, 111, 134);
> >       assert(client);
> > @@ -71,9 +91,16 @@ FAIL_TEST(test_truncated_shm_file)
> >
> >       bad_buffer = create_bad_shm_buffer(client, 200, 200);
> >
> > +     /* from this point we expect the signal */
> > +     if (sigaction(SIGABRT, &old_action, NULL) != 0)
> > +             errx(0, "Failed setting old sigaction for SIGABRT");
> > +
>
> Yeah, we should really stop abusing assert() like here, and do
> something simpler than messing with signals. For instance, have the
> testing framework offer a function to say "from now on, we expect a
> protocol error to be raised". That might actually cover most if not all
> of the non-trivial FAIL_TEST tests.
>

Adding a disscusion from irc:

 <mcy> pq: hi, thanks for the reaction on '[PATCH weston] tests: fix
bad-buffer-test'. I was thinking about the part of creating a function that
would tell the test: "from now expect failure"
<mcy> pq: I think the easiest way would be redefine assert (or define
something like weston_assert)
<mcy> pq: I tried it here: http://pastebin.com/HEQtBtJS
<pq> mcy, I think it should be redesigned completely, so that assert() or
equivalent would be used only when the failure must always be fatal and a
disaster
<pq> also in the helper functions outside of any particular test
<pq> mcy, well, that is somewhat on the right idea, but I'd like to see it
being more specific.
<pq> mcy, for example, expect_protocol_error(uint32_t protocol_error_code)
<pq> then the helper functions would know, that if they receive that
protocol error code, the test is successful, and anything else will be a
failure.
<pq> mcy, so, I don't think we should change how "assert" works, but invent
completely new functions to specify the exact nature of the expected
failure, if that failure will be detected by the helper code.

>       wl_surface_attach(surface, bad_buffer, 0, 0);
> >       wl_surface_damage(surface, 0, 0, 200, 200);
> >       frame_callback_set(surface, &frame);
> >       wl_surface_commit(surface);
> >       frame_callback_wait(client, &frame);
> > +
> > +     /* the client should be already disconnected, but make sure */
> > +     client_roundtrip(client);
>
> This should be unnecessary, the frame_callback_wait is already a
> roundtrip. Did you actually need this?
>
>
No, I did not.


> This patch is papering over an underlying fundamental issue in our test
> framework, but I don't see that as a reason to reject this patch.
> Provided the two latter questions I asked are answered "yes", I am ok
> with this patch.
>
>
> Thanks,
> pq
>

Thanks for review, I'll create a new version

Regards,
Marek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140411/ed0adbe6/attachment-0001.html>