[PATCH v2 0/5] SBIUnit: cover OpenSBI with tests

[PATCH v2 0/5] SBIUnit: cover OpenSBI with tests

[Ivan Orlov]

It is good when the code is covered with tests. Tests help us to keep
the code clean and avoid regressions. Also, a good test is always a nice
documentation for the code it covers.

This patch series introduces SBIUnit - the set of macros and functions 
which simplify the unit test development for OpenSBI and automate tests
execution and evaluation.

This thing is mainly inspired by the KUnit framework from the Linux
Kernel, where the similar unit-test development tooling have been used
successfully for a pretty long time now. SBIUnit uses the same test
structure: multiple test cases are grouped together into the test
suites. SBIUnit also tries to reproduce the KUnit tests API while
allowing for some simplifications.

Another difference between the KUnit and SBIUnit is the location where
the tests are "stored". KUnit creates the ELF section in the kernel and
stores the pointers to all of the test suites there. SBIUnit takes
advantage of the 'carray' functionality of OpenSBI, keeping the pointers
to test suites in the auto-generated ".c" file. ELF section approach
could not be applied to SBIUnit, because OpenSBI could be used as a
static library when linking firmware. Use of a dedicated ELF section
would mean that all firmware linked with OpenSBI would need to have
this ELF section as well.

V1 -> V2:
- Add a cover letter
- Elaborate more on the differences between SBIUnit and KUnit
- Add a new patch adding a new entry to the 'clear' makefile target in
order to clear carray-generated files as well.
- (Patch-specific changes are described in the following patches)

Ivan Orlov (5):
  docs: Add documentation about tests and SBIUnit
  lib: Add SBIUnit testing macros and functions
  Makefile: clean '.c' files generated by carray
  lib: tests: Add a test for sbi_bitmap
  lib: tests: Add sbi_console test

 Makefile                      |   2 +
 docs/writing_tests.md         | 131 ++++++++++++++++++++++++++++++++++
 include/sbi/sbi_unit_test.h   |  74 +++++++++++++++++++
 lib/sbi/Kconfig               |   4 ++
 lib/sbi/objects.mk            |   6 ++
 lib/sbi/sbi_bitmap_test.c     | 103 ++++++++++++++++++++++++++
 lib/sbi/sbi_console.c         |   4 ++
 lib/sbi/sbi_console_test.c    | 102 ++++++++++++++++++++++++++
 lib/sbi/sbi_init.c            |   3 +
 lib/sbi/sbi_unit_test.c       |  43 +++++++++++
 lib/sbi/sbi_unit_tests.carray |   3 +
 11 files changed, 475 insertions(+)
 create mode 100644 docs/writing_tests.md
 create mode 100644 include/sbi/sbi_unit_test.h
 create mode 100644 lib/sbi/sbi_bitmap_test.c
 create mode 100644 lib/sbi/sbi_console_test.c
 create mode 100644 lib/sbi/sbi_unit_test.c
 create mode 100644 lib/sbi/sbi_unit_tests.carray

-- 
2.34.1

[PATCH v2 1/5] docs: Add documentation about tests and SBIUnit

[Ivan Orlov]

This patch contains the documentation for SBIUnit. It describes:

- What is SBIUnit
- Simple test writing scenario
- How we can cover static functions
- How we can "mock" structures in order to test the functions which
operate on them
- SBIUnit API Reference

Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
---
V1 -> V2:
- Use more appropriate language
- Update the documentation in accordance with SBIUnit API and
structure changes
- Remove the redundant documentation for every SBIUNIT_* macro.
- Make the documentation follow the 80-characters rule.

 docs/writing_tests.md | 131 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 131 insertions(+)
 create mode 100644 docs/writing_tests.md

diff --git a/docs/writing_tests.md b/docs/writing_tests.md
new file mode 100644
index 0000000..279d35b
--- /dev/null
+++ b/docs/writing_tests.md
@@ -0,0 +1,131 @@
+Writing tests for OpenSBI
+=========================
+
+SBIUnit
+-------
+SBIUnit is a set of macros and functions which simplify the test development and
+automate the test execution and evaluation. All of the SBIUnit definitions are
+in the `include/sbi/sbi_unit_test.h` header file, and implementations are
+available in `lib/sbi/sbi_unit_test.c`.
+
+Simple SBIUnit test
+-------------------
+
+For instance, we would like to test the following function from
+`lib/sbi/sbi_string.c`:
+
+```c
+size_t sbi_strlen(const char *str)
+{
+	unsigned long ret = 0;
+
+	while (*str != '\0') {
+		ret++;
+		str++;
+	}
+
+	return ret;
+}
+```
+
+Which calculates the string length.
+
+Create the file `lib/sbi/sbi_string_test.c` with the following content:
+
+```c
+#include <sbi/sbi_unit_test.h>
+#include <sbi/sbi_string.h>
+
+static void strlen_test(struct sbiunit_test_case *test)
+{
+	SBIUNIT_EXPECT_EQ(test, sbi_strlen("Hello"), 5);
+	SBIUNIT_EXPECT_EQ(test, sbi_strlen("Hell\0o"), 4);
+}
+
+static struct sbiunit_test_case string_test_cases[] = {
+	SBIUNIT_TEST_CASE(strlen_test),
+	{},
+};
+
+SBIUNIT_TEST_SUITE(string_test_suite, string_test_cases);
+```
+
+Then, add the corresponding Makefile entries to `lib/sbi/objects.mk`:
+```lang-makefile
+...
+libsbi-objs-$(CONFIG_SBIUNIT) += sbi_string_test.o
+carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += string_test_suite
+```
+
+Run `make clean` in order to regenerate the carray-related files, recompile
+OpenSBI with CONFIG_SBIUNIT option enabled and run it in the QEMU.
+You will see something like this:
+```
+# make PLATFORM=generic run
+...
+# Running SBIUNIT tests #
+...
+## Running test suite: string_test_suite
+[PASSED] strlen_test
+1 PASSED / 0 FAILED / 1 TOTAL
+```
+
+Now let's try to change this test in the way that it will fail:
+
+```c
+- SBIUNIT_EXPECT_EQ(test, sbi_strlen("Hello"), 5);
++ SBIUNIT_EXPECT_EQ(test, sbi_strlen("Hello"), 100);
+```
+
+`make all` and `make run` it again:
+```
+...
+# Running SBIUNIT tests #
+...
+## Running test suite: string_test_suite
+[SBIUnit] [.../opensbi/lib/sbi/sbi_string_test.c:6]: strlen_test: Condition
+"(sbi_strlen("Hello")) == (100)" expected to be true!
+[FAILED] strlen_test
+0 PASSED / 1 FAILED / 1 TOTAL
+```
+Covering the static functions / using the static definitions
+------------------------------------------------------------
+
+SBIUnit also allows you to test static functions. In order to do so, simply
+include your test source in the file you would like to test. Complementing the
+example above, just add this to the
+`lib/sbi/sbi_string.c` file:
+
+```c
+#ifdef CONFIG_SBIUNIT
+#include "sbi_string_test.c"
+#endif
+```
+
+In this case you should only add a new carray entry pointing to the test suite
+to `lib/sbi/objects.mk`:
+```lang-makefile
+...
+carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += string_test_suite
+```
+
+You don't have to compile the `sbi_string_test.o` separately, because the
+test code will be included into the `sbi_string` object file.
+
+See example in `lib/sbi/sbi_console_test.c`, where statically declared
+`console_dev` variable is used to mock the `sbi_console_device` structure.
+
+"Mocking" the structures
+------------------------
+See the example of structure "mocking" in the `lib/sbi/sbi_console_test.c`,
+where the sbi_console_device structure was mocked to be used in various
+console-related functions in order to test them.
+
+API Reference
+-------------
+All of the `SBIUNIT_EXPECT_*` macros will cause a test case to fail if the
+corresponding conditions are not met, however, the execution of a particular
+test case will not be stopped.
+
+All of the `SBIUNIT_ASSERT_*` macros will cause a test case to fail and stop
+immediately, triggering the panic.
-- 
2.34.1

[PATCH v2 2/5] lib: Add SBIUnit testing macros and functions

[Ivan Orlov]

This patch introduces all of the SBIUnit macros and functions which
can be used during the test development process. Also, it defines
the 'run_all_tests' function, which is being called during the
'init_coldboot' right after printing the boot hart information.

Also, add the CONFIG_SBIUNIT Kconfig entry in order to be able to
turn the tests on and off. When the CONFIG_SBIUNIT is disabled,
the tests and all related code is excluded completely on the
compilation stage.

Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
---
V1 -> V2:
- Rename the sources to have 'test' in the names: 'sbi_unit.c' =>
'sbi_unit_test.c', 'sbi_unit.h' => 'sbi_unit_test.h'.
- Rewrite using the carray functionality instead of placing the
pointers to the test suites into the source explicitly.
- Add 'ifdef' into the sbi_unit_test header file so we could
avoid writing a lot of 'ifdef' in other files.
- Get rid of unused symbols and macros
- Change the behaviour of SBIUNIT_ASSERT_* functions: now they trigger
sbi_panic if the assertion fails.
- Add a 'SBIUnit' prefix to the SBIUNIT_INFO macro message
- Rename the sbiunit_test_case structure fields, and use true/false
instead of 1/0
- Change the logic of the test fail detection: now we have the 'failed'
sbiunit_test_case structure field instead of the 'result'. 'failed'
field gets set to 'true' if an assertion or an expectaion fails.
- Fix codestyle issues
- Add 'len' parameter to SBIUNIT_*_STREQ

 include/sbi/sbi_unit_test.h   | 74 +++++++++++++++++++++++++++++++++++
 lib/sbi/Kconfig               |  4 ++
 lib/sbi/objects.mk            |  2 +
 lib/sbi/sbi_init.c            |  3 ++
 lib/sbi/sbi_unit_test.c       | 43 ++++++++++++++++++++
 lib/sbi/sbi_unit_tests.carray |  3 ++
 6 files changed, 129 insertions(+)
 create mode 100644 include/sbi/sbi_unit_test.h
 create mode 100644 lib/sbi/sbi_unit_test.c
 create mode 100644 lib/sbi/sbi_unit_tests.carray

diff --git a/include/sbi/sbi_unit_test.h b/include/sbi/sbi_unit_test.h
new file mode 100644
index 0000000..1538332
--- /dev/null
+++ b/include/sbi/sbi_unit_test.h
@@ -0,0 +1,74 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Author: Ivan Orlov <ivan.orlov0322@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);
+};
+
+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;						\
+		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@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) {
+		s_case->test_func(s_case);
+		if (s_case->failed)
+			count_fail++;
+		else
+			count_pass++;
+		sbi_printf("[%s] %s\n", s_case->failed ? "FAILED" : "PASSED",
+			   s_case->name);
+		s_case++;
+	}
+	sbi_printf("%u PASSED / %u FAILED / %u TOTAL\n", count_pass, count_fail,
+		   count_pass + count_fail);
+}
+
+void run_all_tests(void)
+{
+	u32 i;
+
+	sbi_printf("\n# Running SBIUNIT tests #\n");
+
+	for (i = 0; i < sbi_unit_tests_size; i++)
+		run_test_suite(sbi_unit_tests[i]);
+}
diff --git a/lib/sbi/sbi_unit_tests.carray b/lib/sbi/sbi_unit_tests.carray
new file mode 100644
index 0000000..8d6069b
--- /dev/null
+++ b/lib/sbi/sbi_unit_tests.carray
@@ -0,0 +1,3 @@
+HEADER: sbi/sbi_unit_test.h
+TYPE: struct sbiunit_test_suite
+NAME: sbi_unit_tests
-- 
2.34.1

[PATCH v2 3/5] Makefile: clean '.c' files generated by carray

[Ivan Orlov]

`make clean` doesn't clear the '*.c' files generated by carray.sh
in the 'build' directory. Fix it by extending the 'clean' makefile
target.

This is the only way we are able to regenerate the carray .c file in
case if we add a new test to the 'carray-sbi_unit_tests-y' Makefile
variable. However, running `make clear` every time we add a new test
is not really convenient, so the mechanics should be changed in the
future.

Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
---
 Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Makefile b/Makefile
index 680c19a..4519277 100644
--- a/Makefile
+++ b/Makefile
@@ -684,6 +684,8 @@ clean:
 	$(CMD_PREFIX)find $(build_dir) -type f -name "*.bin" -exec rm -rf {} +
 	$(if $(V), @echo " RM        $(build_dir)/*.dtb")
 	$(CMD_PREFIX)find $(build_dir) -type f -name "*.dtb" -exec rm -rf {} +
+	$(if $(V), @echo " RM        $(build_dir)/*.c")
+	$(CMD_PREFIX)find $(build_dir) -type f -name "*.c" -exec rm -rf {} +
 
 # Rule for "make distclean"
 .PHONY: distclean
-- 
2.34.1

[PATCH v2 4/5] lib: tests: Add a test for sbi_bitmap

[Ivan Orlov]

Add test suite covering all of the functions from lib/sbi/sbi_bitmap.c:
__bitmap_and, __bitmap_or and __bitmap_xor.

Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
---
V1 -> V2:
- Extract the part of the commit description to the cover letter
- Update to use the carray functionality instead of placing the pointer
to the test suite directly into the sources
- Fix codestyle issues
- Use the references to the data_* arrays when defining the expected
test results, instead of repeating the same numbers again and again

 lib/sbi/objects.mk        |   3 ++
 lib/sbi/sbi_bitmap_test.c | 103 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 106 insertions(+)
 create mode 100644 lib/sbi/sbi_bitmap_test.c

diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
index 08959f1..b4c273f 100644
--- a/lib/sbi/objects.mk
+++ b/lib/sbi/objects.mk
@@ -14,6 +14,9 @@ libsbi-objs-y += riscv_locks.o
 libsbi-objs-$(CONFIG_SBIUNIT) += sbi_unit_test.o
 libsbi-objs-$(CONFIG_SBIUNIT) += sbi_unit_tests.o
 
+libsbi-objs-$(CONFIG_SBIUNIT) += sbi_bitmap_test.o
+carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += bitmap_test_suite
+
 libsbi-objs-y += sbi_ecall.o
 libsbi-objs-y += sbi_ecall_exts.o
 
diff --git a/lib/sbi/sbi_bitmap_test.c b/lib/sbi/sbi_bitmap_test.c
new file mode 100644
index 0000000..918e7d9
--- /dev/null
+++ b/lib/sbi/sbi_bitmap_test.c
@@ -0,0 +1,103 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
+ */
+#include <sbi/sbi_bitmap.h>
+#include <sbi/sbi_unit_test.h>
+#include <sbi/sbi_console.h>
+
+#define DATA_SIZE sizeof(data_zero)
+#define DATA_BIT_SIZE (DATA_SIZE * 8)
+
+static u64 data_a[] = { 0xDEADBEEF, 0x00BAB10C, 0x1BADB002, 0xABADBABE };
+static u64 data_b[] = { 0xC00010FF, 0x00BAB10C, 0xBAAAAAAD, 0xBADDCAFE };
+static u64 data_zero[] = { 0, 0, 0, 0 };
+
+static void bitmap_and_test(struct sbiunit_test_case *test)
+{
+	u64 res[DATA_SIZE];
+	u64 a_and_b[] = { data_a[0] & data_b[0], data_a[1] & data_b[1],
+			  data_a[2] & data_b[2], data_a[3] & data_b[3] };
+
+	__bitmap_and(res, data_a, data_b, DATA_BIT_SIZE);
+	SBIUNIT_EXPECT_MEMEQ(test, res, a_and_b, DATA_SIZE);
+
+	/* a & a = a */
+	__bitmap_and(res, data_a, data_a, DATA_BIT_SIZE);
+	SBIUNIT_ASSERT_MEMEQ(test, res, data_a, DATA_SIZE);
+
+	/* a & 0 = 0 */
+	__bitmap_and(res, data_a, data_zero, DATA_BIT_SIZE);
+	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
+
+	/* 0 & 0 = 0 */
+	__bitmap_and(res, data_zero, data_zero, DATA_BIT_SIZE);
+	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
+
+	sbi_memcpy(res, data_zero, DATA_SIZE);
+	/* Cover zero 'bits' argument */
+	__bitmap_and(res, data_a, data_b, 0);
+	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
+}
+
+static void bitmap_or_test(struct sbiunit_test_case *test)
+{
+	u64 res[DATA_SIZE];
+	u64 a_or_b[] = { data_a[0] | data_b[0], data_a[1] | data_b[1],
+			 data_a[2] | data_b[2], data_a[3] | data_b[3] };
+
+	__bitmap_or(res, data_a, data_b, DATA_BIT_SIZE);
+	SBIUNIT_EXPECT_MEMEQ(test, res, a_or_b, DATA_SIZE);
+
+	/* a | a = a */
+	__bitmap_or(res, data_a, data_a, DATA_BIT_SIZE);
+	SBIUNIT_EXPECT_MEMEQ(test, res, data_a, DATA_SIZE);
+
+	/* a | 0 = a */
+	__bitmap_or(res, data_a, data_zero, DATA_BIT_SIZE);
+	SBIUNIT_EXPECT_MEMEQ(test, res, data_a, DATA_SIZE);
+
+	/* 0 | 0 = 0 */
+	__bitmap_or(res, data_zero, data_zero, DATA_BIT_SIZE);
+	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
+
+	sbi_memcpy(res, data_zero, DATA_SIZE);
+	__bitmap_or(res, data_a, data_b, 0);
+	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
+}
+
+static void bitmap_xor_test(struct sbiunit_test_case *test)
+{
+	u64 res[DATA_SIZE];
+	u64 a_xor_b[] = { data_a[0] ^ data_b[0], data_a[1] ^ data_b[1],
+			  data_a[2] ^ data_b[2], data_a[3] ^ data_b[3] };
+
+	__bitmap_xor(res, data_a, data_b, DATA_BIT_SIZE);
+	SBIUNIT_EXPECT_MEMEQ(test, res, a_xor_b, DATA_SIZE);
+
+	/* a ^ 0 = a */
+	__bitmap_xor(res, data_a, data_zero, DATA_BIT_SIZE);
+	SBIUNIT_EXPECT_MEMEQ(test, res, data_a, DATA_SIZE);
+
+	/* a ^ a = 0 */
+	__bitmap_xor(res, data_a, data_a, DATA_BIT_SIZE);
+	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
+
+	/* 0 ^ 0 = 0 */
+	__bitmap_xor(res, data_zero, data_zero, DATA_BIT_SIZE);
+	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
+
+	sbi_memcpy(res, data_zero, DATA_SIZE);
+	__bitmap_xor(res, data_a, data_b, 0);
+	SBIUNIT_EXPECT_MEMEQ(test, res, data_zero, DATA_SIZE);
+}
+
+static struct sbiunit_test_case bitmap_test_cases[] = {
+	SBIUNIT_TEST_CASE(bitmap_and_test),
+	SBIUNIT_TEST_CASE(bitmap_or_test),
+	SBIUNIT_TEST_CASE(bitmap_xor_test),
+	{},
+};
+
+SBIUNIT_TEST_SUITE(bitmap_test_suite, bitmap_test_cases);
-- 
2.34.1

[PATCH v2 5/5] lib: tests: Add sbi_console test

[Ivan Orlov]

Add the test suite covering some of the functions from
lib/sbi/sbi_console.c: putc, puts and printf. The test covers a variety
of format specifiers for printf and different strings and characters for
putc and puts.

In order to do that, the test "mocks" the sbi_console_device structure
by setting the 'console_dev' variable to the virtual console.

Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
---
V1 -> V2:
- Rewrite using the carray functionality
- Replace CONSOLE_DO and CONSOLE_DO_RET macros with two inline
functions: one of them "mocks" the default console device, and
the second one restores the old console device.
- Fix codestyle issues (comments, etc.)
- Remove incorrect 'puts' test
- Use updated SBIUNIT_ASSERT_STREQ API

 lib/sbi/objects.mk         |   1 +
 lib/sbi/sbi_console.c      |   4 ++
 lib/sbi/sbi_console_test.c | 102 +++++++++++++++++++++++++++++++++++++
 3 files changed, 107 insertions(+)
 create mode 100644 lib/sbi/sbi_console_test.c

diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
index b4c273f..9d065fa 100644
--- a/lib/sbi/objects.mk
+++ b/lib/sbi/objects.mk
@@ -16,6 +16,7 @@ libsbi-objs-$(CONFIG_SBIUNIT) += sbi_unit_tests.o
 
 libsbi-objs-$(CONFIG_SBIUNIT) += sbi_bitmap_test.o
 carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += bitmap_test_suite
+carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += console_test_suite
 
 libsbi-objs-y += sbi_ecall.o
 libsbi-objs-y += sbi_ecall_exts.o
diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
index ab09a5c..d1229d0 100644
--- a/lib/sbi/sbi_console.c
+++ b/lib/sbi/sbi_console.c
@@ -488,3 +488,7 @@ int sbi_console_init(struct sbi_scratch *scratch)
 
 	return rc;
 }
+
+#ifdef CONFIG_SBIUNIT
+#include "sbi_console_test.c"
+#endif
diff --git a/lib/sbi/sbi_console_test.c b/lib/sbi/sbi_console_test.c
new file mode 100644
index 0000000..f24b329
--- /dev/null
+++ b/lib/sbi/sbi_console_test.c
@@ -0,0 +1,102 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
+ */
+#include <sbi/sbi_unit_test.h>
+#include <sbi/sbi_heap.h>
+
+#define TEST_CONSOLE_BUF_LEN 1024
+
+static const struct sbi_console_device *old_dev;
+static char test_console_buf[TEST_CONSOLE_BUF_LEN];
+static u32 test_console_buf_pos;
+
+static void test_console_putc(char c)
+{
+	test_console_buf[test_console_buf_pos] = c;
+	test_console_buf_pos = (test_console_buf_pos + 1) % TEST_CONSOLE_BUF_LEN;
+}
+
+static void clear_test_console_buf(void)
+{
+	test_console_buf_pos = 0;
+	test_console_buf[0] = '\0';
+}
+
+static const struct sbi_console_device new_dev = {
+	.name = "Test console device",
+	.console_putc = test_console_putc,
+};
+
+/* Mock the console device */
+static inline void test_console_begin(const struct sbi_console_device *device)
+{
+	old_dev = console_dev;
+	console_dev = device;
+}
+
+static inline void test_console_end(void)
+{
+	console_dev = old_dev;
+}
+
+static void putc_test(struct sbiunit_test_case *test)
+{
+	clear_test_console_buf();
+
+	test_console_begin(&new_dev);
+	sbi_putc('a');
+	test_console_end();
+	SBIUNIT_ASSERT_EQ(test, test_console_buf[0], 'a');
+}
+
+#define PUTS_TEST(test, expected, str) do {			\
+	clear_test_console_buf();				\
+	test_console_begin(&new_dev);				\
+	sbi_puts(str);						\
+	test_console_end();					\
+	SBIUNIT_ASSERT_STREQ(test, test_console_buf, expected,	\
+			     sbi_strlen(expected));		\
+} while (0)
+
+static void puts_test(struct sbiunit_test_case *test)
+{
+	PUTS_TEST(test, "Hello, OpenSBI!", "Hello, OpenSBI!");
+	PUTS_TEST(test, "Hello,\r\nOpenSBI!", "Hello,\nOpenSBI!");
+}
+
+#define PRINTF_TEST(test, expected, format, ...) do {		\
+	clear_test_console_buf();				\
+	test_console_begin(&new_dev);				\
+	size_t __res = sbi_printf(format, ##__VA_ARGS__);	\
+	test_console_end();					\
+	SBIUNIT_ASSERT_EQ(test, __res, sbi_strlen(expected));	\
+	SBIUNIT_ASSERT_STREQ(test, test_console_buf, expected,	\
+			     sbi_strlen(expected));		\
+} while (0)
+
+static void printf_test(struct sbiunit_test_case *test)
+{
+	PRINTF_TEST(test, "Hello", "Hello");
+	PRINTF_TEST(test, "3 5 7", "%d %d %d", 3, 5, 7);
+	PRINTF_TEST(test, "Hello", "%s", "Hello");
+	PRINTF_TEST(test, "-1", "%d", -1);
+	PRINTF_TEST(test, "FF", "%X", 255);
+	PRINTF_TEST(test, "ff", "%x", 255);
+	PRINTF_TEST(test, "A", "%c", 'A');
+	PRINTF_TEST(test, "1fe", "%p", (void *)0x1fe);
+	PRINTF_TEST(test, "4294967295", "%u", 4294967295U);
+	PRINTF_TEST(test, "-2147483647", "%ld", -2147483647l);
+	PRINTF_TEST(test, "-9223372036854775807", "%lld", -9223372036854775807LL);
+	PRINTF_TEST(test, "18446744073709551615", "%llu", 18446744073709551615ULL);
+}
+
+static struct sbiunit_test_case console_test_cases[] = {
+	SBIUNIT_TEST_CASE(putc_test),
+	SBIUNIT_TEST_CASE(puts_test),
+	SBIUNIT_TEST_CASE(printf_test),
+	{},
+};
+
+SBIUNIT_TEST_SUITE(console_test_suite, console_test_cases);
-- 
2.34.1

[PATCH v2 5/5] lib: tests: Add sbi_console test

[Xiang W]

? 2024-02-15???? 16:16 +0000?Ivan Orlov???
> Add the test suite covering some of the functions from
> lib/sbi/sbi_console.c: putc, puts and printf. The test covers a variety
> of format specifiers for printf and different strings and characters for
> putc and puts.
> 
> In order to do that, the test "mocks" the sbi_console_device structure
> by setting the 'console_dev' variable to the virtual console.
> 
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> ---
> V1 -> V2:
> - Rewrite using the carray functionality
> - Replace CONSOLE_DO and CONSOLE_DO_RET macros with two inline
> functions: one of them "mocks" the default console device, and
> the second one restores the old console device.
> - Fix codestyle issues (comments, etc.)
> - Remove incorrect 'puts' test
> - Use updated SBIUNIT_ASSERT_STREQ API
> 
> ?lib/sbi/objects.mk???????? |?? 1 +
> ?lib/sbi/sbi_console.c????? |?? 4 ++
> ?lib/sbi/sbi_console_test.c | 102 +++++++++++++++++++++++++++++++++++++
> ?3 files changed, 107 insertions(+)
> ?create mode 100644 lib/sbi/sbi_console_test.c
> 
> diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
> index b4c273f..9d065fa 100644
> --- a/lib/sbi/objects.mk
> +++ b/lib/sbi/objects.mk
> @@ -16,6 +16,7 @@ libsbi-objs-$(CONFIG_SBIUNIT) += sbi_unit_tests.o
> ?
> ?libsbi-objs-$(CONFIG_SBIUNIT) += sbi_bitmap_test.o
> ?carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += bitmap_test_suite
> +carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += console_test_suite
> ?
> ?libsbi-objs-y += sbi_ecall.o
> ?libsbi-objs-y += sbi_ecall_exts.o
> diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
> index ab09a5c..d1229d0 100644
> --- a/lib/sbi/sbi_console.c
> +++ b/lib/sbi/sbi_console.c
> @@ -488,3 +488,7 @@ int sbi_console_init(struct sbi_scratch *scratch)
> ?
> ?	return rc;
> ?}
> +
> +#ifdef CONFIG_SBIUNIT
> +#include "sbi_console_test.c"
> +#endif
> diff --git a/lib/sbi/sbi_console_test.c b/lib/sbi/sbi_console_test.c
> new file mode 100644
> index 0000000..f24b329
> --- /dev/null
> +++ b/lib/sbi/sbi_console_test.c
> @@ -0,0 +1,102 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
> + */
> +#include <sbi/sbi_unit_test.h>
> +#include <sbi/sbi_heap.h>

sbi_heap.h is not needed.

Regards,
Xiang W
> +
> +#define TEST_CONSOLE_BUF_LEN 1024
> +
> +static const struct sbi_console_device *old_dev;
> +static char test_console_buf[TEST_CONSOLE_BUF_LEN];
> +static u32 test_console_buf_pos;
> +
> +static void test_console_putc(char c)
> +{
> +	test_console_buf[test_console_buf_pos] = c;
> +	test_console_buf_pos = (test_console_buf_pos + 1) % TEST_CONSOLE_BUF_LEN;
> +}
> +
> +static void clear_test_console_buf(void)
> +{
> +	test_console_buf_pos = 0;
> +	test_console_buf[0] = '\0';
> +}
> +
> +static const struct sbi_console_device new_dev = {
> +	.name = "Test console device",
> +	.console_putc = test_console_putc,
> +};
> +
> +/* Mock the console device */
> +static inline void test_console_begin(const struct sbi_console_device *device)
> +{
> +	old_dev = console_dev;
> +	console_dev = device;
> +}
> +
> +static inline void test_console_end(void)
> +{
> +	console_dev = old_dev;
> +}
> +
> +static void putc_test(struct sbiunit_test_case *test)
> +{
> +	clear_test_console_buf();
> +
> +	test_console_begin(&new_dev);
> +	sbi_putc('a');
> +	test_console_end();
> +	SBIUNIT_ASSERT_EQ(test, test_console_buf[0], 'a');
> +}
> +
> +#define PUTS_TEST(test, expected, str) do {			\
> +	clear_test_console_buf();				\
> +	test_console_begin(&new_dev);				\
> +	sbi_puts(str);						\
> +	test_console_end();					\
> +	SBIUNIT_ASSERT_STREQ(test, test_console_buf, expected,	\
> +			???? sbi_strlen(expected));		\
> +} while (0)
> +
> +static void puts_test(struct sbiunit_test_case *test)
> +{
> +	PUTS_TEST(test, "Hello, OpenSBI!", "Hello, OpenSBI!");
> +	PUTS_TEST(test, "Hello,\r\nOpenSBI!", "Hello,\nOpenSBI!");
> +}
> +
> +#define PRINTF_TEST(test, expected, format, ...) do {		\
> +	clear_test_console_buf();				\
> +	test_console_begin(&new_dev);				\
> +	size_t __res = sbi_printf(format, ##__VA_ARGS__);	\
> +	test_console_end();					\
> +	SBIUNIT_ASSERT_EQ(test, __res, sbi_strlen(expected));	\
> +	SBIUNIT_ASSERT_STREQ(test, test_console_buf, expected,	\
> +			???? sbi_strlen(expected));		\
> +} while (0)
> +
> +static void printf_test(struct sbiunit_test_case *test)
> +{
> +	PRINTF_TEST(test, "Hello", "Hello");
> +	PRINTF_TEST(test, "3 5 7", "%d %d %d", 3, 5, 7);
> +	PRINTF_TEST(test, "Hello", "%s", "Hello");
> +	PRINTF_TEST(test, "-1", "%d", -1);
> +	PRINTF_TEST(test, "FF", "%X", 255);
> +	PRINTF_TEST(test, "ff", "%x", 255);
> +	PRINTF_TEST(test, "A", "%c", 'A');
> +	PRINTF_TEST(test, "1fe", "%p", (void *)0x1fe);
> +	PRINTF_TEST(test, "4294967295", "%u", 4294967295U);
> +	PRINTF_TEST(test, "-2147483647", "%ld", -2147483647l);
> +	PRINTF_TEST(test, "-9223372036854775807", "%lld", -9223372036854775807LL);
> +	PRINTF_TEST(test, "18446744073709551615", "%llu", 18446744073709551615ULL);
> +}
> +
> +static struct sbiunit_test_case console_test_cases[] = {
> +	SBIUNIT_TEST_CASE(putc_test),
> +	SBIUNIT_TEST_CASE(puts_test),
> +	SBIUNIT_TEST_CASE(printf_test),
> +	{},
> +};
> +
> +SBIUNIT_TEST_SUITE(console_test_suite, console_test_cases);
> -- 
> 2.34.1
> 
> 

[PATCH v2 5/5] lib: tests: Add sbi_console test

[Ivan Orlov]

On 2/18/24 05:34, Xiang W wrote:
> ? 2024-02-15???? 16:16 +0000?Ivan Orlov???
>> Add the test suite covering some of the functions from
>> lib/sbi/sbi_console.c: putc, puts and printf. The test covers a variety
>> of format specifiers for printf and different strings and characters for
>> putc and puts.
>>
>> In order to do that, the test "mocks" the sbi_console_device structure
>> by setting the 'console_dev' variable to the virtual console.
>>
>> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
>> ---
>> V1 -> V2:
>> - Rewrite using the carray functionality
>> - Replace CONSOLE_DO and CONSOLE_DO_RET macros with two inline
>> functions: one of them "mocks" the default console device, and
>> the second one restores the old console device.
>> - Fix codestyle issues (comments, etc.)
>> - Remove incorrect 'puts' test
>> - Use updated SBIUNIT_ASSERT_STREQ API
>>
>>  ?lib/sbi/objects.mk???????? |?? 1 +
>>  ?lib/sbi/sbi_console.c????? |?? 4 ++
>>  ?lib/sbi/sbi_console_test.c | 102 +++++++++++++++++++++++++++++++++++++
>>  ?3 files changed, 107 insertions(+)
>>  ?create mode 100644 lib/sbi/sbi_console_test.c
>>
>> diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
>> index b4c273f..9d065fa 100644
>> --- a/lib/sbi/objects.mk
>> +++ b/lib/sbi/objects.mk
>> @@ -16,6 +16,7 @@ libsbi-objs-$(CONFIG_SBIUNIT) += sbi_unit_tests.o
>>   
>>  ?libsbi-objs-$(CONFIG_SBIUNIT) += sbi_bitmap_test.o
>>  ?carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += bitmap_test_suite
>> +carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += console_test_suite
>>   
>>  ?libsbi-objs-y += sbi_ecall.o
>>  ?libsbi-objs-y += sbi_ecall_exts.o
>> diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
>> index ab09a5c..d1229d0 100644
>> --- a/lib/sbi/sbi_console.c
>> +++ b/lib/sbi/sbi_console.c
>> @@ -488,3 +488,7 @@ int sbi_console_init(struct sbi_scratch *scratch)
>>   
>>  ?	return rc;
>>  ?}
>> +
>> +#ifdef CONFIG_SBIUNIT
>> +#include "sbi_console_test.c"
>> +#endif
>> diff --git a/lib/sbi/sbi_console_test.c b/lib/sbi/sbi_console_test.c
>> new file mode 100644
>> index 0000000..f24b329
>> --- /dev/null
>> +++ b/lib/sbi/sbi_console_test.c
>> @@ -0,0 +1,102 @@
>> +/*
>> + * SPDX-License-Identifier: BSD-2-Clause
>> + *
>> + * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
>> + */
>> +#include <sbi/sbi_unit_test.h>
>> +#include <sbi/sbi_heap.h>
> 
> sbi_heap.h is not needed.
> 

Thanks, I'll fix it in V3.

-- 
Kind regards,
Ivan Orlov

[PATCH v2 1/5] docs: Add documentation about tests and SBIUnit

[Andrew Jones]

On Thu, Feb 15, 2024 at 04:16:21PM +0000, Ivan Orlov wrote:
> This patch contains the documentation for SBIUnit. It describes:
> 
> - What is SBIUnit
> - Simple test writing scenario
> - How we can cover static functions
> - How we can "mock" structures in order to test the functions which
> operate on them
> - SBIUnit API Reference
> 
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> ---
> V1 -> V2:
> - Use more appropriate language
> - Update the documentation in accordance with SBIUnit API and
> structure changes
> - Remove the redundant documentation for every SBIUNIT_* macro.
> - Make the documentation follow the 80-characters rule.
> 
>  docs/writing_tests.md | 131 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 131 insertions(+)
>  create mode 100644 docs/writing_tests.md
> 
> diff --git a/docs/writing_tests.md b/docs/writing_tests.md
> new file mode 100644
> index 0000000..279d35b
> --- /dev/null
> +++ b/docs/writing_tests.md
> @@ -0,0 +1,131 @@
> +Writing tests for OpenSBI
> +=========================
> +
> +SBIUnit
> +-------
> +SBIUnit is a set of macros and functions which simplify the test development and
> +automate the test execution and evaluation. All of the SBIUnit definitions are
> +in the `include/sbi/sbi_unit_test.h` header file, and implementations are
> +available in `lib/sbi/sbi_unit_test.c`.
> +
> +Simple SBIUnit test
> +-------------------
> +
> +For instance, we would like to test the following function from
> +`lib/sbi/sbi_string.c`:
> +
> +```c
> +size_t sbi_strlen(const char *str)
> +{
> +	unsigned long ret = 0;
> +
> +	while (*str != '\0') {
> +		ret++;
> +		str++;
> +	}
> +
> +	return ret;
> +}
> +```
> +
> +Which calculates the string length.

Lowercase 'w' in which, since this line is a continuation of the sentence
started above.

> +
> +Create the file `lib/sbi/sbi_string_test.c` with the following content:
> +
> +```c
> +#include <sbi/sbi_unit_test.h>
> +#include <sbi/sbi_string.h>
> +
> +static void strlen_test(struct sbiunit_test_case *test)
> +{
> +	SBIUNIT_EXPECT_EQ(test, sbi_strlen("Hello"), 5);
> +	SBIUNIT_EXPECT_EQ(test, sbi_strlen("Hell\0o"), 4);
> +}
> +
> +static struct sbiunit_test_case string_test_cases[] = {
> +	SBIUNIT_TEST_CASE(strlen_test),
> +	{},
> +};
> +
> +SBIUNIT_TEST_SUITE(string_test_suite, string_test_cases);
> +```
> +
> +Then, add the corresponding Makefile entries to `lib/sbi/objects.mk`:
> +```lang-makefile
> +...
> +libsbi-objs-$(CONFIG_SBIUNIT) += sbi_string_test.o
> +carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += string_test_suite
> +```
> +
> +Run `make clean` in order to regenerate the carray-related files, recompile
> +OpenSBI with CONFIG_SBIUNIT option enabled and run it in the QEMU.
> +You will see something like this:
> +```
> +# make PLATFORM=generic run
> +...
> +# Running SBIUNIT tests #
> +...
> +## Running test suite: string_test_suite
> +[PASSED] strlen_test
> +1 PASSED / 0 FAILED / 1 TOTAL
> +```
> +
> +Now let's try to change this test in the way that it will fail:
> +
> +```c
> +- SBIUNIT_EXPECT_EQ(test, sbi_strlen("Hello"), 5);
> ++ SBIUNIT_EXPECT_EQ(test, sbi_strlen("Hello"), 100);
> +```
> +
> +`make all` and `make run` it again:
> +```
> +...
> +# Running SBIUNIT tests #
> +...
> +## Running test suite: string_test_suite
> +[SBIUnit] [.../opensbi/lib/sbi/sbi_string_test.c:6]: strlen_test: Condition
> +"(sbi_strlen("Hello")) == (100)" expected to be true!

If the test will output the above all on one line, then the document
should also use a single line even though it will exceed 80 chars.

> +[FAILED] strlen_test
> +0 PASSED / 1 FAILED / 1 TOTAL
> +```
> +Covering the static functions / using the static definitions
> +------------------------------------------------------------
> +
> +SBIUnit also allows you to test static functions. In order to do so, simply
> +include your test source in the file you would like to test. Complementing the
> +example above, just add this to the
> +`lib/sbi/sbi_string.c` file:
> +
> +```c
> +#ifdef CONFIG_SBIUNIT
> +#include "sbi_string_test.c"
> +#endif
> +```
> +
> +In this case you should only add a new carray entry pointing to the test suite
> +to `lib/sbi/objects.mk`:
> +```lang-makefile
> +...
> +carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += string_test_suite
> +```
> +
> +You don't have to compile the `sbi_string_test.o` separately, because the
> +test code will be included into the `sbi_string` object file.
> +
> +See example in `lib/sbi/sbi_console_test.c`, where statically declared
> +`console_dev` variable is used to mock the `sbi_console_device` structure.
> +
> +"Mocking" the structures
> +------------------------
> +See the example of structure "mocking" in the `lib/sbi/sbi_console_test.c`,
> +where the sbi_console_device structure was mocked to be used in various
> +console-related functions in order to test them.
> +
> +API Reference
> +-------------
> +All of the `SBIUNIT_EXPECT_*` macros will cause a test case to fail if the
> +corresponding conditions are not met, however, the execution of a particular
> +test case will not be stopped.
> +
> +All of the `SBIUNIT_ASSERT_*` macros will cause a test case to fail and stop
> +immediately, triggering the panic.

s/the panic/a panic/

> -- 
> 2.34.1
>

Other than the nits,

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew

[PATCH v2 2/5] lib: Add SBIUnit testing macros and functions

[Andrew Jones]

On Thu, Feb 15, 2024 at 04:16:22PM +0000, Ivan Orlov wrote:
> This patch introduces all of the SBIUnit macros and functions which
> can be used during the test development process. Also, it defines
> the 'run_all_tests' function, which is being called during the
> 'init_coldboot' right after printing the boot hart information.
> 
> Also, add the CONFIG_SBIUNIT Kconfig entry in order to be able to
> turn the tests on and off. When the CONFIG_SBIUNIT is disabled,
> the tests and all related code is excluded completely on the
> compilation stage.
> 
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> ---
> V1 -> V2:
> - Rename the sources to have 'test' in the names: 'sbi_unit.c' =>
> 'sbi_unit_test.c', 'sbi_unit.h' => 'sbi_unit_test.h'.
> - Rewrite using the carray functionality instead of placing the
> pointers to the test suites into the source explicitly.
> - Add 'ifdef' into the sbi_unit_test header file so we could
> avoid writing a lot of 'ifdef' in other files.
> - Get rid of unused symbols and macros
> - Change the behaviour of SBIUNIT_ASSERT_* functions: now they trigger
> sbi_panic if the assertion fails.
> - Add a 'SBIUnit' prefix to the SBIUNIT_INFO macro message
> - Rename the sbiunit_test_case structure fields, and use true/false
> instead of 1/0
> - Change the logic of the test fail detection: now we have the 'failed'
> sbiunit_test_case structure field instead of the 'result'. 'failed'
> field gets set to 'true' if an assertion or an expectaion fails.
> - Fix codestyle issues
> - Add 'len' parameter to SBIUNIT_*_STREQ
> 
>  include/sbi/sbi_unit_test.h   | 74 +++++++++++++++++++++++++++++++++++
>  lib/sbi/Kconfig               |  4 ++
>  lib/sbi/objects.mk            |  2 +
>  lib/sbi/sbi_init.c            |  3 ++
>  lib/sbi/sbi_unit_test.c       | 43 ++++++++++++++++++++
>  lib/sbi/sbi_unit_tests.carray |  3 ++
>  6 files changed, 129 insertions(+)
>  create mode 100644 include/sbi/sbi_unit_test.h
>  create mode 100644 lib/sbi/sbi_unit_test.c
>  create mode 100644 lib/sbi/sbi_unit_tests.carray
> 
> diff --git a/include/sbi/sbi_unit_test.h b/include/sbi/sbi_unit_test.h
> new file mode 100644
> index 0000000..1538332
> --- /dev/null
> +++ b/include/sbi/sbi_unit_test.h
> @@ -0,0 +1,74 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Author: Ivan Orlov <ivan.orlov0322@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()

> +};
> +
> +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.

> +		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@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.

> +		s_case->test_func(s_case);
> +		if (s_case->failed)
> +			count_fail++;
> +		else
> +			count_pass++;
> +		sbi_printf("[%s] %s\n", s_case->failed ? "FAILED" : "PASSED",
> +			   s_case->name);
> +		s_case++;
> +	}
> +	sbi_printf("%u PASSED / %u FAILED / %u TOTAL\n", count_pass, count_fail,
> +		   count_pass + count_fail);
> +}
> +
> +void run_all_tests(void)
> +{
> +	u32 i;
> +
> +	sbi_printf("\n# Running SBIUNIT tests #\n");
> +
> +	for (i = 0; i < sbi_unit_tests_size; i++)
> +		run_test_suite(sbi_unit_tests[i]);
> +}
> diff --git a/lib/sbi/sbi_unit_tests.carray b/lib/sbi/sbi_unit_tests.carray
> new file mode 100644
> index 0000000..8d6069b
> --- /dev/null
> +++ b/lib/sbi/sbi_unit_tests.carray
> @@ -0,0 +1,3 @@
> +HEADER: sbi/sbi_unit_test.h
> +TYPE: struct sbiunit_test_suite
> +NAME: sbi_unit_tests
> -- 
> 2.34.1
>

Thanks,
drew

[PATCH v2 3/5] Makefile: clean '.c' files generated by carray

[Andrew Jones]

On Thu, Feb 15, 2024 at 04:16:23PM +0000, Ivan Orlov wrote:
> `make clean` doesn't clear the '*.c' files generated by carray.sh
> in the 'build' directory. Fix it by extending the 'clean' makefile
> target.
> 
> This is the only way we are able to regenerate the carray .c file in
> case if we add a new test to the 'carray-sbi_unit_tests-y' Makefile
> variable. However, running `make clear` every time we add a new test
> is not really convenient, so the mechanics should be changed in the
> future.
> 
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> ---
>  Makefile | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index 680c19a..4519277 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -684,6 +684,8 @@ clean:
>  	$(CMD_PREFIX)find $(build_dir) -type f -name "*.bin" -exec rm -rf {} +
>  	$(if $(V), @echo " RM        $(build_dir)/*.dtb")
>  	$(CMD_PREFIX)find $(build_dir) -type f -name "*.dtb" -exec rm -rf {} +
> +	$(if $(V), @echo " RM        $(build_dir)/*.c")
> +	$(CMD_PREFIX)find $(build_dir) -type f -name "*.c" -exec rm -rf {} +

I realize these *.c files are in $build_dir, so they're supposed to be
generated files, but it still looks scary to remove source files. I
think we should try to be more specific. Maybe we should put all generated
source files in directories named 'generated' or something and then only
remove files in those directories.

Thanks,
drew

>  
>  # Rule for "make distclean"
>  .PHONY: distclean
> -- 
> 2.34.1
> 

[PATCH v2 4/5] lib: tests: Add a test for sbi_bitmap

[Andrew Jones]

On Thu, Feb 15, 2024 at 04:16:24PM +0000, Ivan Orlov wrote:
> Add test suite covering all of the functions from lib/sbi/sbi_bitmap.c:
> __bitmap_and, __bitmap_or and __bitmap_xor.
> 
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> ---
> V1 -> V2:
> - Extract the part of the commit description to the cover letter
> - Update to use the carray functionality instead of placing the pointer
> to the test suite directly into the sources
> - Fix codestyle issues
> - Use the references to the data_* arrays when defining the expected
> test results, instead of repeating the same numbers again and again
> 
>  lib/sbi/objects.mk        |   3 ++
>  lib/sbi/sbi_bitmap_test.c | 103 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 106 insertions(+)
>  create mode 100644 lib/sbi/sbi_bitmap_test.c
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

[PATCH v2 5/5] lib: tests: Add sbi_console test

[Andrew Jones]

On Thu, Feb 15, 2024 at 04:16:25PM +0000, Ivan Orlov wrote:
> Add the test suite covering some of the functions from
> lib/sbi/sbi_console.c: putc, puts and printf. The test covers a variety
> of format specifiers for printf and different strings and characters for
> putc and puts.
> 
> In order to do that, the test "mocks" the sbi_console_device structure
> by setting the 'console_dev' variable to the virtual console.
> 
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> ---
> V1 -> V2:
> - Rewrite using the carray functionality
> - Replace CONSOLE_DO and CONSOLE_DO_RET macros with two inline
> functions: one of them "mocks" the default console device, and
> the second one restores the old console device.
> - Fix codestyle issues (comments, etc.)
> - Remove incorrect 'puts' test
> - Use updated SBIUNIT_ASSERT_STREQ API
> 
>  lib/sbi/objects.mk         |   1 +
>  lib/sbi/sbi_console.c      |   4 ++
>  lib/sbi/sbi_console_test.c | 102 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 107 insertions(+)
>  create mode 100644 lib/sbi/sbi_console_test.c
> 
> diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
> index b4c273f..9d065fa 100644
> --- a/lib/sbi/objects.mk
> +++ b/lib/sbi/objects.mk
> @@ -16,6 +16,7 @@ libsbi-objs-$(CONFIG_SBIUNIT) += sbi_unit_tests.o
>  
>  libsbi-objs-$(CONFIG_SBIUNIT) += sbi_bitmap_test.o
>  carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += bitmap_test_suite
> +carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += console_test_suite
>  
>  libsbi-objs-y += sbi_ecall.o
>  libsbi-objs-y += sbi_ecall_exts.o
> diff --git a/lib/sbi/sbi_console.c b/lib/sbi/sbi_console.c
> index ab09a5c..d1229d0 100644
> --- a/lib/sbi/sbi_console.c
> +++ b/lib/sbi/sbi_console.c
> @@ -488,3 +488,7 @@ int sbi_console_init(struct sbi_scratch *scratch)
>  
>  	return rc;
>  }
> +
> +#ifdef CONFIG_SBIUNIT
> +#include "sbi_console_test.c"
> +#endif
> diff --git a/lib/sbi/sbi_console_test.c b/lib/sbi/sbi_console_test.c
> new file mode 100644
> index 0000000..f24b329
> --- /dev/null
> +++ b/lib/sbi/sbi_console_test.c
> @@ -0,0 +1,102 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
> + */
> +#include <sbi/sbi_unit_test.h>
> +#include <sbi/sbi_heap.h>
> +
> +#define TEST_CONSOLE_BUF_LEN 1024
> +
> +static const struct sbi_console_device *old_dev;
> +static char test_console_buf[TEST_CONSOLE_BUF_LEN];
> +static u32 test_console_buf_pos;
> +
> +static void test_console_putc(char c)
> +{
> +	test_console_buf[test_console_buf_pos] = c;
> +	test_console_buf_pos = (test_console_buf_pos + 1) % TEST_CONSOLE_BUF_LEN;
> +}
> +
> +static void clear_test_console_buf(void)
> +{
> +	test_console_buf_pos = 0;
> +	test_console_buf[0] = '\0';
> +}
> +
> +static const struct sbi_console_device new_dev = {

Please rename 'new_dev' to 'test_console_dev'

> +	.name = "Test console device",
> +	.console_putc = test_console_putc,
> +};
> +
> +/* Mock the console device */
> +static inline void test_console_begin(const struct sbi_console_device *device)
> +{
> +	old_dev = console_dev;
> +	console_dev = device;
> +}
> +
> +static inline void test_console_end(void)
> +{
> +	console_dev = old_dev;
> +}
> +
> +static void putc_test(struct sbiunit_test_case *test)
> +{
> +	clear_test_console_buf();
> +
> +	test_console_begin(&new_dev);
> +	sbi_putc('a');
> +	test_console_end();
> +	SBIUNIT_ASSERT_EQ(test, test_console_buf[0], 'a');
> +}
> +
> +#define PUTS_TEST(test, expected, str) do {			\
> +	clear_test_console_buf();				\
> +	test_console_begin(&new_dev);				\
> +	sbi_puts(str);						\
> +	test_console_end();					\
> +	SBIUNIT_ASSERT_STREQ(test, test_console_buf, expected,	\
> +			     sbi_strlen(expected));		\
> +} while (0)
> +
> +static void puts_test(struct sbiunit_test_case *test)
> +{
> +	PUTS_TEST(test, "Hello, OpenSBI!", "Hello, OpenSBI!");
> +	PUTS_TEST(test, "Hello,\r\nOpenSBI!", "Hello,\nOpenSBI!");
> +}
> +
> +#define PRINTF_TEST(test, expected, format, ...) do {		\
> +	clear_test_console_buf();				\
> +	test_console_begin(&new_dev);				\
> +	size_t __res = sbi_printf(format, ##__VA_ARGS__);	\
> +	test_console_end();					\
> +	SBIUNIT_ASSERT_EQ(test, __res, sbi_strlen(expected));	\
> +	SBIUNIT_ASSERT_STREQ(test, test_console_buf, expected,	\
> +			     sbi_strlen(expected));		\
> +} while (0)
> +
> +static void printf_test(struct sbiunit_test_case *test)
> +{
> +	PRINTF_TEST(test, "Hello", "Hello");
> +	PRINTF_TEST(test, "3 5 7", "%d %d %d", 3, 5, 7);
> +	PRINTF_TEST(test, "Hello", "%s", "Hello");
> +	PRINTF_TEST(test, "-1", "%d", -1);
> +	PRINTF_TEST(test, "FF", "%X", 255);
> +	PRINTF_TEST(test, "ff", "%x", 255);
> +	PRINTF_TEST(test, "A", "%c", 'A');
> +	PRINTF_TEST(test, "1fe", "%p", (void *)0x1fe);
> +	PRINTF_TEST(test, "4294967295", "%u", 4294967295U);
> +	PRINTF_TEST(test, "-2147483647", "%ld", -2147483647l);
> +	PRINTF_TEST(test, "-9223372036854775807", "%lld", -9223372036854775807LL);
> +	PRINTF_TEST(test, "18446744073709551615", "%llu", 18446744073709551615ULL);
> +}
> +
> +static struct sbiunit_test_case console_test_cases[] = {
> +	SBIUNIT_TEST_CASE(putc_test),
> +	SBIUNIT_TEST_CASE(puts_test),
> +	SBIUNIT_TEST_CASE(printf_test),
> +	{},
> +};
> +
> +SBIUNIT_TEST_SUITE(console_test_suite, console_test_cases);
> -- 
> 2.34.1
>

Other than the rename

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

[PATCH v2 1/5] docs: Add documentation about tests and SBIUnit

[Ivan Orlov]

On 2/28/24 13:47, Andrew Jones wrote:
> On Thu, Feb 15, 2024 at 04:16:21PM +0000, Ivan Orlov wrote:
>> This patch contains the documentation for SBIUnit. It describes:
>>
>> - What is SBIUnit
>> - Simple test writing scenario
>> - How we can cover static functions
>> - How we can "mock" structures in order to test the functions which
>> operate on them
>> - SBIUnit API Reference
>>
>> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
>> ---
>> V1 -> V2:
>> - Use more appropriate language
>> - Update the documentation in accordance with SBIUnit API and
>> structure changes
>> - Remove the redundant documentation for every SBIUNIT_* macro.
>> - Make the documentation follow the 80-characters rule.
>>
>>   docs/writing_tests.md | 131 ++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 131 insertions(+)
>>   create mode 100644 docs/writing_tests.md
>>
>> diff --git a/docs/writing_tests.md b/docs/writing_tests.md
>> new file mode 100644
>> index 0000000..279d35b
>> --- /dev/null
>> +++ b/docs/writing_tests.md
>> @@ -0,0 +1,131 @@
>> +Writing tests for OpenSBI
>> +=========================
>> +
>> +SBIUnit
>> +-------
>> +SBIUnit is a set of macros and functions which simplify the test development and
>> +automate the test execution and evaluation. All of the SBIUnit definitions are
>> +in the `include/sbi/sbi_unit_test.h` header file, and implementations are
>> +available in `lib/sbi/sbi_unit_test.c`.
>> +
>> +Simple SBIUnit test
>> +-------------------
>> +
>> +For instance, we would like to test the following function from
>> +`lib/sbi/sbi_string.c`:
>> +
>> +```c
>> +size_t sbi_strlen(const char *str)
>> +{
>> +	unsigned long ret = 0;
>> +
>> +	while (*str != '\0') {
>> +		ret++;
>> +		str++;
>> +	}
>> +
>> +	return ret;
>> +}
>> +```
>> +
>> +Which calculates the string length.
> 
> Lowercase 'w' in which, since this line is a continuation of the sentence
> started above.
> 
>> +
>> +Create the file `lib/sbi/sbi_string_test.c` with the following content:
>> +
>> +```c
>> +#include <sbi/sbi_unit_test.h>
>> +#include <sbi/sbi_string.h>
>> +
>> +static void strlen_test(struct sbiunit_test_case *test)
>> +{
>> +	SBIUNIT_EXPECT_EQ(test, sbi_strlen("Hello"), 5);
>> +	SBIUNIT_EXPECT_EQ(test, sbi_strlen("Hell\0o"), 4);
>> +}
>> +
>> +static struct sbiunit_test_case string_test_cases[] = {
>> +	SBIUNIT_TEST_CASE(strlen_test),
>> +	{},
>> +};
>> +
>> +SBIUNIT_TEST_SUITE(string_test_suite, string_test_cases);
>> +```
>> +
>> +Then, add the corresponding Makefile entries to `lib/sbi/objects.mk`:
>> +```lang-makefile
>> +...
>> +libsbi-objs-$(CONFIG_SBIUNIT) += sbi_string_test.o
>> +carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += string_test_suite
>> +```
>> +
>> +Run `make clean` in order to regenerate the carray-related files, recompile
>> +OpenSBI with CONFIG_SBIUNIT option enabled and run it in the QEMU.
>> +You will see something like this:
>> +```
>> +# make PLATFORM=generic run
>> +...
>> +# Running SBIUNIT tests #
>> +...
>> +## Running test suite: string_test_suite
>> +[PASSED] strlen_test
>> +1 PASSED / 0 FAILED / 1 TOTAL
>> +```
>> +
>> +Now let's try to change this test in the way that it will fail:
>> +
>> +```c
>> +- SBIUNIT_EXPECT_EQ(test, sbi_strlen("Hello"), 5);
>> ++ SBIUNIT_EXPECT_EQ(test, sbi_strlen("Hello"), 100);
>> +```
>> +
>> +`make all` and `make run` it again:
>> +```
>> +...
>> +# Running SBIUNIT tests #
>> +...
>> +## Running test suite: string_test_suite
>> +[SBIUnit] [.../opensbi/lib/sbi/sbi_string_test.c:6]: strlen_test: Condition
>> +"(sbi_strlen("Hello")) == (100)" expected to be true!
> 
> If the test will output the above all on one line, then the document
> should also use a single line even though it will exceed 80 chars.
> 
>> +[FAILED] strlen_test
>> +0 PASSED / 1 FAILED / 1 TOTAL
>> +```
>> +Covering the static functions / using the static definitions
>> +------------------------------------------------------------
>> +
>> +SBIUnit also allows you to test static functions. In order to do so, simply
>> +include your test source in the file you would like to test. Complementing the
>> +example above, just add this to the
>> +`lib/sbi/sbi_string.c` file:
>> +
>> +```c
>> +#ifdef CONFIG_SBIUNIT
>> +#include "sbi_string_test.c"
>> +#endif
>> +```
>> +
>> +In this case you should only add a new carray entry pointing to the test suite
>> +to `lib/sbi/objects.mk`:
>> +```lang-makefile
>> +...
>> +carray-sbi_unit_tests-$(CONFIG_SBIUNIT) += string_test_suite
>> +```
>> +
>> +You don't have to compile the `sbi_string_test.o` separately, because the
>> +test code will be included into the `sbi_string` object file.
>> +
>> +See example in `lib/sbi/sbi_console_test.c`, where statically declared
>> +`console_dev` variable is used to mock the `sbi_console_device` structure.
>> +
>> +"Mocking" the structures
>> +------------------------
>> +See the example of structure "mocking" in the `lib/sbi/sbi_console_test.c`,
>> +where the sbi_console_device structure was mocked to be used in various
>> +console-related functions in order to test them.
>> +
>> +API Reference
>> +-------------
>> +All of the `SBIUNIT_EXPECT_*` macros will cause a test case to fail if the
>> +corresponding conditions are not met, however, the execution of a particular
>> +test case will not be stopped.
>> +
>> +All of the `SBIUNIT_ASSERT_*` macros will cause a test case to fail and stop
>> +immediately, triggering the panic.
> 
> s/the panic/a panic/
> 
>> -- 
>> 2.34.1
>>
> 
> Other than the nits,
> 

Hi Andrew,

Thank you so much for the review. I'll make the suggested changes and 
send the V3.

-- 
Kind regards,
Ivan Orlov

[PATCH v2 2/5] lib: Add SBIUnit testing macros and functions

[Ivan Orlov]

On 2/28/24 14:19, Andrew Jones wrote:

>> + * Author: Ivan Orlov <ivan.orlov0322@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@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...

Thank you for the review!

-- 
Kind regards,
Ivan Orlov

[PATCH v2 3/5] Makefile: clean '.c' files generated by carray

[Ivan Orlov]

On 2/28/24 14:23, Andrew Jones wrote:
> On Thu, Feb 15, 2024 at 04:16:23PM +0000, Ivan Orlov wrote:
>> `make clean` doesn't clear the '*.c' files generated by carray.sh
>> in the 'build' directory. Fix it by extending the 'clean' makefile
>> target.
>>
>> This is the only way we are able to regenerate the carray .c file in
>> case if we add a new test to the 'carray-sbi_unit_tests-y' Makefile
>> variable. However, running `make clear` every time we add a new test
>> is not really convenient, so the mechanics should be changed in the
>> future.
>>
>> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
>> ---
>>   Makefile | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/Makefile b/Makefile
>> index 680c19a..4519277 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -684,6 +684,8 @@ clean:
>>   	$(CMD_PREFIX)find $(build_dir) -type f -name "*.bin" -exec rm -rf {} +
>>   	$(if $(V), @echo " RM        $(build_dir)/*.dtb")
>>   	$(CMD_PREFIX)find $(build_dir) -type f -name "*.dtb" -exec rm -rf {} +
>> +	$(if $(V), @echo " RM        $(build_dir)/*.c")
>> +	$(CMD_PREFIX)find $(build_dir) -type f -name "*.c" -exec rm -rf {} +
> 
> I realize these *.c files are in $build_dir, so they're supposed to be
> generated files, but it still looks scary to remove source files. I
> think we should try to be more specific. Maybe we should put all generated
> source files in directories named 'generated' or something and then only
> remove files in those directories.
> 

Hmm, maybe it would be better to do this in the separate patch series? I 
was thinking about upgrading carrays in general: now the generated .c 
files don't get updated when we change the corresponding Makefile 
variable. It produces inconvenience and these variables should be 
tracked somehow.

For instance, we could possibly move the carray entries to the separate 
files instead of keeping them in the Makefile variables: this way they 
will be tracked by 'make'.

-- 
Kind regards,
Ivan Orlov

[PATCH v2 2/5] lib: Add SBIUnit testing macros and functions

[Andrew Jones]

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@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@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
> 

[PATCH v2 3/5] Makefile: clean '.c' files generated by carray

[Andrew Jones]

On Wed, Feb 28, 2024 at 04:18:28PM +0000, Ivan Orlov wrote:
> On 2/28/24 14:23, Andrew Jones wrote:
> > On Thu, Feb 15, 2024 at 04:16:23PM +0000, Ivan Orlov wrote:
> > > `make clean` doesn't clear the '*.c' files generated by carray.sh
> > > in the 'build' directory. Fix it by extending the 'clean' makefile
> > > target.
> > > 
> > > This is the only way we are able to regenerate the carray .c file in
> > > case if we add a new test to the 'carray-sbi_unit_tests-y' Makefile
> > > variable. However, running `make clear` every time we add a new test
> > > is not really convenient, so the mechanics should be changed in the
> > > future.
> > > 
> > > Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> > > ---
> > >   Makefile | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/Makefile b/Makefile
> > > index 680c19a..4519277 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -684,6 +684,8 @@ clean:
> > >   	$(CMD_PREFIX)find $(build_dir) -type f -name "*.bin" -exec rm -rf {} +
> > >   	$(if $(V), @echo " RM        $(build_dir)/*.dtb")
> > >   	$(CMD_PREFIX)find $(build_dir) -type f -name "*.dtb" -exec rm -rf {} +
> > > +	$(if $(V), @echo " RM        $(build_dir)/*.c")
> > > +	$(CMD_PREFIX)find $(build_dir) -type f -name "*.c" -exec rm -rf {} +
> > 
> > I realize these *.c files are in $build_dir, so they're supposed to be
> > generated files, but it still looks scary to remove source files. I
> > think we should try to be more specific. Maybe we should put all generated
> > source files in directories named 'generated' or something and then only
> > remove files in those directories.
> > 
> 
> Hmm, maybe it would be better to do this in the separate patch series? I was
> thinking about upgrading carrays in general: now the generated .c files
> don't get updated when we change the corresponding Makefile variable. It
> produces inconvenience and these variables should be tracked somehow.

Yes, a separate series is a good idea. For this series, I'd just make the
new carrays behave like the rest, even if that behavior isn't ideal.

Thanks,
drew

> 
> For instance, we could possibly move the carray entries to the separate
> files instead of keeping them in the Makefile variables: this way they will
> be tracked by 'make'.
> 
> -- 
> Kind regards,
> Ivan Orlov
> 

[PATCH v2 2/5] lib: Add SBIUnit testing macros and functions

[Ivan Orlov]

On 2/28/24 16:30, Andrew Jones wrote:

> If a no-name test is OK, then the stop condition should be
> s_case->test_func being NULL.
> 

Yeah, I guess we can check the test_func. Thanks!

Also, I believe it would be good to mention that we need the last test 
cases array entry to be empty in the documentation, so it will be more 
explicit for a developer. I'll do that in the V3 if it is OK with you.

-- 
Kind regards,
Ivan Orlov

[PATCH v2 3/5] Makefile: clean '.c' files generated by carray

[Ivan Orlov]

On 2/28/24 16:32, Andrew Jones wrote:
> On Wed, Feb 28, 2024 at 04:18:28PM +0000, Ivan Orlov wrote:
>> On 2/28/24 14:23, Andrew Jones wrote:
>>> On Thu, Feb 15, 2024 at 04:16:23PM +0000, Ivan Orlov wrote:
>>>> `make clean` doesn't clear the '*.c' files generated by carray.sh
>>>> in the 'build' directory. Fix it by extending the 'clean' makefile
>>>> target.
>>>>
>>>> This is the only way we are able to regenerate the carray .c file in
>>>> case if we add a new test to the 'carray-sbi_unit_tests-y' Makefile
>>>> variable. However, running `make clear` every time we add a new test
>>>> is not really convenient, so the mechanics should be changed in the
>>>> future.
>>>>
>>>> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
>>>> ---
>>>>    Makefile | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/Makefile b/Makefile
>>>> index 680c19a..4519277 100644
>>>> --- a/Makefile
>>>> +++ b/Makefile
>>>> @@ -684,6 +684,8 @@ clean:
>>>>    	$(CMD_PREFIX)find $(build_dir) -type f -name "*.bin" -exec rm -rf {} +
>>>>    	$(if $(V), @echo " RM        $(build_dir)/*.dtb")
>>>>    	$(CMD_PREFIX)find $(build_dir) -type f -name "*.dtb" -exec rm -rf {} +
>>>> +	$(if $(V), @echo " RM        $(build_dir)/*.c")
>>>> +	$(CMD_PREFIX)find $(build_dir) -type f -name "*.c" -exec rm -rf {} +
>>>
>>> I realize these *.c files are in $build_dir, so they're supposed to be
>>> generated files, but it still looks scary to remove source files. I
>>> think we should try to be more specific. Maybe we should put all generated
>>> source files in directories named 'generated' or something and then only
>>> remove files in those directories.
>>>
>>
>> Hmm, maybe it would be better to do this in the separate patch series? I was
>> thinking about upgrading carrays in general: now the generated .c files
>> don't get updated when we change the corresponding Makefile variable. It
>> produces inconvenience and these variables should be tracked somehow.
> 
> Yes, a separate series is a good idea. For this series, I'd just make the
> new carrays behave like the rest, even if that behavior isn't ideal.
> 

Alright, then I'll drop this patch by now and mention in the docs that 
you may need to manually remove autogenerated .c files in order to make 
the new test work.

Thanks!

-- 
Kind regards,
Ivan Orlov

[PATCH v2 2/5] lib: Add SBIUnit testing macros and functions

[Andrew Jones]

On Wed, Feb 28, 2024 at 04:43:59PM +0000, Ivan Orlov wrote:
> On 2/28/24 16:30, Andrew Jones wrote:
> 
> > If a no-name test is OK, then the stop condition should be
> > s_case->test_func being NULL.
> > 
> 
> Yeah, I guess we can check the test_func. Thanks!
> 
> Also, I believe it would be good to mention that we need the last test cases
> array entry to be empty in the documentation, so it will be more explicit
> for a developer. I'll do that in the V3 if it is OK with you.
>

Sure. You can define an 'END_CASE = { }' type of macro and put it in the
document's code examples. That'd probably be enough, but you could also
add a sentence to explicitly state it's required too.

Thanks,
drew

[PATCH v2 2/5] lib: Add SBIUnit testing macros and functions

[Ivan Orlov]

On 2/28/24 17:00, Andrew Jones wrote:
> On Wed, Feb 28, 2024 at 04:43:59PM +0000, Ivan Orlov wrote:
>> On 2/28/24 16:30, Andrew Jones wrote:
>>
>>> If a no-name test is OK, then the stop condition should be
>>> s_case->test_func being NULL.
>>>
>>
>> Yeah, I guess we can check the test_func. Thanks!
>>
>> Also, I believe it would be good to mention that we need the last test cases
>> array entry to be empty in the documentation, so it will be more explicit
>> for a developer. I'll do that in the V3 if it is OK with you.
>>
> 
> Sure. You can define an 'END_CASE = { }' type of macro and put it in the
> document's code examples. That'd probably be enough, but you could also
> add a sentence to explicitly state it's required too.
> 

Great, thanks! Perhaps I could define it as SBIUNIT_END_CASE macro and 
use in other tests as well.
-- 
Kind regards,
Ivan Orlov