<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On 10 April 2014 08:10, Pekka Paalanen <span dir="ltr"><<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>On Wed, 9 Apr 2014 16:44:53 +0200<br>
Marek Chalupa <<a href="mailto:mchqwerty@gmail.com" target="_blank">mchqwerty@gmail.com</a>> wrote:<br>
<br>
> bad-buffer-test is FAIL_TEST and every assert() (or even SIGSEGV signal)<br>
> make it pass. It shouldn't be so for example when assert() is invoked<br>
> when a client couldn't connect to display.<br>
><br>
> Make sure that only relevant asserts make the test pass<br>
> and the other make it fail (by returning 0)<br>
<br>
</div>Hi,<br>
<br>
yes, the FAIL_TEST is fundamentally broken. When expecting a failure, we<br>
always expect a particular kind of failure to be the success condition.<br>
<br>
Could you describe more, what exactly you were hitting that prompted<br>
for this patch?<br>
</blockquote><div><br></div><div>When you run the binary as it is, without Weston running, then it (obviously) can not connect to<br></div><div>display, but the test passes.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>
> ---<br>
> tests/bad-buffer-test.c | 27 +++++++++++++++++++++++++++<br>
> 1 file changed, 27 insertions(+)<br>
><br>
> diff --git a/tests/bad-buffer-test.c b/tests/bad-buffer-test.c<br>
> index 6eae313..4c5eb64 100644<br>
> --- a/tests/bad-buffer-test.c<br>
> +++ b/tests/bad-buffer-test.c<br>
> @@ -25,6 +25,8 @@<br>
><br>
> #include <unistd.h><br>
> #include <sys/types.h><br>
> +#include <err.h><br>
> +#include <signal.h><br>
><br>
> #include "../shared/os-compatibility.h"<br>
> #include "weston-test-client-helper.h"<br>
> @@ -58,12 +60,30 @@ create_bad_shm_buffer(struct client *client, int width, int height)<br>
> return buffer;<br>
> }<br>
><br>
> +static void sighandler(int signum)<br>
> +{<br>
> + /* this means failure */<br>
> + errx(0, "Got %s", (signum == SIGABRT) ? "SIGABRT" : "SIGSEGV");<br>
<br>
</div>The manual says: "These functions are nonstandard BSD extensions."<br>
Can we use those?<br>
<div></div></blockquote><div><br></div><div>Sorry, I'm used to use these and did it automatically. It's not necessary, I will use standard functions.<br><br>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br><div><br>
><br>
> client = client_create(46, 76, 111, 134);<br>
> assert(client);<br>
> @@ -71,9 +91,16 @@ FAIL_TEST(test_truncated_shm_file)<br>
><br>
> bad_buffer = create_bad_shm_buffer(client, 200, 200);<br>
><br>
> + /* from this point we expect the signal */<br>
> + if (sigaction(SIGABRT, &old_action, NULL) != 0)<br>
> + errx(0, "Failed setting old sigaction for SIGABRT");<br>
> +<br>
<br>
</div>Yeah, we should really stop abusing assert() like here, and do<br>
something simpler than messing with signals. For instance, have the<br>
testing framework offer a function to say "from now on, we expect a<br>
protocol error to be raised". That might actually cover most if not all<br>
of the non-trivial FAIL_TEST tests.<br></blockquote><div> </div><div>Adding a disscusion from irc:<br> <br></div><div> <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"<br>
<mcy> pq: I think the easiest way would be redefine assert (or define something like weston_assert)<br><mcy> pq: I tried it here: <a href="http://pastebin.com/HEQtBtJS">http://pastebin.com/HEQtBtJS</a><br><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<br>
<pq> also in the helper functions outside of any particular test<br><pq> mcy, well, that is somewhat on the right idea, but I'd like to see it being more specific.<br><pq> mcy, for example, expect_protocol_error(uint32_t protocol_error_code)<br>
<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.<br><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.<br>
<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>
> wl_surface_attach(surface, bad_buffer, 0, 0);<br>
> wl_surface_damage(surface, 0, 0, 200, 200);<br>
> frame_callback_set(surface, &frame);<br>
> wl_surface_commit(surface);<br>
> frame_callback_wait(client, &frame);<br>
> +<br>
> + /* the client should be already disconnected, but make sure */<br>
> + client_roundtrip(client);<br>
<br>
</div>This should be unnecessary, the frame_callback_wait is already a<br>
roundtrip. Did you actually need this?<br>
<br></blockquote><div> </div><div>No, I did not.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
This patch is papering over an underlying fundamental issue in our test<br>
framework, but I don't see that as a reason to reject this patch.<br>
Provided the two latter questions I asked are answered "yes", I am ok<br>
with this patch.<br>
<br>
<br>
Thanks,<br>
pq<br>
</blockquote></div><br></div><div class="gmail_extra">Thanks for review, I'll create a new version<br><br></div><div class="gmail_extra">Regards,<br></div><div class="gmail_extra">Marek<br></div></div>