On Wed, 9 Apr 2014 16:44:53 +0200
Marek Chalupa <[email protected]> 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?
> ---
> 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?
> +}
> +
> FAIL_TEST(test_truncated_shm_file)
> {
> struct client *client;
> struct wl_buffer *bad_buffer;
> struct wl_surface *surface;
> int frame;
> + struct sigaction new_action, old_action;
> +
> + /* until the bad buffer creation, the SIGABRT or SIGSEGV signals
> + * should fail the test. That means returning 0 */
> + new_action.sa_handler = sighandler;
> + sigemptyset(&new_action.sa_mask);
> + new_action.sa_flags = 0;
> +
> + if (sigaction(SIGSEGV, &new_action, NULL) != 0)
> + errx(0, "Failed setting new sigaction for SIGSEGV");
> + if (sigaction(SIGABRT, &new_action, &old_action) != 0)
> + errx(0, "Failed setting new sigaction for SIGABRT");
Yeah, true.
>
> 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.
> 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?
> }
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
_______________________________________________
wayland-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
