[PATCH weston v2] tests: Adding simple waycheck validation tool.

[Daniel Stone]

Hi,
Thanks a bunch for this - I agree with basically everything that's been said.

On 25 February 2016 at 16:06, Pekka Paalanen <ppaalanen at gmail.com> wrote:
>> +     wl_list_for_each_safe(entry, tmp, &pointer->private->link, link)
>> +             if (entry->listener && entry->listener->enter)
>> +                     entry->listener->enter(entry->data, wl_pointer,
>> +                                            serial, wl_surface, x, y);
>
> I think checking for entry->listener and entry->listener->enter should
> not be here. I would consider it a test writer's mistake to add a
> listener that doesn't have a listener with all function pointers set.
> On the other hand, it could be convenient if a test that is only
> interested in few events does not have to fill in all hooks with
> stubs, but it seems waycheck.c already has stubs for everything.
>
> Btw. this brings up an interesting question about protocol interface
> versions. If one implements a new version of an interface in
> wtst_fixtures, does that imply that one must update all tests and other
> fixtures to manage the new version too?
>
> What about passing the maximum supported protocol interface version as
> an argument to the fixture initializer somehow? Assert that it's within
> what the fixture itself supports. But that will get awkward as the
> number of wrapped interfaces grows.
>
> These comments apply also to all wl_pointer event handlers below.

It seems like it'd definitely be good to add the version, to test the
semantics of different interface versions (i.e. that we really are
compatible with old clients).

> Only one thing in waycheck calls wtst_pointer_add_listener(), and I
> can't see it doing any actual testing. It is hooked up to set a cursor
> and check some things, but I cannot see anything actually controlling
> the pointer in the compositor and causing the events you expect.
> Therefore I'd propose to remove wtst_pointer_add_listener() and support
> for it for now to get initial waycheck landed, and come back to it
> later.

Yeah. If there's boilerplate like this which exists as a kind of
proof-of-concept, I'd prefer to see it removed (not that you have to
delete it: feel free to add it on as a follow-on patch marked RFC, so
it's not lost) to make this easier to review and merge.

>> +static void
>> +reg_global_remove(void *data,
>> +               struct wl_registry *registry,
>> +               uint32_t name)
>> +{
>> +     struct wtst_ctx *ctx = (struct wtst_ctx *)data;
>> +
>> +     /*
>> +      * Note that removal of wl_compositor, wl_shm and wl_shell are not
>> +      * likely to occur. However, handling the cases will make crashes
>> +      * easier to debug if some implementation has an odd behavior.
>> +      */
>
> I think we could just abort() the whole test if any of the below get
> removed. Their removal is something that literally no-one expects to
> handle.

Plus, I disagree with 'easier to debug'. Trying to handle corner cases
like this in core test suite code (irony: those codepaths will _never_
be tested) makes life more difficult for test writers, because it
makes it harder to reason about the behaviour of the framework you're
building on. Which isn't really what you want in a test suite - boring
and predictable is an absolute must.

If we do ever need to cross that bridge, then we can do it when we've
established that a) it's a necessity, and b) we actually have the
_tests_ in place to make sure that the code does what it says it does.

Cheers,
Daniel