[PATCH v2 2/5] lib: Add SBIUnit testing macros and functions
Andrew Jones
ajones at ventanamicro.com
Wed Feb 28 08:30:45 PST 2024
On Wed, Feb 28, 2024 at 04:12:13PM +0000, Ivan Orlov wrote:
> On 2/28/24 14:19, Andrew Jones wrote:
>
> > > + * Author: Ivan Orlov <ivan.orlov0322 at gmail.com>
> > > + */
> > > +#ifdef CONFIG_SBIUNIT
> > > +#ifndef __SBI_UNIT_H__
> > > +#define __SBI_UNIT_H__
> > > +
> > > +#include <sbi/sbi_types.h>
> > > +#include <sbi/sbi_console.h>
> > > +#include <sbi/sbi_string.h>
> > > +
> > > +struct sbiunit_test_case {
> > > + const char *name;
> > > + bool enabled;
> > > + bool failed;
> > > + void (*test_func)(struct sbiunit_test_case *test);
> > > + void (*onerr)(struct sbiunit_test_case *test);
> >
> > Please rename to on_error()
> >
>
> Hmm, it looks like this 'onerr' is not used anywhere, so I'll just get rid
> of it in the V3.
>
> > > +};
> > > +
> > > +struct sbiunit_test_suite {
> > > + const char *name;
> > > + struct sbiunit_test_case *cases;
> > > +};
> > > +
> > > +#define SBIUNIT_TEST_CASE(func) \
> > > + { \
> > > + .name = #func, \
> > > + .enabled = true, \
> > > + .failed = false, \
> > > + .test_func = (func) \
> > > + }
> > > +
> > > +#define SBIUNIT_TEST_SUITE(suite_name, cases_arr) \
> > > + struct sbiunit_test_suite suite_name = { \
> > > + .name = #suite_name, \
> > > + .cases = cases_arr \
> > > + }
> > > +
> > > +#define _sbiunit_msg(test, msg) "[SBIUnit] [%s:%d]: %s: %s", __FILE__, \
> > > + __LINE__, test->name, msg
> > > +
> > > +#define SBIUNIT_INFO(test, msg) sbi_printf(_sbiunit_msg(test, msg))
> > > +#define SBIUNIT_PANIC(test, msg) sbi_panic(_sbiunit_msg(test, msg))
> > > +
> > > +#define SBIUNIT_EXPECT(test, cond) do { \
> > > + if (!(cond)) { \
> > > + test->failed = true; \
> > > + SBIUNIT_INFO(test, "Condition \"" #cond "\" expected to be true!\n"); \
> > > + } \
> > > +} while (0)
> > > +
> > > +#define SBIUNIT_ASSERT(test, cond) do { \
> > > + if (!(cond)) { \
> > > + test->failed = true; \
> >
> > No need to set failed since the next line calls panic.
> >
>
> Yeah, I agree.
>
> > > + SBIUNIT_PANIC(test, "Condition \"" #cond "\" must be true!\n"); \
> > > + } \
> > > +} while (0)
> > > +
> > > +#define SBIUNIT_EXPECT_EQ(test, a, b) SBIUNIT_EXPECT(test, (a) == (b))
> > > +#define SBIUNIT_ASSERT_EQ(test, a, b) SBIUNIT_ASSERT(test, (a) == (b))
> > > +#define SBIUNIT_EXPECT_NE(test, a, b) SBIUNIT_EXPECT(test, (a) != (b))
> > > +#define SBIUNIT_ASSERT_NE(test, a, b) SBIUNIT_ASSERT(test, (a) != (b))
> > > +#define SBIUNIT_EXPECT_MEMEQ(test, a, b, len) SBIUNIT_EXPECT(test, !sbi_memcmp(a, b, len))
> > > +#define SBIUNIT_ASSERT_MEMEQ(test, a, b, len) SBIUNIT_ASSERT(test, !sbi_memcmp(a, b, len))
> > > +#define SBIUNIT_EXPECT_STREQ(test, a, b, len) SBIUNIT_EXPECT(test, !sbi_strncmp(a, b, len))
> > > +#define SBIUNIT_ASSERT_STREQ(test, a, b, len) SBIUNIT_ASSERT(test, !sbi_strncmp(a, b, len))
> > > +
> > > +void run_all_tests(void);
> > > +#endif
> > > +#else
> > > +#define run_all_tests()
> > > +#endif
> > > diff --git a/lib/sbi/Kconfig b/lib/sbi/Kconfig
> > > index 81dd2db..e3038ee 100644
> > > --- a/lib/sbi/Kconfig
> > > +++ b/lib/sbi/Kconfig
> > > @@ -50,4 +50,8 @@ config SBI_ECALL_DBTR
> > > bool "Debug Trigger Extension"
> > > default y
> > > +config SBIUNIT
> > > + bool "Enable SBIUNIT tests"
> > > + default n
> > > +
> > > endmenu
> > > diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
> > > index 0a50e95..08959f1 100644
> > > --- a/lib/sbi/objects.mk
> > > +++ b/lib/sbi/objects.mk
> > > @@ -11,6 +11,8 @@ libsbi-objs-y += riscv_asm.o
> > > libsbi-objs-y += riscv_atomic.o
> > > libsbi-objs-y += riscv_hardfp.o
> > > libsbi-objs-y += riscv_locks.o
> > > +libsbi-objs-$(CONFIG_SBIUNIT) += sbi_unit_test.o
> > > +libsbi-objs-$(CONFIG_SBIUNIT) += sbi_unit_tests.o
> > > libsbi-objs-y += sbi_ecall.o
> > > libsbi-objs-y += sbi_ecall_exts.o
> > > diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
> > > index 804b01c..796cccc 100644
> > > --- a/lib/sbi/sbi_init.c
> > > +++ b/lib/sbi/sbi_init.c
> > > @@ -29,6 +29,7 @@
> > > #include <sbi/sbi_timer.h>
> > > #include <sbi/sbi_tlb.h>
> > > #include <sbi/sbi_version.h>
> > > +#include <sbi/sbi_unit_test.h>
> > > #define BANNER \
> > > " ____ _____ ____ _____\n" \
> > > @@ -398,6 +399,8 @@ static void __noreturn init_coldboot(struct sbi_scratch *scratch, u32 hartid)
> > > sbi_boot_print_hart(scratch, hartid);
> > > + run_all_tests();
> > > +
> > > /*
> > > * Configure PMP at last because if SMEPMP is detected,
> > > * M-mode access to the S/U space will be rescinded.
> > > diff --git a/lib/sbi/sbi_unit_test.c b/lib/sbi/sbi_unit_test.c
> > > new file mode 100644
> > > index 0000000..f4988d8
> > > --- /dev/null
> > > +++ b/lib/sbi/sbi_unit_test.c
> > > @@ -0,0 +1,43 @@
> > > +/*
> > > + * SPDX-License-Identifier: BSD-2-Clause
> > > + *
> > > + * Author: Ivan Orlov <ivan.orlov0322 at gmail.com>
> > > + */
> > > +#include <sbi/sbi_unit_test.h>
> > > +#include <sbi/sbi_types.h>
> > > +#include <sbi/sbi_console.h>
> > > +
> > > +extern struct sbiunit_test_suite *sbi_unit_tests[];
> > > +extern unsigned long sbi_unit_tests_size;
> > > +
> > > +static void run_test_suite(struct sbiunit_test_suite *suite)
> > > +{
> > > + struct sbiunit_test_case *s_case;
> > > + u32 count_pass = 0, count_fail = 0;
> > > +
> > > + sbi_printf("## Running test suite: %s\n", suite->name);
> > > +
> > > + s_case = suite->cases;
> > > + while (s_case->enabled) {
> >
> > Can't test cases be enabled/disabled sparsely? Shouldn't we instead
> > loop while s_case->name is not NULL? And we need to ensure the cases
> > array always has an empty case at the end.
> >
>
> They can't be enabled/disabled separately at the moment, but I believe it
> would be good to have such a functionality in the future, when we have more
> tests. Also, I reckon we should be able to iterate and execute the test even
> if it has no name: technically it doesn't mean that the test is invalid, and
> breaking the tests execution completely in this case would be pretty
> implicit and non-obvious for the test developer...
If a no-name test is OK, then the stop condition should be
s_case->test_func being NULL.
Thanks,
drew
>
> Thank you for the review!
>
> --
> Kind regards,
> Ivan Orlov
>
More information about the opensbi
mailing list