<div dir="ltr"><div><div>Hi, </div>I fixed the comment as Bryce wrote (s/came/come) and fixed a white-space error. </div><div>The logic of this version of the patch is equivalent to the old one. </div><div> </div>Thanks, Marek <div><div class="gmail_extra"> <div class="gmail_quote">On 7 July 2014 20:41, Bryce W. Harrington <<a href="mailto:b.harrington@samsung.com" target="_blank">b.harrington@samsung.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Mon, Jul 07, 2014 at 05:47:41PM +0300, Pekka Paalanen wrote: > On Thu, 19 Jun 2014 02:37:40 +0000 > "Bryce W. Harrington" <<a href="mailto:b.harrington@samsung.com">b.harrington@samsung.com</a>> wrote: > > > 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 <<a href="mailto:b.harrington@samsung.com">b.harrington@samsung.com</a>> > > Hi Bryce, > > is this a reviewed-by also for the original patch without > suggested changes? </div></div>Yes <div class=""> > And only for patch 1? </div>I didn't see any issues with patches 2 and 3. Bryce</blockquote></div> </div></div></div>