Re: [PATCH 09/17] test: add basic barebox self-test infrastructure

[Jules Maselbas <jmaselbas@kalray.eu>]

Hi Ahmad,

This looks nice, I can't wait to try it out :]

On Mon, Apr 12, 2021 at 09:16:43AM +0200, Ahmad Fatoum wrote:
> Self tests is code written to run within barebox to exercise
> functionality. They offer flexibility to test specific units of barebox
> instead of the program as a whole. Add a very simple infrastructure
> for registering and executing self-tests. THis is based on the Linux
> kselftest modules. We don't utilize modules for this, however, because
> we only have module support on ARM, but we need a generic solution.
> 
> Selftests can be enabled individually and even tested without shell
> support to allow tests to happen for size-restricted barebox images
> as well.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  Kconfig             |  1 +
>  Makefile            |  2 +-
>  commands/Makefile   |  1 +
>  commands/selftest.c | 88 +++++++++++++++++++++++++++++++++++++++++++++
>  include/bselftest.h | 70 ++++++++++++++++++++++++++++++++++++
>  test/Kconfig        |  8 +++++
>  test/Makefile       |  1 +
>  test/self/Kconfig   | 33 +++++++++++++++++
>  test/self/Makefile  |  3 ++
>  test/self/core.c    | 39 ++++++++++++++++++++
>  10 files changed, 245 insertions(+), 1 deletion(-)
>  create mode 100644 commands/selftest.c
>  create mode 100644 include/bselftest.h
>  create mode 100644 test/Kconfig
>  create mode 100644 test/Makefile
>  create mode 100644 test/self/Kconfig
>  create mode 100644 test/self/Makefile
>  create mode 100644 test/self/core.c
> 
> diff --git a/Kconfig b/Kconfig
> index 29c32463fb43..7c4cf36881b4 100644
> --- a/Kconfig
> +++ b/Kconfig
> @@ -15,3 +15,4 @@ source "lib/Kconfig"
>  source "crypto/Kconfig"
>  source "firmware/Kconfig"
>  source "scripts/Kconfig"
> +source "test/Kconfig"
> diff --git a/Makefile b/Makefile
> index f6c0c4817046..0a93915cf347 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -581,7 +581,7 @@ endif
>  include $(srctree)/scripts/Makefile.lib
>  
>  # Objects we will link into barebox / subdirs we need to visit
> -common-y		:= common/ drivers/ commands/ lib/ crypto/ net/ fs/ firmware/
> +common-y		:= common/ drivers/ commands/ lib/ crypto/ net/ fs/ firmware/ test/
>  
>  include $(srctree)/arch/$(SRCARCH)/Makefile
>  
> diff --git a/commands/Makefile b/commands/Makefile
> index 447349fd157d..4b45d266fd56 100644
> --- a/commands/Makefile
> +++ b/commands/Makefile
> @@ -130,5 +130,6 @@ obj-$(CONFIG_CMD_SEED)		+= seed.o
>  obj-$(CONFIG_CMD_IP_ROUTE_GET)  += ip-route-get.o
>  obj-$(CONFIG_CMD_BTHREAD)	+= bthread.o
>  obj-$(CONFIG_CMD_UBSAN)		+= ubsan.o
> +obj-$(CONFIG_CMD_SELFTEST)	+= selftest.o
>  
>  UBSAN_SANITIZE_ubsan.o := y
> diff --git a/commands/selftest.c b/commands/selftest.c
> new file mode 100644
> index 000000000000..de5d100de04a
> --- /dev/null
> +++ b/commands/selftest.c
> @@ -0,0 +1,88 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#define pr_fmt(fmt) "selftest: " fmt
> +
> +#include <common.h>
> +#include <command.h>
> +#include <getopt.h>
> +#include <bselftest.h>
> +#include <complete.h>
> +
> +static int run_selftest(const char *match, bool list)
> +{
> +	struct selftest *test;
> +	int matches = 0;
> +	int err = 0;
> +
> +	list_for_each_entry(test, &selftests, list) {
> +		if (list) {
> +			printf("%s\n", test->name);
> +			matches++;
> +			continue;
> +		}
> +
> +		if (match && strcmp(test->name, match))
> +			continue;
> +
> +		err |= test->func();
> +		matches++;
> +	}
> +
> +	if (!matches) {
> +		if (match)
> +			printf("No tests matching '%s' found.\n", match);
> +		else
> +			printf("No matching tests found.\n");
> +
> +		return -EINVAL;
What if there is no tests at all: is that an error if no tests were ran?
despit not using a match condition ? 

> +	}
> +
> +	return err;
> +}
> +
snip
> diff --git a/test/self/core.c b/test/self/core.c
> new file mode 100644
> index 000000000000..d8c6e5d271ba
> --- /dev/null
> +++ b/test/self/core.c
> @@ -0,0 +1,39 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#define pr_fmt(fmt) "bselftest: " fmt
> +
> +#include <common.h>
> +#include <bselftest.h>
> +
> +LIST_HEAD(selftests);
> +
> +static int (*old_main)(void);
> +
> +static int run_selftests(void)
> +{
> +	struct selftest *test;
> +	int err = 0;
> +
> +	pr_notice("Configured tests will run now\n");
> +
> +	list_for_each_entry(test, &selftests, list)
> +		err |= test->func();
> +
> +	if (err)
> +		pr_err("Some selftests failed\n");
> +
> +	barebox_main = old_main;
> +	return barebox_main();
> +}
Why not call run_selftests in start_barebox, right before barebox_main ?
Or maybe add a selftest initcall to be run at the very end of initcalls.
So you don't have to save and restore the barebox_main function.

> +
> +static int init_selftests(void)
> +{
> +	if (!IS_ENABLED(CONFIG_SELFTEST_AUTORUN))
> +		return 0;
> +
> +	old_main = barebox_main;
> +	barebox_main = run_selftests;
> +
> +	return 0;
> +}
> +postenvironment_initcall(init_selftests);
> -- 
> 2.29.2
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox