[1/3,RFC,v2] selftests: introduce testing abstractions

Signed-of-by Alexandru Copot <alex.mihai.c@gmail.com>
Cc: Daniel Baluta <dbaluta@ixiacom.com>
---
 tools/testing/selftests/Makefile        |  3 +-
 tools/testing/selftests/lib/Makefile    | 14 +++++++
 tools/testing/selftests/lib/selftests.c | 57 ++++++++++++++++++++++++++++
 tools/testing/selftests/lib/selftests.h | 67 +++++++++++++++++++++++++++++++++
 4 files changed, 140 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/lib/Makefile
 create mode 100644 tools/testing/selftests/lib/selftests.c
 create mode 100644 tools/testing/selftests/lib/selftests.h

On 04/25/2013 01:04 PM, Alexandru Copot wrote:
> Signed-of-by Alexandru Copot <alex.mihai.c@gmail.com>
> Cc: Daniel Baluta <dbaluta@ixiacom.com>
> ---
>   tools/testing/selftests/Makefile        |  3 +-
>   tools/testing/selftests/lib/Makefile    | 14 +++++++
>   tools/testing/selftests/lib/selftests.c | 57 ++++++++++++++++++++++++++++
>   tools/testing/selftests/lib/selftests.h | 67 +++++++++++++++++++++++++++++++++
>   4 files changed, 140 insertions(+), 1 deletion(-)
>   create mode 100644 tools/testing/selftests/lib/Makefile
>   create mode 100644 tools/testing/selftests/lib/selftests.c
>   create mode 100644 tools/testing/selftests/lib/selftests.h
>
> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> index a480593..e0fccd9 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -1,4 +1,5 @@
> -TARGETS = breakpoints
> +TARGETS = lib
> +TARGETS += breakpoints
>   TARGETS += kcmp
>   TARGETS += mqueue
>   TARGETS += vm
> diff --git a/tools/testing/selftests/lib/Makefile b/tools/testing/selftests/lib/Makefile
> new file mode 100644
> index 0000000..9c81d0c
> --- /dev/null
> +++ b/tools/testing/selftests/lib/Makefile
> @@ -0,0 +1,14 @@
> +
> +CFLAGS = -Wall -O2 -c -g
> +
> +selftests.a: selftests.o
> +	ar qc $@ $^
> +
> +%.c: %.h
> +
> +%.o: %.c
> +	$(CC) $(CFLAGS) -o $@ $^
> +
> +clean:
> +	rm -rf *.o *.a
> +
> diff --git a/tools/testing/selftests/lib/selftests.c b/tools/testing/selftests/lib/selftests.c
> new file mode 100644
> index 0000000..1a65785
> --- /dev/null
> +++ b/tools/testing/selftests/lib/selftests.c
> @@ -0,0 +1,57 @@
> +#include <stdio.h>
> +#include <stdarg.h>
> +
> +#include "selftests.h"
> +
> +test_result_t __assert(int expr, const char *filename, int line, const char *fmt, ...)
> +{

Usually I associate sth named assert() that it does an ungraceful exit. Maybe __test() or __check()?

> +	va_list vl;
> +	const char *m;
> +	char msg[BUFSIZ];
> +
> +	if (expr)
> +		return TEST_PASS;
> +
> +	fprintf(stderr, "\n(%s:%d) ", filename, line);
> +
> +	va_start(vl, fmt);
> +	m = va_arg(vl, char *);
> +	if (!m)
> +		m = fmt;
> +	else
> +		fprintf(stderr, "%s ", fmt);
> +
> +	vsnprintf(msg, sizeof msg, m, vl);

Nitpick: sizeof(msg)

> +	va_end(vl);
> +
> +	fprintf(stderr, "%s\n", msg);

Not sure what exactly you are trying to accomplish here in this function?
Why don't you use vfprintf() and leave this hackery aside?

va_list vl;

va_start(vl, fmt);
fprintf(stderr, "(%s:%d) ", filename, line);
vfprintf(stderr, fmt, vl);
va_end(vl);

> +	return TEST_FAIL;
> +}
> +
> +test_result_t run_all_tests(struct generic_test *test, void *param)
> +{
> +	int i;
> +	char *ptr = test->testcases;
> +	test_result_t rc = TEST_PASS;

Here you don't need to inint rc, since you overwrite immediately.

> +	rc = test->prepare ? test->prepare(param) : 0;

Also replace 0 with whatever you have defined for TEST_PASS or TEST_FAIL,
otherwise misleading.

> +	if (rc == TEST_FAIL)
> +		return rc;
> +
> +	fprintf(stderr, "Testing: %s ", test->name);
> +	for (i = 0; i < test->testcase_count; i++) {
> +

No whitespace here.

> +		rc |= test->run(ptr);
> +		ptr += test->testcase_size;
> +		
> +		if (rc == TEST_FAIL && test->abort_on_fail) {
> +			fprintf(stderr, "Test aborted\n");
> +			break;
> +		}
> +	}
> +
> +	fprintf(stderr, "\t\t%s\n", rc ? "[FAIL]" : "[PASS]");
> +	return rc;
> +}
> +
> diff --git a/tools/testing/selftests/lib/selftests.h b/tools/testing/selftests/lib/selftests.h
> new file mode 100644
> index 0000000..6210199
> --- /dev/null
> +++ b/tools/testing/selftests/lib/selftests.h
> @@ -0,0 +1,67 @@
> +#ifndef SELFTESTS_H
> +#define SELFTESTS_H
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <errno.h>
> +
> +typedef enum test_result {
> +	TEST_PASS = 0,
> +	TEST_FAIL,
> +} test_result_t;

I have no strong opinion here, but do we need to make this as an enum and then do
bit-operations on it?

> +test_result_t __assert(int expr, const char *filename, int line, const char *fmt, ...);

Nitpick:

If would be nice if you move run_all_tests() either here below this one or the other way
around, so that function decls are not distributed over the file, but together.

> +#define abort(cond) do {							\
> +	if (!(cond)) {								\
> +		fprintf(stderr, "%s:%d %s\n", __FILE__, __LINE__, #cond);	\
> +		perror("");							\

perror() could be removed from here and put into fprintf() by using f.e. %s + strerror(errno).

> +		exit(EXIT_FAILURE);						\
> +	}									\
> +} while (0)
> +
> +#define pass_if(expr, label, ret)				\
> +	do {                                                    \
> +		if (expr) {					\
> +			ret = TEST_PASS;			\
> +			goto label;				\
> +		}						\
> +	} while (0)

In cases where you make use of this macro, it's always like ...

<function>
{
	pass_if(<cond>, out, ret);
[...]
out:
	return ret;
}

Not sure if this simplifies or rather obfuscates things. Maybe other people
like it, not sure ...

> +/* @expr:   a boolean expression
> + * @label:  jump here to free resources if it fails
> + * @ret:    a test_result_t variable that will hold the result of the test
> + *          This can be later returned from the test function.
> + */
> +#define check(expr, label, ret, ...)					     \
> +	do {                                                                 \
> +		ret = __assert(expr, __FILE__, __LINE__,       		     \
> +			       "Assertion '" #expr "' failed", __VA_ARGS__); \
> +		if (!(expr)) {						     \
> +			perror("");					     \
> +			goto label;                                          \
> +		}							     \
> +	} while (0)

Ditto.

> +struct generic_test {
> +	const char *name;
> +	void *private;
> +
> +

Why do you have two blank lines here?

> +	void *testcases;

It seems you like having void* pointers, right? This is very unintuitive here (and
weird that you iterate with a char* pointer over it in function run_all_tests())
and at least needs some comments. Otherwise people will have to look into your
example to get to know what you mean.

Also, what is the difference between testcases and private? I have not seen that
you use private anywhere.

> +	int testcase_size;

Nitpick (but that depends if you keep void*): size_t len;

> +	int testcase_count;

Nitpick (ditto): unsigned int num_cases;

> +	/* Ends all tests if one fails */
> +	int abort_on_fail;

bool abort_on_fail;

> +	test_result_t (*prepare)(void *);
> +	test_result_t (*run)(void *);
> +	test_result_t (*cleanup)(void *);
> +};
> +
> +test_result_t run_all_tests(struct generic_test *test, void *param);
> +
> +#endif
> +
> +

Also no blank lines at the end of a file!

Ok, for now I leave the other patches aside, maybe you have some comments to the
review, a new version, or wait a while for others to give feedback as well.

Thanks and cheers,

Daniel
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Apr 25, 2013 at 9:19 PM, Daniel Borkmann <dborkman@redhat.com> wrote:
> Usually I associate sth named assert() that it does an ungraceful exit.
> Maybe __test() or __check()?

This looks like a good idea, will do.

>> +       va_list vl;
>> +       const char *m;
>> +       char msg[BUFSIZ];
>> +
>> +       if (expr)
>> +               return TEST_PASS;
>> +
>> +       fprintf(stderr, "\n(%s:%d) ", filename, line);
>> +
>> +       va_start(vl, fmt);
>> +       m = va_arg(vl, char *);
>> +       if (!m)
>> +               m = fmt;
>> +       else
>> +               fprintf(stderr, "%s ", fmt);
>> +
>> +       vsnprintf(msg, sizeof msg, m, vl);
>
>
> Nitpick: sizeof(msg)

sizeof is actually an operator. I could change it especially to
keep an uniform coding style with the rest of the kernel and tools.

>
>> +       va_end(vl);
>> +
>> +       fprintf(stderr, "%s\n", msg);
>
>
> Not sure what exactly you are trying to accomplish here in this function?
> Why don't you use vfprintf() and leave this hackery aside?
>
> va_list vl;
>
> va_start(vl, fmt);
> fprintf(stderr, "(%s:%d) ", filename, line);
> vfprintf(stderr, fmt, vl);
> va_end(vl);
>

Here I am trying to format both using the fmt string and the one that
the user might write as the next argument. It's not really safe, but
this way we can print whatever data is needed to diagnose a failure.

Example: check(r == 0, out_label, ret, "option %d", 5);

But I think you are right, I will keep it simple for the moment.

>> +typedef enum test_result {
>> +       TEST_PASS = 0,
>> +       TEST_FAIL,
>> +} test_result_t;
>
>
> I have no strong opinion here, but do we need to make this as an enum and
> then do bit-operations on it?

I like enums because of type safety, but I will change these to defines.

>> +
>> +#define pass_if(expr, label, ret)                              \
>> +       do {                                                    \
>> +               if (expr) {                                     \
>> +                       ret = TEST_PASS;                        \
>> +                       goto label;                             \
>> +               }                                               \
>> +       } while (0)
>
>
> In cases where you make use of this macro, it's always like ...
>
> <function>
> {
>         pass_if(<cond>, out, ret);
> [...]
> out:
>         return ret;
> }
>
> Not sure if this simplifies or rather obfuscates things. Maybe other people
> like it, not sure ...

This looks like the cleanest solution to me. We have found some other
implementations
which used weird things like setjmp/longjmp. We are open to any
suggestions on this.

>
>> +struct generic_test {
>> +       const char *name;
>> +       void *private;
>> +
>> +
>
>
> Why do you have two blank lines here?
>

Correct, will remove.

>> +       void *testcases;
>
>
> It seems you like having void* pointers, right? This is very unintuitive
> here (and weird that you iterate with a char* pointer over it in function
> run_all_tests())
> and at least needs some comments. Otherwise people will have to look into
> your example to get to know what you mean.

Having void pointers is simpler because it doesn't force you to write casts. It
is also generic. I iterate with char* because I can't do it with a void*.
Yes, you are right about the comments.

> Also, what is the difference between testcases and private? I have not seen
> that you use private anywhere.

The private field is mostly for future use, will remove it for now until
we find some real use case.

>> +       int testcase_size;
>
>
> Nitpick (but that depends if you keep void*): size_t len;
>
>> +       int testcase_count;
>
>
> Nitpick (ditto): unsigned int num_cases;
>
>
>> +       /* Ends all tests if one fails */
>> +       int abort_on_fail;
>
>
> bool abort_on_fail;
>
> Also no blank lines at the end of a file!

I agree with all of these.

> Ok, for now I leave the other patches aside, maybe you have some comments to
> the review, a new version, or wait a while for others to give feedback as well.
>
> Thanks and cheers,
>
> Daniel

Thanks for being so thorough!

Alex
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html