[PATCH 1/3] tests: add expect_protocol_error function

[Bryce W. Harrington]

On Mon, May 26, 2014 at 04:58:05PM +0200, Marek Chalupa wrote:
> This function checks if a particular protocol error came in wire.
> It's usefull in the cases where we hitherto used FAIL_TEST.
> The problem with FAIL_TEST is that *any* assert will pass the test,
> but we want only some asserts to pass the test (i. e. we don't
> want the test to pass when it, for example, can't connect to display).
> FAIL_TESTs are good only for sanity testing.
> 
> The expect_protocol_error allows us to turn all FAIL_TESTs to TESTs
> as will be introduced in following patches.
> ---
>  tests/weston-test-client-helper.c | 49 +++++++++++++++++++++++++++++++++++++++
>  tests/weston-test-client-helper.h |  4 ++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/tests/weston-test-client-helper.c b/tests/weston-test-client-helper.c
> index 186b395..fb2e477 100644
> --- a/tests/weston-test-client-helper.c
> +++ b/tests/weston-test-client-helper.c
> @@ -26,6 +26,7 @@
>  #include <stdio.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include <errno.h>
>  #include <sys/mman.h>
>  
>  #include "../shared/os-compatibility.h"
> @@ -512,6 +513,54 @@ skip(const char *fmt, ...)
>  	exit(77);
>  }
>  
> +void
> +expect_protocol_error(struct client *client,
> +		      const struct wl_interface *intf,
> +		      uint32_t code)
> +{
> +	int err;
> +	uint32_t errcode, failed = 0;
> +	const struct wl_interface *interface;
> +	unsigned int id;
> +
> +	/* if the error has not came yet, make it happen */

"has not come yet"

> +	wl_display_roundtrip(client->wl_display);
> +
> +	err = wl_display_get_error(client->wl_display);
> +
> +	assert(err && "Expected protocol error but nothing came");
> +	assert(err == EPROTO && "Expected protocol error but got local error");
> +
> +
> +	errcode = wl_display_get_protocol_error(client->wl_display,
> +						&interface, &id);
> +
> +	/* check error */
> +	if (errcode != code) {
> +		fprintf(stderr, "Should get error code %d but got %d\n",
> +			code, errcode);
> +		failed = 1;
> +	}
> +
> +	/* this should be definitely set */
> +	assert(interface);
> +
> +	if (strcmp(intf->name, interface->name) != 0) {
> +		fprintf(stderr, "Should get interface '%s' but got '%s'\n",
> +			intf->name, interface->name);
> +		failed = 1;
> +	}
> +
> +	if (failed) {
> +		fprintf(stderr, "Expected other protocol error\n");
> +		abort();
> +	}
> +
> +	/* all OK */
> +	fprintf(stderr, "Got expected protocol error on '%s' (object id: %d) "
> +			"with code %d\n", interface->name, id, errcode);
> +}
> +

I think if you move the assert higher, you can condense the logic a
bit.  Maybe it's clearer:

	/* this should be definitely set */
	assert(interface);

	if (errcode != code) {
		fprintf(stderr, "Should get error code %d but got %d\n",
			code, errcode);
	} else if (strcmp(intf->name, interface->name) != 0) {
		fprintf(stderr, "Should get interface '%s' but got '%s'\n",
			intf->name, interface->name);
	} else {
		fprintf(stderr, "Got expected protocol error on '%s' (object id: %d) "
			"with code %d\n", interface->name, id, errcode);
		return;
	}

	fprintf(stderr, "Expected a different protocol error\n");
	abort();
}

I wonder if the final error message is a bit redundant.

>  static void
>  log_handler(const char *fmt, va_list args)
>  {
> diff --git a/tests/weston-test-client-helper.h b/tests/weston-test-client-helper.h
> index 4bfc3fa..f154661 100644
> --- a/tests/weston-test-client-helper.h
> +++ b/tests/weston-test-client-helper.h
> @@ -129,4 +129,8 @@ get_n_egl_buffers(struct client *client);
>  void
>  skip(const char *fmt, ...);
>  
> +void
> +expect_protocol_error(struct client *client,
> +		      const struct wl_interface *intf, uint32_t code);
> +
>  #endif


Reviewed-by: Bryce Harrington <b.harrington at samsung.com>