Re: [PATCH 1/2, libnftnl] tests: Consolidate printing error utilities

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Thu, Aug 11, 2016 at 03:25:06PM +0200, Carlos Falgueras García wrote:
> Created libtest.[hc] in order to consolidate code that is repeated in all
> tests.

Please, use present tense, eg.

This patch adds libtest.c and libtest.h to reduce test code and
consolidate it.

> diff --git a/tests/libtest.c b/tests/libtest.c
> new file mode 100644
> index 0000000..91f2d5e
> --- /dev/null
> +++ b/tests/libtest.c
> @@ -0,0 +1,49 @@
> +#include <libtest.h>
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <stdarg.h>
> +#include <string.h>
> +#include <errno.h>
> +#include <stdbool.h>
> +
> +#define COLOR_RED     "\x1b[31m"
> +#define COLOR_GREEN   "\x1b[32m"
> +#define COLOR_RESET   "\x1b[0m"
> +
> +static bool test_ok = true;

I don't like this internal test_ok.

> +void __oom(const char *prog, const char *file, int line)
> +{
> +	fprintf(stderr,
> +		COLOR_RED "OOM" COLOR_RESET " at %s:%d\n\t%s\n", file, line,
> +		strerror(errno));
> +
> +	test_ok = false;
> +	test_exit(prog);
> +	exit(EXIT_FAILURE);
> +}
> +
> +void print_err(const char *fmt, ...)
> +{
> +	va_list args;
> +
> +	fprintf(stderr, COLOR_RED "ERROR: " COLOR_RESET);
> +	va_start(args, fmt);
> +	vfprintf(stderr, fmt, args);
> +	va_end(args);
> +	fprintf(stderr, "\n");
> +
> +	test_ok = false;
> +}
> +
> +int test_exit(const char *prog)
> +{
> +	if (test_ok) {
> +		printf("%s: " COLOR_GREEN "OK\n" COLOR_RESET, prog);
> +		return EXIT_SUCCESS;
> +	} else {
> +		printf("%s: " COLOR_RED "FAIL\n" COLOR_RESET, prog);
> +		return EXIT_FAILURE;
> +	}

Looks better like this?

	switch (test_ok) {
        case true:
		printf("%s: " COLOR_GREEN "OK\n" COLOR_RESET, prog);
		return EXIT_SUCCESS;
        case false:
        	printf("%s: " COLOR_RED "FAIL\n" COLOR_RESET, prog);
               	return EXIT_FAILURE;
        }

> +}
> diff --git a/tests/libtest.h b/tests/libtest.h
> new file mode 100644
> index 0000000..810bd82
> --- /dev/null
> +++ b/tests/libtest.h
> @@ -0,0 +1,9 @@
> +#ifndef _TESTS_UTILS_H
> +#define _TESTS_UTILS_H
> +
> +#define oom(prog) __oom(prog, __FILE__, __LINE__)
> +void __oom(const char *prog, const char *file, int line);
> +void print_err(const char *fmt, ...);
> +int test_exit(const char *prog);
> +
> +#endif
> diff --git a/tests/nft-chain-test.c b/tests/nft-chain-test.c
> index d678d46..82431c2 100644
> --- a/tests/nft-chain-test.c
> +++ b/tests/nft-chain-test.c
> @@ -15,13 +15,7 @@
>  #include <linux/netfilter/nf_tables.h>
>  #include <libnftnl/chain.h>
>  
> -static int test_ok = 1;
> -
> -static void print_err(const char *msg)
> -{
> -	test_ok = 0;
> -	printf("\033[31mERROR:\e[0m %s\n", msg);
> -}
> +#include <libtest.h>

Please, use:

#include "libtest.h" instead, I prefer it so we know this is locally
declare it.

>  static void cmp_nftnl_chain(struct nftnl_chain *a, struct nftnl_chain *b)
>  {
> @@ -73,7 +67,7 @@ int main(int argc, char *argv[])
>  	a = nftnl_chain_alloc();
>  	b = nftnl_chain_alloc();
>  	if (a == NULL || b == NULL)
> -		print_err("OOM");
> +		oom(argv[0]);

You can replace this above by:

        test_assert(a != NULL);
        test_assert(b != NULL);

>  	nftnl_chain_set_str(a, NFTNL_CHAIN_NAME, "test");
>  	nftnl_chain_set_u32(a, NFTNL_CHAIN_FAMILY, AF_INET);
> @@ -101,10 +95,6 @@ int main(int argc, char *argv[])
>  	nftnl_chain_free(a);
>  	nftnl_chain_free(b);
>  
> -	if (!test_ok)
> -		exit(EXIT_FAILURE);
> -
> -	printf("%s: \033[32mOK\e[0m\n", argv[0]);
> -	return EXIT_SUCCESS;
> +	return test_exit(argv[0]);

Better name, instead of test_exit:

        return test_report(argv[0]);