[RFC/PATCH 0/2] The beginnings of an in-server unit test suite.

[Dan Nicholson]

On Mon, Apr 20, 2009 at 6:09 PM, Peter Hutterer
<peter.hutterer at who-t.net> wrote:
> I looked at the glib testing framework and I think it's good for what we need.
> It provides a simple setup and supports more complicated use-cases (that need
> seperate setup and cleanup).
> It gives XML/HTML output through gtester and gtester-report, which is useful
> for the tinderbox.
>
> Also, as airlied pointed out, glib is likely to be present anyway on a
> developer's machine - an advantage over other test suites.
>
> Here's the updated patch, I'm quite happy with it and I'm rather eager to
> merge this soon. We can fix up the automake pain at a later point in time.
>
> Differences to the previous patch:
> - configure supports --disable-unit-tests
> - glib-based instead of macro hacks
> - one automake warning cleaned up (AM_LDFLAGS was misused)
> - automake cleans temporary files.
>
> The test currently runs on "make check" only. At a later point in time we
> might want to run this on each make, but currently the objcopy recreates the
> library each time, and that drags down compilation time.

Seems pretty nice, but here's just a few comments on the automake/make.

>
> Cheers,
>  Peter
>
> From cfcc187cd879049194e8778cfee896dcdd374895 Mon Sep 17 00:00:00 2001
> From: Peter Hutterer <peter.hutterer at who-t.net>
> Date: Wed, 15 Apr 2009 17:21:08 +1000
> Subject: [PATCH 1/2] Add a test-suite for in-server unit-testing.
>
> This patch adds a test/ directory that contains the setup for a unit-testing
> suite designed for in-server unit-testing. All functions available to the X
> server are available to the test binaries through static linking.
>
> This test suite uses the glib testing framework.
> Do not use glib calls outside of the test/ directory.
>
> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> ---
>  configure.ac     |   15 +++++
>  test/Makefile.am |   52 ++++++++++++++++
>  test/README      |   36 +++++++++++
>  test/xkb.c       |  173 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 276 insertions(+), 0 deletions(-)
>  create mode 100644 test/Makefile.am
>  create mode 100644 test/README
>  create mode 100644 test/xkb.c
>
> diff --git a/configure.ac b/configure.ac
> index ef50627..56847da 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -79,6 +79,9 @@ AC_SYS_LARGEFILE
>  XORG_PROG_RAWCPP
>  AC_PATH_PROG(SED,sed)
>
> +# for the test suite
> +AC_CHECK_TOOL(OBJCOPY, objcopy)
> +
>  dnl Check for dtrace program (needed to build Xserver dtrace probes)
>  dnl Also checks for <sys/sdt.h>, since some Linux distros have an
>  dnl ISDN trace program named dtrace
> @@ -439,6 +442,9 @@ AC_ARG_ENABLE(werror,        AS_HELP_STRING([--enable-werror],
>  AC_ARG_ENABLE(debug,         AS_HELP_STRING([--enable-debug],
>                                  [Enable debugging (default: disabled)]),
>                                [DEBUGGING=$enableval], [DEBUGGING=no])
> +AC_ARG_ENABLE(unit-tests,    AS_HELP_STRING([--enable-unit-tests],
> +                                  [Enable unit-tests (default: enabled)]),
> +                                [UNITTESTS=$enableval], [UNITTESTS=yes])
>  AC_ARG_WITH(int10,           AS_HELP_STRING([--with-int10=BACKEND], [int10 backend: vm86, x86emu or stub]),
>                                [INT10="$withval"],
>                                [INT10="$DEFAULT_INT10"])
> @@ -1141,6 +1147,14 @@ if test "x$DEBUGGING" = xyes; then
>  fi
>  AM_CONDITIONAL(DEBUG, [test "x$DEBUGGING" = xyes])
>
> +if test "x$UNITTESTS" = xyes; then
> +       AC_DEFINE(UNITTESTS, 1, [Enable unit tests])
> +       PKG_CHECK_MODULES([GLIB], [glib-2.0])
> +       AC_SUBST([GLIB_LIBS])
> +       AC_SUBST([GLIB_CFLAGS])
> +fi
> +AM_CONDITIONAL(UNITTESTS, [test "x$UNITTESTS" = xyes])
> +
>  AC_DEFINE(XTEST, 1, [Support XTest extension])
>  AC_DEFINE(XSYNC, 1, [Support XSync extension])
>  AC_DEFINE(XCMISC, 1, [Support XCMisc extension])
> @@ -1976,5 +1990,6 @@ hw/kdrive/fbdev/Makefile
>  hw/kdrive/linux/Makefile
>  hw/kdrive/sdl/Makefile
>  hw/kdrive/src/Makefile
> +test/Makefile
>  xorg-server.pc
>  ])
> diff --git a/test/Makefile.am b/test/Makefile.am
> new file mode 100644
> index 0000000..6abcc92
> --- /dev/null
> +++ b/test/Makefile.am
> @@ -0,0 +1,52 @@
> +if UNITTESTS
> +check_PROGRAMS = xkb
> +check_LTLIBRARIES = libxservertest.la libxservertest_t.la
> +
> +TESTS=$(check_PROGRAMS)
> +
> +# This makefile is not happy if you have concurrent makes going on.
> +AM_MAKEFLAGS = -j1
> +
> +AM_CFLAGS = $(DIX_CFLAGS) $(GLIB_CFLAGS) @XORG_CFLAGS@
> +INCLUDES = @XORG_INCS@
> +TEST_LDFLAGS=.libs/libxservertest_t.a $(XORG_SYS_LIBS) $(XSERVER_SYS_LIBS) $(GLIB_LIBS)
> +
> +xkb_LDFLAGS=$(TEST_LDFLAGS)

It might not ever matter, but this breaks how arguments are ordered
for libtool. For programs, you want to use _LDADD to add libraries.

> +
> +
> +libxservertest_la_LIBADD = \
> +            $(XSERVER_LIBS) \
> +            $(top_builddir)/hw/xfree86/loader/libloader.la \
> +            $(top_builddir)/hw/xfree86/os-support/libxorgos.la \
> +            $(top_builddir)/hw/xfree86/common/libcommon.la \
> +            $(top_builddir)/hw/xfree86/parser/libxf86config.la \
> +            $(top_builddir)/hw/xfree86/dixmods/libdixmods.la \
> +            $(top_builddir)/hw/xfree86/modes/libxf86modes.la \
> +            $(top_builddir)/hw/xfree86/ramdac/libramdac.la \
> +            $(top_builddir)/hw/xfree86/ddc/libddc.la \
> +            $(top_builddir)/hw/xfree86/i2c/libi2c.la \
> +            $(top_builddir)/hw/xfree86/dixmods/libxorgxkb.la \
> +            $(top_builddir)/hw/xfree86/libxorg.la \
> +            $(top_builddir)/mi/libmi.la \
> +            $(top_builddir)/os/libos.la \
> +            @XORG_LIBS@
> +
> +CLEANFILES=libxservertest_t.c libxservertest.c
> +
> +libxservertest_t.c:
> +       touch $@
> +
> +libxservertest.c:
> +       touch $@
> +
> +# libxservertest.la defines main, so we need to get rid of it somehow.
> +# we do this by copying libxservertest.a into libxservertest_t.a and removing the
> +# symbol during the copy process.
> +libxservertest_t.la: libxservertest.la
> +       $(OBJCOPY) --strip-symbol=main .libs/libxservertest.a .libs/libxservertest_t.a

If this is how you want to do it, don't define libxservertest_t as a
libtool library (check_LTLIBRARIES). It just confuses the tools. Just
make the target libxservertest.a and then the file make is looking for
(libxserverttest_t.a) will actually be there. Since you're defining a
custom rule for generating the .a, there's no benefit to defining it
as libtool library. It also means you don't have to create
libxservertest_t.c.

On the other hand, it's pretty gross. :) The two alternatives that come to mind:

1. Don't actually define main in dix. Instead have it called dixmain
or something so that any DDX (or test program) can decide to override
it or not. I think you already mentioned this.

2. Try to get libtool to not export main from libxservert.la. I
haven't tried this, but I believe you could do:

libxservertest_la_LDFLAGS = -export-symbols-regex '...'

Unfortunately, my regex foo does not match my autotools foo, so I
don't know a regex that says "everything except main".

> +
> +
> +all:
> +       echo "Run 'make check' to run the test suite"

@echo ... to suppress the command being printed.

> +
> +endif
> diff --git a/test/README b/test/README
> new file mode 100644
> index 0000000..5759a72
> --- /dev/null
> +++ b/test/README
> @@ -0,0 +1,36 @@
> +                        X server test suite
> +
> +This suite contains a set of tests to verify the behaviour of functions used
> +internally to the server. This test suite is based on glib's testing
> +framework [1].
> +
> += How it works =
> +Through some automake abuse, we link the test programs with the same static
> +libraries as the Xorg binary. The test suites can then call various functions
> +and verify their behaviour - without the need to start the server or connect
> +clients.
> +
> +This testing only works for functions that do not rely on a particular state
> +of the X server. Unless the test suite replicates the expected state, which
> +may be difficult.
> +
> += How to run the tests =
> +Run "make check" the test directory. This will compile the tests and execute
> +them in the order specified in the TESTS variable in test/Makefile.am.
> +
> +Each set of tests related to a subsystem are available as a binary that can be
> +executed directly. For example, run "xkb" to perform some xkb-related tests.
> +
> +== Adding a new test ==
> +When adding a new test, ensure that you add a short description of what the
> +test does and what the expected outcome is. If the test reproduces a
> +particular bug, using g_test_bug().
> +
> +== Misc ==
> +
> +The programs "gtester" and "gtester-report" may be used to generate XML/HTML
> +log files of tests succeeded and failed.
> +
> +---------
> +
> +[1] http://library.gnome.org/devel/glib/stable/glib-Testing.html
> diff --git a/test/xkb.c b/test/xkb.c
> new file mode 100644
> index 0000000..6fbb26a
> --- /dev/null
> +++ b/test/xkb.c
> @@ -0,0 +1,173 @@
> +/**
> + * Copyright © 2009 Red Hat, Inc.
> + *
> + *  Permission is hereby granted, free of charge, to any person obtaining a
> + *  copy of this software and associated documentation files (the "Software"),
> + *  to deal in the Software without restriction, including without limitation
> + *  the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + *  and/or sell copies of the Software, and to permit persons to whom the
> + *  Software is furnished to do so, subject to the following conditions:
> + *
> + *  The above copyright notice and this permission notice (including the next
> + *  paragraph) shall be included in all copies or substantial portions of the
> + *  Software.
> + *
> + *  THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + *  IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + *  FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + *  THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + *  LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + *  FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + *  DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifdef HAVE_DIX_CONFIG_H
> +#include <dix-config.h>
> +#endif
> +
> +#include <xkb-config.h>
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <ctype.h>
> +#include <unistd.h>
> +#include <math.h>
> +#include <X11/X.h>
> +#include <X11/Xproto.h>
> +#include <X11/keysym.h>
> +#include <X11/Xatom.h>
> +#include "misc.h"
> +#include "inputstr.h"
> +#include "opaque.h"
> +#include "property.h"
> +#define        XKBSRV_NEED_FILE_FUNCS
> +#include <xkbsrv.h>
> +#include "../xkb/xkbgeom.h"
> +#include <X11/extensions/XKMformat.h>
> +#include "xkbfile.h"
> +#include "../xkb/xkb.h"
> +
> +#include <glib.h>
> +
> +/**
> + * Initialize an empty XkbRMLVOSet.
> + * Call XkbGetRulesDflts to obtain the default ruleset.
> + * Compare obtained ruleset with the built-in defaults.
> + *
> + * Result: RMLVO defaults are the same as obtained.
> + */
> +static void xkb_get_rules_test(void)
> +{
> +    XkbRMLVOSet rmlvo = { NULL};
> +    XkbGetRulesDflts(&rmlvo);
> +
> +
> +    g_assert(rmlvo.rules);
> +    g_assert(rmlvo.model);
> +    g_assert(rmlvo.layout);
> +    g_assert(rmlvo.variant);
> +    g_assert(rmlvo.options);
> +    g_assert(strcmp(rmlvo.rules, XKB_DFLT_RULES) == 0);
> +    g_assert(strcmp(rmlvo.model, XKB_DFLT_MODEL) == 0);
> +    g_assert(strcmp(rmlvo.layout, XKB_DFLT_LAYOUT) == 0);
> +    g_assert(strcmp(rmlvo.variant, XKB_DFLT_VARIANT) == 0);
> +    g_assert(strcmp(rmlvo.options, XKB_DFLT_OPTIONS) == 0);
> +}
> +
> +/**
> + * Initialize an random XkbRMLVOSet.
> + * Call XkbGetRulesDflts to obtain the default ruleset.
> + * Compare obtained ruleset with the built-in defaults.
> + * Result: RMLVO defaults are the same as obtained.
> + */
> +static void xkb_set_rules_test(void)
> +{
> +    XkbRMLVOSet rmlvo = {
> +        .rules = "test-rules",
> +        .model = "test-model",
> +        .layout = "test-layout",
> +        .variant = "test-variant",
> +        .options = "test-options"
> +    };
> +    XkbRMLVOSet rmlvo_new = { NULL };
> +
> +    XkbSetRulesDflts(&rmlvo);
> +    XkbGetRulesDflts(&rmlvo_new);
> +
> +    /* XkbGetRulesDflts strdups the values */
> +    g_assert(rmlvo.rules != rmlvo_new.rules);
> +    g_assert(rmlvo.model != rmlvo_new.model);
> +    g_assert(rmlvo.layout != rmlvo_new.layout);
> +    g_assert(rmlvo.variant != rmlvo_new.variant);
> +    g_assert(rmlvo.options != rmlvo_new.options);
> +
> +    g_assert(strcmp(rmlvo.rules, rmlvo_new.rules) == 0);
> +    g_assert(strcmp(rmlvo.model, rmlvo_new.model) == 0);
> +    g_assert(strcmp(rmlvo.layout, rmlvo_new.layout) == 0);
> +    g_assert(strcmp(rmlvo.variant, rmlvo_new.variant) == 0);
> +    g_assert(strcmp(rmlvo.options, rmlvo_new.options) == 0);
> +}
> +
> +
> +/**
> + * Get the default RMLVO set.
> + * Set the default RMLVO set.
> + * Get the default RMLVO set.
> + * Repeat the last two steps.
> + *
> + * Result: RMLVO set obtained is the same as previously set.
> + */
> +static void xkb_set_get_rules_test(void)
> +{
> +/* This test failed before XkbGetRulesDftlts changed to strdup.
> +   We test this twice because the first time using XkbGetRulesDflts we obtain
> +   the built-in defaults. The unexpected free isn't triggered until the second
> +   XkbSetRulesDefaults.
> + */
> +    XkbRMLVOSet rmlvo = { NULL };
> +    XkbRMLVOSet rmlvo_backup;
> +
> +    XkbGetRulesDflts(&rmlvo);
> +
> +    /* pass 1 */
> +    XkbSetRulesDflts(&rmlvo);
> +    XkbGetRulesDflts(&rmlvo);
> +
> +    /* Make a backup copy */
> +    rmlvo_backup.rules = strdup(rmlvo.rules);
> +    rmlvo_backup.layout = strdup(rmlvo.layout);
> +    rmlvo_backup.model = strdup(rmlvo.model);
> +    rmlvo_backup.variant = strdup(rmlvo.variant);
> +    rmlvo_backup.options = strdup(rmlvo.options);
> +
> +    /* pass 2 */
> +    XkbSetRulesDflts(&rmlvo);
> +
> +    /* This test is iffy, because strictly we may be comparing against already
> +     * freed memory */
> +    g_assert(strcmp(rmlvo.rules, rmlvo_backup.rules) == 0);
> +    g_assert(strcmp(rmlvo.model, rmlvo_backup.model) == 0);
> +    g_assert(strcmp(rmlvo.layout, rmlvo_backup.layout) == 0);
> +    g_assert(strcmp(rmlvo.variant, rmlvo_backup.variant) == 0);
> +    g_assert(strcmp(rmlvo.options, rmlvo_backup.options) == 0);
> +
> +    XkbGetRulesDflts(&rmlvo);
> +    g_assert(strcmp(rmlvo.rules, rmlvo_backup.rules) == 0);
> +    g_assert(strcmp(rmlvo.model, rmlvo_backup.model) == 0);
> +    g_assert(strcmp(rmlvo.layout, rmlvo_backup.layout) == 0);
> +    g_assert(strcmp(rmlvo.variant, rmlvo_backup.variant) == 0);
> +    g_assert(strcmp(rmlvo.options, rmlvo_backup.options) == 0);
> +}
> +
> +
> +int main(int argc, char** argv)
> +{
> +    g_test_init(&argc, &argv,NULL);
> +    g_test_bug_base("https://bugzilla.freedesktop.org/show_bug.cgi?id=");
> +
> +    g_test_add_func("/xkb/set-get-rules", xkb_set_get_rules_test);
> +    g_test_add_func("/xkb/get-rules", xkb_get_rules_test);
> +    g_test_add_func("/xkb/set-rules", xkb_set_rules_test);
> +
> +    return g_test_run();
> +}
> --
> 1.6.2.2.447.g4afa7
>
> _______________________________________________
> xorg-devel mailing list
> xorg-devel at lists.x.org
> http://lists.x.org/mailman/listinfo/xorg-devel
>