[PATCH weston v2] Refactored simple unit/integration test framework and corresponding test program.
Daniel Stone
daniel at fooishbar.org
Wed May 6 03:49:15 PDT 2015
Hi,
On 5 May 2015 at 23:50, Jon A. Cruz <jonc at osg.samsung.com> wrote:
> Refactored to a simple C-based test framework and an example program
> that uses it to run through some simple wayland client checks.
Thanks for this. I'll give some more structural feedback for what I
think should be good design goals etc, separately, but here are a
bunch of stylistic comments, purely in order to make future submission
easier for you. So please don't rework this to match these comments
just yet, as I imagine we'll all have some more suggestions on shed
colouring. :)
In general though, as a rule - just look at the existing codebase and
copy the style of what's already there.
> @@ -0,0 +1,60 @@
> +digraph toolarch_new {
> + rankdir = "TB";
> +
> + node[shape=record]
> +
> + subgraph cluster_0 {
> + label = "./tests";
> +
> + keyboard_test_c [label = "{keyboard-test.c|tests\l}"]
> + text_test_c [label = "{text-test.c|tests\l}"]
> + vertex_clip_test_c [label = "{vertex-clip-test.c|tests\l}"]
Hmm, does this need to be updated for every single test then? Can this
be autogenerated?
> --- /dev/null
> +++ b/tools/tools_arch_old.gv
What's the difference between old and new?
> +static void fillRect(pixel *raw, pixel val, int width, int height, int stride)
underscores please, not camel-case. We also tend to avoid bare
typedefs, so this would be 'struct pixel'.
> +static pixel meld(pixel val1, pixel val2, double factor)
> +{
> + pixel result = val1;
> + unsigned char *p0 = (unsigned char *)&result;
> + unsigned char *p1 = (unsigned char *)&val2;
> + size_t i;
> + for (i = 0; i < sizeof(pixel); ++i) {
> + p0[i] = ((int)(((p0[i] << 8) * (1.0 - factor))
> + + ((p1[i] << 8) * factor))) >> 8;
> + }
> + return result;
> +}
Isn't this more of a blend?
> +void draw_bars(pixel *raw, int width, int height)
> +{
> + pixel bars[] = {0xffb4b4b4, 0xffb4b410, 0xff10b4b4, 0xff10b410,
> + 0xffb410b4, 0xffb41010, 0xff1010b4};
> + if (raw) {
> + int y0 = ((height * 1.75) / 3);
> + int y1 = ((height * 2) / 3);
> + int y2 = ((height * 3) / 4);
> + int h1 = y1 - y0;
> + int h2 = height - y2;
> + int w1 = width / 8;
> + int w2 = (width * 0.75) / 7;
> + int xr = (width * 7) / 8;
> +
> + fillRect(raw, 0x7f1f1f1f, width, height, width);
> +
> + // left gray:
> + fillRect(raw, 0xff686868, w1, y0, width);
> + fillRect(raw + (y0 * width), 0xff10ebeb, w1, h1, width);
> + fillRect(raw + (y1 * width), 0xffebeb10, w1, h1, width);
> + fillRect(raw + (y2 * width), 0xff313131, w1, h2, width);
Yikes, so very many magic numbers. It would be great if these could be
calculated from constants (e.g. ((1.0 * alpha_size) << alpha_shift) |
((0.7 * red_size) << red_shift), etc). This makes it easier to test
16bpp, 10bpc/32bpp, etc. If you had pixel as a structure, you could
then have helpers to determine the correct pixel value at fill time,
e.g.:
struct pixel grey70 = { .a = 1.0, .r = 0.7, .g = 0.7, .b = 0.7 };
and then have the various fill*() functions normalise to an actual
pixel value for your desired format.
> + double barSpan = (width * 0.75);
> + size_t barCount = sizeof(bars) / sizeof(bars[0]);
> + size_t i = 0;
No variable declarations mid-function please. The compiler should've
warned about this ... and also, camelCasing.
> + for (i = 0; i < barCount; ++i)
> + {
Braces on the same line, _except_ for function declarations.
> +void draw_crosshairs(pixel *raw, int width, int height)
> +{
> + if (raw) {
> + pixel fg = 0x7f000000;
> + pixel bg = 0x7f7f7f7f;
> + int x = (width + 1) / 2;
> + int y = (height + 1) / 2;
> +
> + fillRect(raw + ((y - 1) * width), bg,
> + width, 3, width);
> + fillRect(raw + (y * width), fg, width, 1, width);
> +
> + fillRect(raw + ((y - 2) * width), bg,
> + width / 3, 5, width);
> + fillRect(raw + ((y - 1) * width), fg,
> + width / 3, 3, width);
> + fillRect(raw + ((y - 2) * width + (width * 2 / 3)), bg,
> + width / 3, 5, width);
> + fillRect(raw + ((y - 1) * width + (width * 2 / 3)), fg,
> + width / 3, 3, width);
> +
> +//
This comment doesn't seem overly helpful. ;)
> +
> + fillRect(raw + x - 1, bg,
> + 3, height, width);
> + fillRect(raw + x, fg,
> + 1, height, width);
> +
> + fillRect(raw + x - 2, bg,
> + 5, height / 3, width);
> + fillRect(raw + x - 1, fg,
> + 3, height / 3, width);
> + fillRect(raw + x - 2 + ((height * 2 / 3) * width), bg,
> + 5, height / 3, width);
> + fillRect(raw + x - 1 + ((height * 2 / 3) * width), fg,
> + 3, height / 3, width);
> + }
> +}
You can avoid an entire level of indentation here by:
void draw_crosshairs(...)
{
struct pixel fg = {...};
if (!raw)
return;
fillRect(...);
}
> +#ifndef ROUGH_DRAW_H_SEEN_
> +#define ROUGH_DRAW_H_SEEN_
Just ROUGH_DRAW_H, no need for the _SEEN_.
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
Are we expecting C++ code to call this?
> +#include <stdint.h>
> +
> +typedef uint32_t pixel;
If you do retain the ARGB32 hardcoding, calling it pixel_argb32 or
similar would be nice, I think.
> +// - - - - - - - - - -
We don't generally use separator comments. If you want to break into
functional groups, then the Doxygen comment syntax supports that
explicitly, complete with documentation per-group!
> +/**
> + * Baseline test to see if a connection can be made to the Wayland server.
> + */
> +ZUC_TEST(BaseTest, CanConnectDisplay)
SoMuchCamelCase. :(
> +{
> + struct wl_display *display = wl_display_connect(NULL);
> +
> + ZUC_EXPECT_TRUE(display != NULL);
Should this be asserting that $WAYLAND_SOCKET or $WAYLAND_DISPLAY is
set or similar?
> +// A very simple case that only exercises the test fixture.
No C++ comments please. /* only these */.
> +ZUC_TEST(BaseTest, AdvertisesRequired)
> +{
> + char const *required[] = {
> + "wl_compositor",
> + "wl_data_device_manager",
> + //wl_drm
> + //wl_input_method
> + //wl_input_panel
> + "wl_output",
> + //wl_scaler
> + "wl_seat",
> + "wl_shell",
> + "wl_shm",
> + "wl_subcompositor",
> + //wl_text_input_manager
> + };
You can delete all the spurious entries here, or at least move them to
a comment above explaining which ones might be useful to add in
future.
> + wtst_ctx *ctx = wtst_ctx_create();
> +
> + int num_required = sizeof(required) / sizeof(required[0]);
We tend to use an ARRAY_LENGTH macro here.
> + int i;
> + for (i = 0; i < num_required; i++) {
> + printf("Check %s\n", required[i]);
I'll come back to a point about machine-parseable output later, but
this is more or less what I mean.
> +// Forward declaration of internal testing structs:
> +typedef struct _wtst_shm_pool wtst_shm_pool;
> +typedef struct _wtst_pointer wtst_pointer;
> +typedef struct _wtst_shell_surface wtst_shell_surface;
> +typedef struct _wtst_surface wtst_surface;
No struct typedefs please.
> +// Simple testing helpers:
Do these really all need to be forward-declared?
> +static const uint32_t PIXEL_FORMAT = WL_SHM_FORMAT_ARGB8888;
NO_SHOUTY_VARIABLE_NAMES.
> +ZUC_TEST(BaseTest, DoTheWork)
This is a pretty unhelpful test name.
> +{
> + wtst_ctx *ctx = wtst_ctx_create();
> +
> + keep_alive = false; // Don't run the main loop for normal testing
> +
> + // 320x200 == 4:3
> +
> + // 320x180
> + // 355x200~
> + // 480x270
> + // 640x360
What do these comments mean?
> + const int surf_width = 355;
> + const int surf_height = 200;
> +
> + const int cursor_size = 45;
> +
> + size_t needed = sizeof(pixel) * surf_width * surf_height;
> + needed += sizeof(pixel) * cursor_size * cursor_size;
> +
> + int anon = os_create_anonymous_file(needed);
> + wtst_shm_pool *pool = wtst_create_mempool(ctx->shm, anon, needed);
> + struct wl_buffer *buffer = NULL;
> + if (pool) {
> + pixel *mem = wtst_shm_pool_get_membuf(pool);
> + draw_bars(mem, surf_width, surf_height);
> +
> + struct wl_shm_pool *shpool =
> + wtst_shm_pool_get_pool(pool);
> + buffer = wl_shm_pool_create_buffer(shpool,
> + 0,
> + surf_width,
> + surf_height,
> + surf_width * sizeof(pixel),
> + PIXEL_FORMAT);
> + wtst_shm_pool_consume(pool,
> + surf_width * surf_height * sizeof(pixel));
> + }
Why do we attempt to carry on if there's no pool ... ? Shouldn't this
be a cause for failure/skip?
> +// ========================================================================
> +// ========================================================================
> +// ========================================================================
> +// ========================================================================
Separators again; they're not consistent either.
> +pixel *wtst_shm_pool_get_membuf(wtst_shm_pool *pool)
> +{
> + return (pool) ? pool->membuf : NULL;
> +}
> +
> +struct wl_shm_pool *wtst_shm_pool_get_pool(wtst_shm_pool *pool)
> +{
> + return (pool) ? pool->pool : NULL;
> +}
> +
> +static void wtst_shm_pool_consume(wtst_shm_pool *pool, size_t mem_used)
> +{
> + if (pool)
> + pool->used += mem_used;
> +}
Probably worth considering if pool should ever be NULL in this case.
If someone wants to consume a certain amount from a pool, but there is
no pool, does it make sense to carry on and pretend everything's OK? I
suspect not. This kind of defensive programming can be quite useful
sometimes, but it's also often incredibly good at hiding problems. In
this case, I can't really see how it's helpful.
> +wtst_shm_pool *wtst_create_mempool(struct wl_shm *shm, int fd, size_t flen)
> +{
> + wtst_shm_pool *wtst_pool = malloc(sizeof(wtst_shm_pool));
> + if (wtst_pool) {
This is a prime candidate for early-return.
> + memset(wtst_pool, 0, sizeof(*wtst_pool));
We also have zalloc for this.
> +wtst_shell_surface *wtst_create_shell_surface(struct wl_compositor *compositor,
> + struct wl_shell *shell)
> +{
> + wtst_shell_surface *wss = malloc(sizeof(wtst_shell_surface));
> + if (wss) {
> + memset(wss, 0, sizeof(*wss));
> + wss->listener.ping = shell_surface_ping;
> + wss->listener.configure = shell_surface_configure;
> +
> + wss->surface = wl_compositor_create_surface(compositor);
> + }
> +
> + if (wss && wss->surface) {
> + wss->shell_surface = wl_shell_get_shell_surface(shell,
> + wss->surface);
> + if (wss->shell_surface) {
> + wl_shell_surface_add_listener(wss->shell_surface,
> + &wss->listener,
> + wss);
> + wl_shell_surface_set_toplevel(wss->shell_surface);
> + wl_shell_surface_set_user_data(wss->shell_surface, wss);
> + wl_surface_set_user_data(wss->surface, NULL);
> + }
> + }
> +
> + if (wss && (!wss->surface || !wss->shell_surface)) {
Early-return (or in this case, perhaps goto err) would make the
branching much, much more obvious.
> +void set_button_cb(wtst_shell_surface *shell_surface, button_cb callback)
> +{
> + if (shell_surface && shell_surface->surface) {
> + wtst_surface *wsurf =
> + wl_surface_get_user_data(shell_surface->surface);
> + if (!wsurf) {
> + wsurf = malloc(sizeof(wtst_surface));
> + if (wsurf) {
> + memset(wsurf, 0, sizeof(*wsurf));
Wait, how are we expecting to receive an input event on a surface
we've created, without an internal tracking structure for that
surface? This again seems a little bit like hiding problems (or at
least kicking them down the road), and makes lifetime tracking
infinitely less obvious. I get the principle that crashing is bad, but
if the defensiveness here is papering over very real problems in our
lifetime tracking, then that's much worse. In this case, it's better
to surface the problems immediately - crash - and have them fixed.
One of the core principles of Wayland and Weston development is that
we have to be able to immediately and easily reason about the lifetime
of the objects we create. This violates that.
> + wtptr->membuf = pool->membuf;
> + wtptr->membuf += pool->used / sizeof(*wtptr->membuf);
Perhaps this could be part of membuf_consume()? Perhaps combine them both into:
static void *wtst_shm_pool_reserve(struct wtst_shm_pool *pool, size_t
to_reserve)
{
void *ret;
if (pool->used + to_reserve >= pool->size ||
that_check_would_wrap_around)
return NULL;
ret = pool->membuf + pool->used;
pool->used += to_reserve;
return ret;
}
> + draw_crosshairs(wtptr->membuf, width, height);
> + //wtptr->membuf[0] = 0xfffc0000;
Leftovers from testing?
> + wtptr->buffer = wl_shm_pool_create_buffer(pool->pool,
> + pool->used,
> + width,
> + height,
> + stride,
> + PIXEL_FORMAT);
> + pool->used += width * height * sizeof(pixel);
And this bit should use it.
> + }
> + }
> +
> + if (wtptr && wtptr->buffer) {
> + draw_crosshairs(wtptr->membuf, width, height);
> + //wtptr->membuf[0] = 0xffc00000;
> + }
> +
> + if (wtptr && !wtptr->buffer) {
> + if (wtptr->surface)
> + wl_surface_destroy(wtptr->surface);
> + free(wtptr);
> + wtptr = NULL;
> + }
> +
> + wl_pointer_set_user_data(pointer, wtptr);
Ah, so maybe this explains some of the broken tracking. We
deliberately create Wayland objects which may not have an internal
tracking object. That seems broken: why leak the Wayland object? It
should be destroyed along with its tracking object if anything fails.
Keeping the lifetimes of the two strictly in sync removes a lot of
strange mystical lifetime, and allows us to reason sensibly about the
state of everything. Which, when testing, is pretty crucial - if we're
not testing in a well-controlled environment, we're just testing
entropy.
> diff --git a/tools/waycheck/waycheck.dox b/tools/waycheck/waycheck.dox
> new file mode 100644
> index 0000000..66ae65b
> --- /dev/null
> +++ b/tools/waycheck/waycheck.dox
> @@ -0,0 +1,29 @@
> +/*
> + * Copyright © 2015 Samsung Electronics Co., Ltd
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and
> + * its documentation for any purpose is hereby granted without fee, provided
> + * that the above copyright notice appear in all copies and that both that
> + * copyright notice and this permission notice appear in supporting
> + * documentation, and that the name of the copyright holders not be used in
> + * advertising or publicity pertaining to distribution of the software
> + * without specific, written prior permission. The copyright holders make
> + * no representations about the suitability of this software for any
> + * purpose. It is provided "as is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
> + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
> + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER
> + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF
> + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +/**
> + at page waycheck
> +
> +A simple integration/acceptance tool to exercise Wayland compositors.
> +
> +Tests use @ref zunitc for their infrastructure, and most include use of a common wtst_ctx test fixture.
> +*/
Could this not be inline as part of a header?
> +typedef struct wtst_advertised {
> + uint32_t name;
> + char *interface;
> + uint32_t version;
> + //
> + struct wtst_advertised *next;
> +} wtst_advertised;
Please use wl_list instead of your own linked list.
> +struct wtst_ctx_private {
> + int val;
What is 'val'?
> +static int logle = 0;
> +
> +
> +wtst_ctx *wtst_ctx_create(void)
> +{
> + wtst_ctx *ctx = malloc(sizeof(*ctx));
> + if (!ctx) {
> + perror("Unable to allocate context.");
> + } else {
> + memset(ctx, 0, sizeof(*ctx));
> + ctx->reg_listener = malloc(sizeof(*(ctx->reg_listener)));
This can fail.
> + memset(ctx->reg_listener, 0, sizeof(*(ctx->reg_listener)));
> +
> + ctx->display = wl_display_connect(NULL);
> + if (!ctx->display) {
> + perror("Unable to connect to display");
> + free(ctx);
> + ctx = NULL;
> + } else {
> + ctx->private = wtst_ctx_private_create();
Surely this can fail too?
> +wtst_ctx_private *wtst_ctx_private_create(void)
> +{
> + wtst_ctx_private *private = malloc(sizeof(wtst_ctx_private));
> + if (private) {
> + memset(private, 0, sizeof(*private));
> +
> + // fill in simple marker
> + private->val = 0x59;
???
Is this for magic-number checking?
> + }
> +
> + return private;
> +}
> +
> +void wtst_ctx_private_destroy(wtst_ctx_private *private)
> +{
> + if (private) {
> + if (private->adverts) {
> + // TODO
> + }
> + free(private);
> + }
> +}
> +
> +
> +void reg_global(void *data,
> + struct wl_registry *registry,
> + uint32_t name,
> + const char *interface,
> + uint32_t version)
> +{
> + struct wtst_ctx *ctx = (struct wtst_ctx *)data;
> + if (logle > 1) {
> + printf("reg_global ++: %2d %s v%d\n",
> + name, interface, version);
> + }
Again with machine-parseable output. Also, I feel this would be much
better off as a series of macros which test the log level themselves,
i.e. rather than if (logle > somemagicnumber) { printf() } at every
caller, have a series of log_crit(), log_err(), log_warn(), ...
macros. We already have precedent for this in weston_log, which also
allows you to log to files instead.
> + if (strcmp(interface, wl_seat_interface.name) == 0) {
> + if (ctx->seat)
> + wl_seat_destroy(ctx->seat);
We can have multiple seats, so this would be better done as a list of
seats, with a pointer to an arbitrary primary seat.
> + ctx->seat = wl_registry_bind(registry,
> + name,
> + &wl_seat_interface,
> + version);
> + ctx->seat_name = name;
> +
> + ctx->pointer = wl_seat_get_pointer(ctx->seat);
> +
> + } else if (strcmp(interface, wl_compositor_interface.name) == 0) {
> + if (ctx->compositor)
> + wl_compositor_destroy(ctx->compositor);
This should really be an assertion, as there can only be one
wl_compositor object; if we get two, we've screwed up by having
multiple registries on the same context, and anything which was
previously using ctx->compositor is now deep into undefined behaviour.
> + ctx->compositor = wl_registry_bind(registry,
> + name,
> + &wl_compositor_interface,
> + version);
> + ctx->compositor_name = name;
> + } else if (strcmp(interface, wl_shm_interface.name) == 0) {
> + if (ctx->shm)
> + wl_shm_destroy(ctx->shm);
Ditto: singleton.
> + ctx->shm = wl_registry_bind(registry,
> + name,
> + &wl_shm_interface,
> + version);
> + ctx->shm_name = name;
> + } else if (strcmp(interface, wl_shell_interface.name) == 0) {
> + if (ctx->shell)
> + wl_shell_destroy(ctx->shell);
Ditto.
> +void add_global_advert(wtst_ctx *ctx,
> + uint32_t name,
> + const char *interface,
> + uint32_t version)
> +{
> + if (ctx && ctx->private) {
How would we be called with no context here?
> + // See if we've got this interface already
> + wtst_advertised *curr;
> + for (curr = ctx->private->adverts; curr; curr = curr->next) {
> + if (strcmp(curr->interface, interface) == 0)
> + break;
> + }
> +
> + if (curr) {
> + // Update these in case they have changed.
> + curr->name = name;
> + curr->version = version;
Global advertisements don't change. They're added or removed; if we
get multiple advertisements, it's because there are multiple - and
separate - live objects.
> + curr = ctx->private->adverts;
> + while (curr && curr->next) {
> + curr = curr->next;
> + }
> + if (curr)
> + curr->next = advert;
> + else
> + ctx->private->adverts = advert;
wl_list would eliminate this whole body of code.
> +To get started, one or more tests should be defined via ZUC_TEST(), which sets up
> +automatic test registration via gcc extensions. To actually execute tests,
> +ZUC_RUN_TESTS() should be called.
I like this bit!
> +Tests can use several ZUC_EXPECT_* or ZUC_ASSERT_* checks to validate
> +conditions. The ZUC_EXPECT_* ones upon failure will mark the current test
> +as failing, but allow execution to continue. On the other hand, the
> +ZUC_ASSERT_* tests will mark the current test as failed and then immediately
> +terminate the test.
What's the usecase for marked-fail but allowing execution to continue?
> +int zucimpl_testFailed(void);
underscores_and_camelCase_together_make_meQueasy.
> +// Required #define to get clock_gettime() & strdup
> +#define _POSIX_C_SOURCE 200809L
Covered by AC_USE_SYSTEM_EXTENSIONS. Defining it ourselves here leads
to massive headaches down the road.
> +#if _POSIX_MONOTONIC_CLOCK
> +static const clockid_t TARGET_TIMER = CLOCK_MONOTONIC;
> +#else
> +static const clockid_t TARGET_TIMER = CLOCK_REALTIME;
> +#endif
Is REALTIME guaranteed to be monotonic?
> +// Data structure simple, but should suffice for smaller numbers of tests.
> +static int caseListCount = 0;
> +static ZucCase *caseListHead = NULL;
wl_list.
> +int zucimpl_expect_true(int condition, char const *file, int line,
const char *valueStr){
> + int failed = 0;
> + if (!condition)
> + {
> + printf("%s:%d: error: Value of: %s\n", file, line, valueStr);
> + printf(" Actual: %d\n", condition);
> + printf("Expected: TRUE\n");
> + failed = 1;
> + markFailed();
> + }
> + return failed;
> +}
> +
> +int zucimpl_expect_false(int condition, char const *file, int line, const char *valueStr)
> +{
> + int failed = 0;
> + if (condition)
> + {
> + printf("%s:%d: error: Value of: %s\n", file, line, valueStr);
> + printf(" Actual: %d\n", condition);
> + printf("Expected: FALSE\n");
> + failed = 1;
> + markFailed();
> + }
> + return failed;
> +}
Can we not generate these functions through macros perhaps, to cut
down on duplication a bit? Though I have to say I do really like that
it prints out not only the condition (in C terms), but the actual live
values. That's very useful.
Also, early-return would again save you indentation and code.
> +ZUC_TEST(BaseTest, MathIsSane)
> +{
> + ZUC_EXPECT_EQ(4, 2 + 2);
> +}
> +
> +ZUC_TEST(BaseTest, MathIsHard)
> +{
> +#ifdef ENABLE_FAIL_TESTS
> + ZUC_EXPECT_EQ(5, 2 + 2);
> + ZUC_EXPECT_EQ(7, 9);
> +#endif
> +}
> +
> +ZUC_TEST(BaseTest, MathIsMoreHard)
> +{
> +#ifdef ENABLE_FAIL_TESTS
> + ZUC_ASSERT_EQ(5, 2 + 2);
> + ZUC_ASSERT_EQ(7, 9);
> +#endif
> +}
It's not immediately obvious to me where ENABLE_FAIL_TESTS gets
defined, or what it does.
> +int main( int argc, const char* argv[] )
No padded parentheses please.
Like I said, this isn't a substantive review of the test suite itself,
just a whole host of style issues that will become important as the
code comes to actual review/submission; we keep a consistent style to
make it easier on everyone. Most of these are cosmetic issues, but in
general I do find the lifetime and defensive-program issues very
concerning: I worry that in your quest to plough on and try to recover
no matter what the circumstances, you are creating a whole world of
undefined behaviour which will be rarely hit, but extremely confusing
when it is.
Not only is being unpredictable about the worst thing we can do in a
test suite, but the added complexity makes it extremely difficult to
discern which behaviour is actually desired. How long do the Wayland
objects live? Do they always have a tracking structure associated with
them? Do they get destroyed with the tracking structure, or do they
outlive them, or vice-versa? I'm not really sure from the first pass,
and I find that pretty troubling.
I liked a distinction Havoc Pennington once drew between 'programming
errors' and transient errors, in regards to when you should do
checking. If you're passing in a NULL context or a NULL interface
name, or the tracking structure has has disappeared, then that's a
sign of something seriously troubling that you want to understand and
fix immediately. Trying to paper over the issue, and changing the
behaviour in the process, means we can only be _less_ certain that our
tests are testing what they're supposed to. And that's not good. But
things like allocation failure, etc, are expected to occur, and that's
OK: we can return failure and punt those up the chain.
Cheers,
Daniel
More information about the wayland-devel
mailing list