Merge lp:~christof-mroz/hipl/mock-functions into lp:hipl

[Christof Mroz]

A framework for mock functions (library function overrides) in unit tests.
Refer to the documentation in test/mocks.c for motivation and usage details.

The hipfw-timeout and hipfw-esp-speedup branches depend on this.

Lectures start again next week... if there are objections, please consider branching and fixing them yourself: I will be available for review but probably not for coding and testing for quite some time.

[Miika Komu (miika-iki)]

Is the word "mock" or "mock up" function?

Just by reading the file I could not get the big picture behind it. Is the idea to overload some functions during the runtime of hipfw and force it to execute some unit tests?

review: Needs Information

[René Hummen (rene-hummen)]

review needs-fixing

On 04.04.2011, at 00:12, Christof Mroz wrote:
> Christof Mroz has proposed merging lp:~christof-mroz/hipl/mock-functions into lp:hipl.
>
> Requested reviews:
> HIPL core team (hipl-core)
>
> For more details, see:
> https://code.launchpad.net/~christof-mroz/hipl/mock-functions/+merge/56090
>
> A framework for mock functions (library function overrides) in unit tests.
> Refer to the documentation in test/mocks.c for motivation and usage details.
>
> The hipfw-timeout and hipfw-esp-speedup branches depend on this.
>
> Lectures start again next week... if there are objections, please consider branching and fixing them yourself: I will be available for review but probably not for coding and testing for quite some time.
> --
> https://code.launchpad.net/~christof-mroz/hipl/mock-functions/+merge/56090
> Your team HIPL core team is requested to review the proposed merge of lp:~christof-mroz/hipl/mock-functions into lp:hipl.
> === modified file 'Makefile.am'
> --- Makefile.am 2011-03-25 17:51:40 +0000
> +++ Makefile.am 2011-04-03 22:12:27 +0000
> @@ -23,7 +23,7 @@
>
> ACLOCAL_AMFLAGS = -I m4
>
> -HIPL_HEADER_LOCATIONS = firewall/*.h hipd/*.h lib/*/*.h modules/*/*/*.h test/*/*.h test/*/*/*.h
> +HIPL_HEADER_LOCATIONS = firewall/*.h hipd/*.h lib/*/*.h modules/*/*/*.h test/*/*.h test/*/*/*.h test/mocks.h

Use test/*.h to prepare for future headers.

> +
> +/*** time(2) ***/
> +

Why do you need to overload time()? Contrary to system(), this should not have side-effects when executed from a unit-test.

> +bool mock_time = false; /**< time(2) mock enabled? */
> +time_t mock_time_next; /**< value returned on next invocation of time(2) mock */
> +
> +/**
> + * time(2) mock function. Controlled by the ::mock_time flag.
> + * Returns the preset value ::mock_time_next, if enabled.
> + *
> + * @param t If non-NULL, the return value is also stored in the memory pointed
> + * to by @a t.
> + * @return The current value of ::mock_time_next.
> + */
> +time_t time(time_t *t) {
> + if (!mock_time) {
> + time_t (*original)(time_t*) = get_original(time, "time");
> + return original(t);
> + }
> +
> +
> + if (t) {
> + *t = mock_time_next;
> + }
> +
> + return mock_time_next;
> +}
> +
> +/*** system(3) ***/
> +
> +bool mock_system = false;
> +char *mock_system_last = NULL;
> +int mock_system_exit = EXIT_SUCCESS;
> +
> +/**
> + * system(3) mock function. Controlled by the ::mock_system flag.
> + * Stores a copy of @a command in ::mock_system_last, if enabled.

mock_system_exit seems to be more interesting to talk about here.

> + * @param command A copy of this string will be stored in ::mock_system_last of
> + * non-NULL. Otherwise, ::mock_system_last is set to NULL.
> + * @return The value of ::mock_system_exit if @a command was non-NULL, -1
> + * otherwise.
> + */

The usage is unclear to me. Why do I need mock_system_last?

> === added file 'test/mocks.o'
> Binary files test/mocks.o 1970-01-01 00:00:00 +0000 and test/mocks.o 2011-04-03 22:12:27 +0000 differ

I'm quite sure, you don't want to add a binary to the repository :)

René

[Diego Biurrun (diego-biurrun)]

 review abstain

On Sun, Apr 03, 2011 at 10:12:32PM +0000, Christof Mroz wrote:
>
> --- Makefile.am 2011-03-25 17:51:40 +0000
> +++ Makefile.am 2011-04-03 22:12:27 +0000
> @@ -23,7 +23,7 @@
>
> -HIPL_HEADER_LOCATIONS = firewall/*.h hipd/*.h lib/*/*.h modules/*/*/*.h test/*/*.h test/*/*/*.h
> +HIPL_HEADER_LOCATIONS = firewall/*.h hipd/*.h lib/*/*.h modules/*/*/*.h test/*/*.h test/*/*/*.h test/mocks.h
> HIPL_HEADER_LIST = $(wildcard $(addprefix $(srcdir)/,$(HIPL_HEADER_LOCATIONS)))

As Rene noted, just test/*.h will be more future-proof.

> --- test/mocks.c 1970-01-01 00:00:00 +0000
> +++ test/mocks.c 2011-04-03 22:12:27 +0000
> @@ -0,0 +1,188 @@
> + * This also implies that each other suite can not define an own implementation

cannot, its own

> + * Remember that each test runs in a process of its own, so the flags must be
> + * set at the beginning of each.

each what? process or test?

In general I

> +// RTLD_NEXT is optional in POSIX, and unit tests relying on mock functions will
> +// be silently ignored on systems that don't support it. See get_original().
> +//
> +// Some libc implementations (e.g. uclibc) export RTLD_NEXT by default, if
> +// supported. glibc considers this a GNU extension, so we pass _GNU_SOURCE
> +// (which should not affect non-glibc systems).
> +//
> +#define _GNU_SOURCE

http://pubs.opengroup.org/onlinepubs/9699919799/functions/dlsym.html

tells me that RTLD_NEXT is reserved for future use

nit: We generally use /* */ for multiline comments.

> +#include "test/mocks.h"

Just mocks.h should be enough for headers in the same directory.

> + * @param name Symbolic (i.e. exported) name, of the original function. It will

s/,//

> +/*** time(2) ***/
> +
> +time_t time(time_t *t) {

K&R places the { on the next line for function declarations.

> + if (!mock_time) {
> + time_t (*original)(time_t*) = get_original(time, "time");
> + return original(t);
> + }
> +
> +
> + if (t) {
> + *t = mock_time_next;
> + }

nit: two emply lines seem excessive

> +int system(const char *command) {

see above

> --- test/mocks.h 1970-01-01 00:00:00 +0000
> +++ test/mocks.h 2011-04-03 22:12:27 +0000
> @@ -0,0 +1,40 @@
> +/*
> + * Copyright (c) 2010 Aalto University and RWTH Aachen University.

Happy new year to you :)

...same for the other file...

> +#ifndef HIP_TEST_MOCKS_H
> +#define HIP_TEST_MOCKS_H
> +
> +#endif /* HIP_TEST_FIREWALL_TEST_SUITES_H */

The comment is wrongly copy + pasted.

> === added file 'test/mocks.o'
> Binary files test/mocks.o 1970-01-01 00:00:00 +0000 and test/mocks.o 2011-04-03 22:12:27 +0000 differ

We don't want that one.

Diego

review: Abstain

[Christof Mroz (christof-mroz)]

> Is the word "mock" or "mock up" function?

AFAIK it's the usual unit test terminology, see e.g.
http://www.lastcraft.com/mock_callbacks.php

> Just by reading the file I could not get the big picture behind it. Is the
> idea to overload some functions during the runtime of hipfw and force it to
> execute some unit tests?

The overloading applies to unit tests only; test/mocks.c is not built for hipd, hipfw etc. at all, just for the check programs.

No unit test in this branch relies on the feature yes, but some newly added tests in hipfw-timeout and hipfw-esp-speedup do... please take a look at them if still in doubt.

[Christof Mroz (christof-mroz)]

> > === modified file 'Makefile.am'
> > --- Makefile.am 2011-03-25 17:51:40 +0000
> > +++ Makefile.am 2011-04-03 22:12:27 +0000
> > @@ -23,7 +23,7 @@
> >
> > ACLOCAL_AMFLAGS = -I m4
> >
> > -HIPL_HEADER_LOCATIONS = firewall/*.h hipd/*.h lib/*/*.h modules/*/*/*.h
> test/*/*.h test/*/*/*.h
> > +HIPL_HEADER_LOCATIONS = firewall/*.h hipd/*.h lib/*/*.h modules/*/*/*.h
> test/*/*.h test/*/*/*.h test/mocks.h
>
> Use test/*.h to prepare for future headers.

OK (my idea was to prevent accidental inclusion of a stray header: test/ is currently not supposed to contain other headers than mocks.h)

> > +
> > +/*** time(2) ***/
> > +
>
> Why do you need to overload time()? Contrary to system(), this should not have
> side-effects when executed from a unit-test.

To simulate clock skews, for example (which are handled by periodic cleanup).

> > +/*** system(3) ***/
> > +
> > +bool mock_system = false;
> > +char *mock_system_last = NULL;
> > +int mock_system_exit = EXIT_SUCCESS;
> > +
> > +/**
> > + * system(3) mock function. Controlled by the ::mock_system flag.
> > + * Stores a copy of @a command in ::mock_system_last, if enabled.
>
> mock_system_exit seems to be more interesting to talk about here.

Oops, forgot to document the globals...

> > + * @param command A copy of this string will be stored in
> ::mock_system_last of
> > + * non-NULL. Otherwise, ::mock_system_last is set to NULL.
> > + * @return The value of ::mock_system_exit if @a command was non-
> NULL, -1
> > + * otherwise.
> > + */
>
> The usage is unclear to me. Why do I need mock_system_last?

See the hipfw-esp-speedup branch for an example: There, we expect a certain command line to be built, which can be checked by comparing the last command executed.

[Christof Mroz (christof-mroz)]

> > +// RTLD_NEXT is optional in POSIX, and unit tests relying on mock functions
> will
> > +// be silently ignored on systems that don't support it. See
> get_original().
> > +//
> > +// Some libc implementations (e.g. uclibc) export RTLD_NEXT by default, if
> > +// supported. glibc considers this a GNU extension, so we pass _GNU_SOURCE
> > +// (which should not affect non-glibc systems).
> > +//
> > +#define _GNU_SOURCE
>
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/dlsym.html
>
> tells me that RTLD_NEXT is reserved for future use

OK, I'll be more specific... just trying to take the scariness out of RTLD_NEXT, because it actually seems to be widely supported.

> > --- test/mocks.h 1970-01-01 00:00:00 +0000
> > +++ test/mocks.h 2011-04-03 22:12:27 +0000
> > @@ -0,0 +1,40 @@
> > +/*
> > + * Copyright (c) 2010 Aalto University and RWTH Aachen University.
>
> Happy new year to you :)

Heh, good catch.

[lp:~christof-mroz/hipl/mock-functions]

5818. By Christof Mroz on 2011-04-04

Add test/*.h to header locations, rather than just test/mocks.h.

5819. By Christof Mroz on 2011-04-04

Don't version test/mocks.c object file.

5820. By Christof Mroz on 2011-04-04

Files were created in 2011; update license boilerplate accordingly.

5821. By Christof Mroz on 2011-04-04

Documentation fixes and clarifications.

5822. By Christof Mroz on 2011-04-04

Use relative path for header file in same directory.

5823. By Christof Mroz on 2011-04-04

Uncrustified test/mocks.c.

5824. By Christof Mroz on 2011-04-04

Fix header file guard comment.

5825. By Christof Mroz on 2011-04-04

Merged trunk revision 5824.

5826. By Christof Mroz on 2011-04-04

Clarify that mock functions apply to unit testing only.

5827. By Christof Mroz on 2011-04-04

Documentation fixes in test/mocks.c.

Unmerged revisions

=== modified file 'Makefile.am'
--- Makefile.am	2011-03-25 17:51:40 +0000
+++ Makefile.am	2011-04-04 15:57:28 +0000
@@ -23,7 +23,7 @@
 
 ACLOCAL_AMFLAGS  = -I m4
 
-HIPL_HEADER_LOCATIONS = firewall/*.h hipd/*.h lib/*/*.h modules/*/*/*.h test/*/*.h test/*/*/*.h
+HIPL_HEADER_LOCATIONS = firewall/*.h hipd/*.h lib/*/*.h modules/*/*/*.h test/*/*.h test/*/*/*.h test/*.h
 HIPL_HEADER_LIST = $(wildcard $(addprefix $(srcdir)/,$(HIPL_HEADER_LOCATIONS)))
 
 # For "make dist"

=== added file 'test/mocks.c'
--- test/mocks.c	1970-01-01 00:00:00 +0000
+++ test/mocks.c	2011-04-04 15:57:28 +0000
@@ -0,0 +1,196 @@
+/*
+ * Copyright (c) 2011 Aalto University and RWTH Aachen University.
+ *
+ * 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 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.
+ */
+
+/**
+ * @file
+ * @brief Mock functions for unit tests.
+ *
+ * Because unit tests should be self-contained and predictable, testing
+ * functions that rely on external state can be tricky. Sometimes, the easiest
+ * solution lies in overloading certain library functions with so-called
+ * <i>mock functions</i> that simulate those calls and produce user-supplied
+ * fake output for the duration of the test. Nothing is overridden outside of
+ * unit tests, of course.
+ *
+ * <h2>Short tutorial</h2>
+ * - In Makefile.am
+ *   - add test/mock.c to your check program's _SOURCE,
+ *   - add -ldl to your check program's _LDFLAGS.
+ * - Copy & Paste one of the implementations below.
+ *
+ *
+ * <h2>Background</h2>
+ *
+ * Mock functions in HIPL are implemented using the fact that the linker tries
+ * to satisfy declarations by using statically defined symbols first, before
+ * resorting to dynamic libraries.
+ * In other words: providing an implementation with the exact same prototype as
+ * the library function is all you need, first of all.
+ *
+ * Mocks can be defined in the file they're needed in, but beware:
+ * The overriding implementation's definition cannot be static: a compiler error
+ * will be emitted. On the other hand, a non-static definition will be picked up
+ * by other object files in the same test suite, i.e. they will invariably use
+ * your mock function which may not be intended.
+ * This also implies that each other suite cannot define its own implementation
+ * due to "duplicate symbol" errors.
+ *
+ * For this reason, and to encourage modularity, mock functions should be
+ * defined in this file. They should be opt-in by enabling a global boolean
+ * flag, in order not to disrupt other unit tests.
+ * Remember that each test runs in a process of its own, so the flags must be
+ * set at the beginning of each test.
+ *
+ * @note The documentation implies that argments of disabled mock functions are
+ *       passed to the original implementation.
+ *
+ * @author Christof Mroz <[e-mail]>
+ */
+
+/*
+ * RTLD_NEXT is just "reserved for future use" in POSIX but not mandatory, so it
+ * may not be available.
+ * Unit tests relying on mock functions will be silently ignored on systems that
+ * don't support it. See get_original().
+ *
+ * Some libc implementations (e.g. uclibc) export RTLD_NEXT by default, if
+ * supported. glibc considers this a GNU extension, so we pass _GNU_SOURCE
+ * (which should not affect non-glibc systems).
+ */
+#define _GNU_SOURCE
+
+#include <check.h>
+#include <dlfcn.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <time.h>
+
+#include "mocks.h"
+
+
+// A NULL argument to dlsym() is allowed as per POSIX, and may do the trick,
+// even though the chances are slim. We try it anyway.
+#ifndef RTLD_NEXT
+#define RTLD_NEXT NULL
+#endif
+
+/**
+ * Retrieve a pointer to the real implementation of the library function
+ * named by symbol @a name.
+ * Exits with status 0 if it cannot be retrieved.
+ *
+ * @param mock A pointer to the mock function. This is used to guard against
+ *             returning a pointer to the mock rather than the real function
+ *             instead, which would in turn be called by the mock itself and so
+ *             on, thereby triggering infinite recursion.
+ * @param name Symbolic (i.e. exported) name,of the original function. It will
+ *             be searched in default shared object load order and the first
+ *             occurence retrieved.
+ * @return     Function pointer to import symbol @a name.
+ *             Does not return on failure.
+ */
+static void *get_original(const void *const mock, const char *const name)
+{
+    void *ret = dlsym(RTLD_NEXT, name);
+
+    fail_if(!ret, "dlsym(\"%s\"): %s\n", name, dlerror());
+    fail_if(!mock);
+
+    // Avoid infinite recursion. This can happen if RTLD_NEXT was aliased to
+    // NULL above.
+    if (ret == mock) {
+        fprintf(stderr,
+                "Skipping check: function chaining not supported by lib\n");
+        exit(EXIT_SUCCESS);
+    }
+
+    return ret;
+}
+
+/*** time(2) ***/
+
+bool   mock_time = false; /**< time(2) mock enabled? */
+time_t mock_time_next;    /**< value returned on next invocation of time(2) mock */
+
+/**
+ * time(2) mock function. Controlled by the ::mock_time flag.
+ * Returns the preset value ::mock_time_next, if enabled.
+ *
+ * @param t If non-NULL, the return value is also stored in the memory pointed
+ *          to by @a t.
+ * @return  The current value of ::mock_time_next.
+ */
+time_t time(time_t *t)
+{
+    if (!mock_time) {
+        time_t (*original)(time_t *) = get_original(time, "time");
+        return original(t);
+    }
+
+    if (t) {
+        *t = mock_time_next;
+    }
+
+    return mock_time_next;
+}
+
+/*** system(3) ***/
+
+bool  mock_system      = false;        /**< system(3) mock enabled? */
+char *mock_system_last = NULL;         /**< copy of last argument passed to
+                                        *   system(3) mock, or NULL if mock was
+                                        *   not called yet. */
+int   mock_system_exit = EXIT_SUCCESS; /**< value that will be returned by
+                                        *   system(3) mock if a non-NULL
+                                        *   argument is supplied to it.*/
+
+/**
+ * system(3) mock function. Controlled by the ::mock_system flag.
+ * Stores a copy of @a command in ::mock_system_last, if enabled.
+ *
+ * @param command A copy of this string will be stored in ::mock_system_last of
+ *                non-NULL. Otherwise, ::mock_system_last is set to NULL.
+ * @return        The value of ::mock_system_exit if @a command was non-NULL, -1
+ *                otherwise.
+ */
+int system(const char *command)
+{
+    if (!mock_system) {
+        int (*original)(const char *) = get_original(system, "system");
+        return original(command);
+    }
+
+    free(mock_system_last);
+    if (command) {
+        mock_system_last = strdup(command);
+    } else {
+        mock_system_last = NULL;
+        return -1;
+    }
+
+    return mock_system_exit;
+}

=== added file 'test/mocks.h'
--- test/mocks.h	1970-01-01 00:00:00 +0000
+++ test/mocks.h	2011-04-04 15:57:28 +0000
@@ -0,0 +1,40 @@
+/*
+ * Copyright (c) 2011 Aalto University and RWTH Aachen University.
+ *
+ * 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 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.
+ */
+
+#ifndef HIP_TEST_MOCKS_H
+#define HIP_TEST_MOCKS_H
+
+#include <stdbool.h>
+#include <time.h>
+
+
+extern bool   mock_time;
+extern time_t mock_time_next;
+
+extern bool  mock_system;
+extern char *mock_system_last;
+extern int   mock_system_exit;
+
+#endif /* HIP_TEST_MOCKS_H */