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

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

[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 and the subsequent patches in this series introduce SBIUnit - the
set of macros and functions which simplify the unit test development
for OpenSBI and automate tests execution and evaluation. Also, this
patch series contains two of the tests: one which covers functions from
lib/sbi_bitmap.c, and another covering a part of functions from
lib/sbi_console.c.

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

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. I believe it would be good to
have such a thing in OpenSBI as well.

Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
---
 docs/writing_tests.md | 143 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 143 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..481f2ee
--- /dev/null
+++ b/docs/writing_tests.md
@@ -0,0 +1,143 @@
+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 could be found in
+`include/sbi/sbi_unit.h` header file, and implementations are available in `lib/sbi/sbi_unit.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;
+}
+```
+
+Apparently, it calculates the string length.
+
+Create the file `lib/sbi/sbi_string_test.c` with the following content:
+
+```c
+#include <sbi/sbi_unit.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);
+```
+
+After that, add the corresponding entry to `lib/sbi/sbi_unit.c` and update the `test_suites` array:
+```c
+...
+extern struct sbiunit_test_suite string_test_suite;
+...
+static struct sbiunit_test_suite *test_suites[] = {
+    ...
+    &string_test_suite,
+};
+...
+```
+
+Add the corresponding Makefile entry to `lib/sbi/objects.mk`:
+```lang-makefile
+...
+libsbi-objs-$(CONFIG_SBIUNIT) += sbi_string_test.o
+```
+
+Now 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
+[OK] strlen_test
+1 SUCCESS / 0 FAIL / 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);
+```
+
+Compile and run it again:
+```
+...
+# Running SBIUNIT tests #
+...
+## Running test suite: string_test_suite
+strlen_test: Condition "sbi_strlen("Hello") == 100" expected to be true!
+[FAIL] strlen_test
+0 SUCCESS / 1 FAIL / 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 not add a new entry to `lib/sbi/objects.mk`, 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.
+
+- `SBIUNIT_EXPECT(test_case, condition)` - sets an expectation that 'condition' is true
+- `SBIUNIT_ASSERT(test_case, condition)` - sets an assertion that 'condition' is true
+- `SBIUNIT_EXPECT_EQ(test_case, a, b)` - sets an expectation that a = b
+- `SBIUNIT_ASSERT_EQ(test_case, a, b)` - sets an assertion that a = b
+- `SBIUNIT_EXPECT_NE(test_case, a, b)` - sets an expectation that a != b
+- `SBIUNIT_ASSERT_NE(test_case, a, b)` - sets an assertion that a != b
+- `SBIUNIT_EXPECT_MEMEQ(test_case, a, b, len)` - performs sbi_memcmp on memory regions a and b with
+a length 'len', and sets an expectation that they are equal
+- `SBIUNIT_ASSERT_MEMEQ(test_case, a, b, len)` - performs sbi_memcmp on memory regions a and b with
+a length 'len', and sets an assertion that they are equal
+- `SBIUNIT_EXPECT_STREQ(test_case, a, b)` - performs sbi_strcmp on strings a and b and sets
+an expectation that they are equal
+- `SBIUNIT_ASSERT_STREQ(test_case, a, b)` - performs sbi_strcmp on strings a and b and sets
+an assertion that they are equal
-- 
2.34.1

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

[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 and the subsequent patches in this series introduce SBIUnit - the
set of macros and functions which simplify the unit test development for
OpenSBI and automate tests execution and evaluation.

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 should be excluded completely on the
compilation stage (and, apparently, it works in this way).

Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
---
 include/sbi/sbi_unit.h | 69 ++++++++++++++++++++++++++++++++++++++++++
 lib/sbi/Kconfig        |  4 +++
 lib/sbi/objects.mk     |  1 +
 lib/sbi/sbi_init.c     |  8 +++++
 lib/sbi/sbi_unit.c     | 44 +++++++++++++++++++++++++++
 5 files changed, 126 insertions(+)
 create mode 100644 include/sbi/sbi_unit.h
 create mode 100644 lib/sbi/sbi_unit.c

diff --git a/include/sbi/sbi_unit.h b/include/sbi/sbi_unit.h
new file mode 100644
index 0000000..685e58d
--- /dev/null
+++ b/include/sbi/sbi_unit.h
@@ -0,0 +1,69 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
+ */
+#ifndef __SBI_UNIT_H__
+#define __SBI_UNIT_H__
+
+extern struct sbiunit_test_suite *__start_sbiunit_test_suites;
+extern struct sbiunit_test_suite *__end_sbiunit_test_suites;
+
+#include <sbi/sbi_types.h>
+#include <sbi/sbi_console.h>
+#include <sbi/sbi_string.h>
+
+#define _CONCAT(a, b) a ## b
+#define CONCAT(a, b) _CONCAT(a, b)
+
+struct sbiunit_test_case {
+	char *name;
+	bool should_run;
+	bool result;
+	void (*test_func)(struct sbiunit_test_case *test);
+	void (*onerr)(struct sbiunit_test_case *test);
+};
+
+struct sbiunit_test_suite {
+	struct sbiunit_test_case *cases;
+	const char *name;
+};
+
+#define SBIUNIT_TEST_CASE(func)	\
+	{ .name = #func, .should_run = 1, .result = 0, .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_INFO(test, msg) sbi_printf("%s: %s", test->name, msg)
+
+#define SBIUNIT_EXPECT(test, cond) do {							\
+	if (!(cond)) {									\
+		test->result = 0;							\
+		SBIUNIT_INFO(test, "Condition \"" #cond "\" expected to be true!\n");	\
+	}										\
+} while (0)
+
+#define SBIUNIT_ASSERT(test, cond) do {						\
+	if (!(cond)) {								\
+		test->result = 0;						\
+		SBIUNIT_INFO(test, "Condition \"" #cond "\" must be true!\n");	\
+		return;								\
+	}									\
+} 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) == 0)
+#define SBIUNIT_ASSERT_MEMEQ(test, a, b, len) \
+	SBIUNIT_ASSERT(test, sbi_memcmp(a, b, len) == 0)
+#define SBIUNIT_EXPECT_STREQ(test, a, b) \
+	SBIUNIT_EXPECT(test, sbi_strcmp(a, b) == 0)
+#define SBIUNIT_ASSERT_STREQ(test, a, b) \
+	SBIUNIT_ASSERT(test, sbi_strcmp(a, b) == 0)
+
+void run_all_tests(void);
+#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..0a2318b 100644
--- a/lib/sbi/objects.mk
+++ b/lib/sbi/objects.mk
@@ -11,6 +11,7 @@ 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-y += sbi_unit.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..16d0a00 100644
--- a/lib/sbi/sbi_init.c
+++ b/lib/sbi/sbi_init.c
@@ -30,6 +30,10 @@
 #include <sbi/sbi_tlb.h>
 #include <sbi/sbi_version.h>
 
+#ifdef CONFIG_SBIUNIT
+#include <sbi/sbi_unit.h>
+#endif
+
 #define BANNER                                              \
 	"   ____                    _____ ____ _____\n"     \
 	"  / __ \\                  / ____|  _ \\_   _|\n"  \
@@ -398,6 +402,10 @@ static void __noreturn init_coldboot(struct sbi_scratch *scratch, u32 hartid)
 
 	sbi_boot_print_hart(scratch, hartid);
 
+#ifdef CONFIG_SBIUNIT
+	run_all_tests();
+#endif
+
 	/*
 	 * 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.c b/lib/sbi/sbi_unit.c
new file mode 100644
index 0000000..9c7efec
--- /dev/null
+++ b/lib/sbi/sbi_unit.c
@@ -0,0 +1,44 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
+ */
+#include <sbi/sbi_unit.h>
+#include <sbi/sbi_console.h>
+
+static struct sbiunit_test_suite *test_suites[] = {
+};
+
+static void run_test_suite(struct sbiunit_test_suite *suite)
+{
+	struct sbiunit_test_case *s_case;
+	u32 count_pass, count_fail;
+
+	sbi_printf("## Running test suite: %s\n", suite->name);
+	count_pass = 0;
+	count_fail = 0;
+
+	s_case = suite->cases;
+	while (s_case->should_run) {
+		s_case->result = 1;
+		s_case->test_func(s_case);
+		if (s_case->result)
+			count_pass++;
+		else
+			count_fail++;
+		sbi_printf("[%s] %s\n", s_case->result ? "OK" : "FAIL", s_case->name);
+		s_case++;
+	}
+	sbi_printf("%u SUCCESS / %u FAIL / %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 < sizeof(test_suites) / sizeof(test_suites[0]); i++)
+		run_test_suite(test_suites[i]);
+}
-- 
2.34.1

[PATCH 3/4] 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.

This patch depends on the previous patches in this series as it uses
SBIUnit macros, functions and definitions

Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
---
 lib/sbi/objects.mk        |   2 +
 lib/sbi/sbi_bitmap_test.c | 104 ++++++++++++++++++++++++++++++++++++++
 lib/sbi/sbi_unit.c        |   2 +
 3 files changed, 108 insertions(+)
 create mode 100644 lib/sbi/sbi_bitmap_test.c

diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
index 0a2318b..b29e63f 100644
--- a/lib/sbi/objects.mk
+++ b/lib/sbi/objects.mk
@@ -56,6 +56,8 @@ libsbi-objs-$(CONFIG_SBI_ECALL_VENDOR) += sbi_ecall_vendor.o
 carray-sbi_ecall_exts-$(CONFIG_SBI_ECALL_DBTR) += ecall_dbtr
 libsbi-objs-$(CONFIG_SBI_ECALL_DBTR) += sbi_ecall_dbtr.o
 
+libsbi-objs-$(CONFIG_SBIUNIT) += sbi_bitmap_test.o
+
 libsbi-objs-y += sbi_bitmap.o
 libsbi-objs-y += sbi_bitops.o
 libsbi-objs-y += sbi_console.o
diff --git a/lib/sbi/sbi_bitmap_test.c b/lib/sbi/sbi_bitmap_test.c
new file mode 100644
index 0000000..0fd0091
--- /dev/null
+++ b/lib/sbi/sbi_bitmap_test.c
@@ -0,0 +1,104 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
+ */
+#include <sbi/sbi_bitmap.h>
+#include <sbi/sbi_unit.h>
+#include <sbi/sbi_console.h>
+
+static u64 data_a[] = { 0xDEADBEEF, 0x00BAB10C, 0x1BADB002, 0xABADBABE };
+static u64 data_b[] = { 0xC00010FF, 0x00BAB10C, 0xBAAAAAAD, 0xBADDCAFE };
+static u64 data_zero[] = { 0, 0, 0, 0 };
+
+
+#define DATA_SIZE sizeof(data_zero)
+#define DATA_BIT_SIZE (DATA_SIZE * 8)
+
+static void bitmap_and_test(struct sbiunit_test_case *test)
+{
+	u64 res[DATA_SIZE];
+	u64 a_and_b[] = { 0xDEADBEEF & 0xC00010FF, 0x00BAB10C & 0x00BAB10C,
+			  0x1BADB002 & 0xBAAAAAAD, 0xABADBABE & 0xBADDCAFE };
+
+	__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[] = { 0xDEADBEEF | 0xC00010FF, 0x00BAB10C | 0x00BAB10C,
+		       0x1BADB002 | 0xBAAAAAAD, 0xABADBABE | 0xBADDCAFE };
+
+	__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[] = { 0xDEADBEEF ^ 0xC00010FF, 0x00BAB10C ^ 0x00BAB10C,
+			  0x1BADB002 ^ 0xBAAAAAAD, 0xABADBABE ^ 0xBADDCAFE };
+
+	__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);
diff --git a/lib/sbi/sbi_unit.c b/lib/sbi/sbi_unit.c
index 9c7efec..2d3da7f 100644
--- a/lib/sbi/sbi_unit.c
+++ b/lib/sbi/sbi_unit.c
@@ -6,7 +6,9 @@
 #include <sbi/sbi_unit.h>
 #include <sbi/sbi_console.h>
 
+extern struct sbiunit_test_suite bitmap_test_suite;
 static struct sbiunit_test_suite *test_suites[] = {
+	&bitmap_test_suite,
 };
 
 static void run_test_suite(struct sbiunit_test_suite *suite)
-- 
2.34.1

[PATCH 4/4] 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.

This patch depends on the previous patches from the series as it uses
SBIUnit macros, functions and definitions.

Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
---
 lib/sbi/sbi_console.c      |   4 ++
 lib/sbi/sbi_console_test.c | 104 +++++++++++++++++++++++++++++++++++++
 lib/sbi/sbi_unit.c         |   2 +
 3 files changed, 110 insertions(+)
 create mode 100644 lib/sbi/sbi_console_test.c

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..8c3b215
--- /dev/null
+++ b/lib/sbi/sbi_console_test.c
@@ -0,0 +1,104 @@
+/*
+ * SPDX-License-Identifier: BSD-2-Clause
+ *
+ * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
+ */
+#include <sbi/sbi_unit.h>
+#include <sbi/sbi_heap.h>
+
+/*
+ * Console functions are wrapped in order to mock the console object and don't affect the actual
+ * console output
+ */
+#define CONSOLE_DO(action) ({		\
+	old_dev = console_dev;		\
+	console_dev = &new_dev;		\
+	action;				\
+	console_dev = old_dev;		\
+})
+
+// We are using a GCC extension here, which allows us to return a value from a block
+#define CONSOLE_DO_RET(action) ({	\
+	old_dev = console_dev;		\
+	console_dev = &new_dev;		\
+	u64 res = action;		\
+	console_dev = old_dev;		\
+	res;				\
+})
+
+#define BUF_LEN 1024
+
+static const struct sbi_console_device *old_dev;
+
+static char buf[BUF_LEN];
+static u32 pos;
+static void test_console_putc(char c)
+{
+	buf[pos] = c;
+	pos = (pos + 1) % BUF_LEN;
+}
+
+static void clear_buf(void)
+{
+	pos = 0;
+	sbi_memset(buf, 0, BUF_LEN);
+}
+
+// Mocking the console
+static const struct sbi_console_device new_dev = {
+	.name = "Test console device",
+	.console_putc = test_console_putc,
+};
+
+static void putc_test(struct sbiunit_test_case *test)
+{
+	clear_buf();
+
+	CONSOLE_DO(sbi_putc('a'));
+	SBIUNIT_ASSERT_EQ(test, buf[0], 'a');
+}
+
+#define PUTS_TEST(test, expected, param) do {				\
+	clear_buf();							\
+	CONSOLE_DO(sbi_puts(param));					\
+	SBIUNIT_ASSERT_STREQ(test, buf, 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!");
+	PUTS_TEST(test, "Hello,", "Hello,\0OpenSBI!");
+}
+
+#define PRINTF_TEST(test, expected, format, ...) do {					\
+	clear_buf();									\
+	SBIUNIT_ASSERT_EQ(test, CONSOLE_DO_RET(sbi_printf(format, ##__VA_ARGS__)),	\
+			  sbi_strlen(expected));					\
+	SBIUNIT_ASSERT_STREQ(test, buf, 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);
diff --git a/lib/sbi/sbi_unit.c b/lib/sbi/sbi_unit.c
index 2d3da7f..b2f2063 100644
--- a/lib/sbi/sbi_unit.c
+++ b/lib/sbi/sbi_unit.c
@@ -7,8 +7,10 @@
 #include <sbi/sbi_console.h>
 
 extern struct sbiunit_test_suite bitmap_test_suite;
+extern struct sbiunit_test_suite console_test_suite;
 static struct sbiunit_test_suite *test_suites[] = {
 	&bitmap_test_suite,
+	&console_test_suite,
 };
 
 static void run_test_suite(struct sbiunit_test_suite *suite)
-- 
2.34.1

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

[Andrew Jones]

On Thu, Feb 08, 2024 at 09:50:47AM +0000, Ivan Orlov wrote:
> 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 and the subsequent patches in this series introduce SBIUnit - the
> set of macros and functions which simplify the unit test development
> for OpenSBI and automate tests execution and evaluation. Also, this
> patch series contains two of the tests: one which covers functions from
> lib/sbi_bitmap.c, and another covering a part of functions from
> lib/sbi_console.c.

The above should be in the cover letter, which this series is missing.
Cover letters are nice since they give reviewers a place to comment on the
overall series.

> 
> 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
> 
> 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. I believe it would be good to
> have such a thing in OpenSBI as well.

The above paragraph belongs in the cover letter along with elaboration
on how only certain parts of KUnit were reimplemented and why those
certain parts, and not others, were chosen.

> 
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> ---
>  docs/writing_tests.md | 143 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 143 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..481f2ee
> --- /dev/null
> +++ b/docs/writing_tests.md
> @@ -0,0 +1,143 @@
> +Writing tests for OpenSBI
> +=========================
> +
> +SBIUnit
> +-------
> +SBIUnit is a set of macros and functions which simplify the test development and automate the

We aim for 80 char line length, especially in documentation.

> +test execution and evaluation. All of the SBIUnit definitions could be found in

s/could be found in/are in the/


> +`include/sbi/sbi_unit.h` header file, and implementations are available in `lib/sbi/sbi_unit.c`.

I think the string 'test' should be somewhere in the filenames. 'unit' is
too generic.

> +
> +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;
> +}
> +```
> +
> +Apparently, it calculates the string length.

s/Apparently, it/which/

> +
> +Create the file `lib/sbi/sbi_string_test.c` with the following content:
> +
> +```c
> +#include <sbi/sbi_unit.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);
> +```
> +
> +After that, add the corresponding entry to `lib/sbi/sbi_unit.c` and update the `test_suites` array:

s/After that,/Then,/

But I think we should be able to add the test suite pointer to an elf
section with the SBIUNIT_TEST_SUITE() macro to avoid this step.

> +```c
> +...
> +extern struct sbiunit_test_suite string_test_suite;
> +...
> +static struct sbiunit_test_suite *test_suites[] = {
> +    ...
> +    &string_test_suite,
> +};
> +...
> +```
> +
> +Add the corresponding Makefile entry to `lib/sbi/objects.mk`:
> +```lang-makefile
> +...
> +libsbi-objs-$(CONFIG_SBIUNIT) += sbi_string_test.o
> +```
> +
> +Now 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
> +[OK] strlen_test
> +1 SUCCESS / 0 FAIL / 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);
> +```
> +
> +Compile and run it again:
> +```
> +...
> +# Running SBIUNIT tests #
> +...
> +## Running test suite: string_test_suite
> +strlen_test: Condition "sbi_strlen("Hello") == 100" expected to be true!
> +[FAIL] strlen_test
> +0 SUCCESS / 1 FAIL / 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 not add a new entry to `lib/sbi/objects.mk`, 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.

The distinction between SBIUNIT_EXPECT* and SBIUNIT_ASSERT* is good to
document, but I don't think we want to list the functions as is done
below. We should just reference the header from here, since we'll be
lucky to keep the header filename in sync, let alone each function...

> +
> +- `SBIUNIT_EXPECT(test_case, condition)` - sets an expectation that 'condition' is true
> +- `SBIUNIT_ASSERT(test_case, condition)` - sets an assertion that 'condition' is true
> +- `SBIUNIT_EXPECT_EQ(test_case, a, b)` - sets an expectation that a = b
> +- `SBIUNIT_ASSERT_EQ(test_case, a, b)` - sets an assertion that a = b
> +- `SBIUNIT_EXPECT_NE(test_case, a, b)` - sets an expectation that a != b
> +- `SBIUNIT_ASSERT_NE(test_case, a, b)` - sets an assertion that a != b
> +- `SBIUNIT_EXPECT_MEMEQ(test_case, a, b, len)` - performs sbi_memcmp on memory regions a and b with
> +a length 'len', and sets an expectation that they are equal
> +- `SBIUNIT_ASSERT_MEMEQ(test_case, a, b, len)` - performs sbi_memcmp on memory regions a and b with
> +a length 'len', and sets an assertion that they are equal
> +- `SBIUNIT_EXPECT_STREQ(test_case, a, b)` - performs sbi_strcmp on strings a and b and sets
> +an expectation that they are equal
> +- `SBIUNIT_ASSERT_STREQ(test_case, a, b)` - performs sbi_strcmp on strings a and b and sets
> +an assertion that they are equal

Thanks,
drew

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

[Andrew Jones]

On Thu, Feb 08, 2024 at 09:50:48AM +0000, Ivan Orlov wrote:
> 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 and the subsequent patches in this series introduce SBIUnit - the
> set of macros and functions which simplify the unit test development for
> OpenSBI and automate tests execution and evaluation.

The above paragraphs should be dropped.

> 
> 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 should be excluded completely on the
> compilation stage (and, apparently, it works in this way).

The '(and, apparently, it works in this way)' should be dropped.

> 
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> ---
>  include/sbi/sbi_unit.h | 69 ++++++++++++++++++++++++++++++++++++++++++
>  lib/sbi/Kconfig        |  4 +++
>  lib/sbi/objects.mk     |  1 +
>  lib/sbi/sbi_init.c     |  8 +++++
>  lib/sbi/sbi_unit.c     | 44 +++++++++++++++++++++++++++
>  5 files changed, 126 insertions(+)
>  create mode 100644 include/sbi/sbi_unit.h
>  create mode 100644 lib/sbi/sbi_unit.c
> 
> diff --git a/include/sbi/sbi_unit.h b/include/sbi/sbi_unit.h
> new file mode 100644
> index 0000000..685e58d
> --- /dev/null
> +++ b/include/sbi/sbi_unit.h
> @@ -0,0 +1,69 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
> + */
> +#ifndef __SBI_UNIT_H__
> +#define __SBI_UNIT_H__
> +
> +extern struct sbiunit_test_suite *__start_sbiunit_test_suites;
> +extern struct sbiunit_test_suite *__end_sbiunit_test_suites;

These start/end pointers don't appear to be used?

> +
> +#include <sbi/sbi_types.h>
> +#include <sbi/sbi_console.h>
> +#include <sbi/sbi_string.h>
> +
> +#define _CONCAT(a, b) a ## b
> +#define CONCAT(a, b) _CONCAT(a, b)

This CONCAT macro isn't used.

> +
> +struct sbiunit_test_case {
> +	char *name;

const char *name ?

> +	bool should_run;

Rename 'should_run' to 'enabled'

> +	bool result;
> +	void (*test_func)(struct sbiunit_test_case *test);
> +	void (*onerr)(struct sbiunit_test_case *test);
> +};
> +
> +struct sbiunit_test_suite {
> +	struct sbiunit_test_case *cases;
> +	const char *name;

nit: I'd put name first.

> +};
> +
> +#define SBIUNIT_TEST_CASE(func)	\
> +	{ .name = #func, .should_run = 1, .result = 0, .test_func = &func }

nit: Should use true/false instead of 1/0.
nit: No need for the '&' in front of func.

> +
> +#define SBIUNIT_TEST_SUITE(suite_name, cases_arr) \
> +	struct sbiunit_test_suite suite_name = { .name = #suite_name, .cases = cases_arr }
> +
> +#define SBIUNIT_INFO(test, msg) sbi_printf("%s: %s", test->name, msg)

Maybe some sort of prefix like "SBIUNIT" should be on this line.

> +
> +#define SBIUNIT_EXPECT(test, cond) do {							\
> +	if (!(cond)) {									\
> +		test->result = 0;							\
> +		SBIUNIT_INFO(test, "Condition \"" #cond "\" expected to be true!\n");	\
> +	}										\
> +} while (0)
> +
> +#define SBIUNIT_ASSERT(test, cond) do {						\
> +	if (!(cond)) {								\
> +		test->result = 0;						\
> +		SBIUNIT_INFO(test, "Condition \"" #cond "\" must be true!\n");	\
> +		return;								\

Unnecessary 'return'. It's strange that an ASSERT macro doesn't result in
an sbi_panic().

> +	}									\
> +} 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)

These macros need () around their arguments, e.g.

#define SBIUNIT_EXPECT_EQ(test, a, b) SBIUNIT_EXPECT(test, (a) == (b))

> +#define SBIUNIT_EXPECT_MEMEQ(test, a, b, len) \
> +	SBIUNIT_EXPECT(test, sbi_memcmp(a, b, len) == 0)
> +#define SBIUNIT_ASSERT_MEMEQ(test, a, b, len) \
> +	SBIUNIT_ASSERT(test, sbi_memcmp(a, b, len) == 0)
> +#define SBIUNIT_EXPECT_STREQ(test, a, b) \
> +	SBIUNIT_EXPECT(test, sbi_strcmp(a, b) == 0)
> +#define SBIUNIT_ASSERT_STREQ(test, a, b) \
> +	SBIUNIT_ASSERT(test, sbi_strcmp(a, b) == 0)
> +
> +void run_all_tests(void);
> +#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..0a2318b 100644
> --- a/lib/sbi/objects.mk
> +++ b/lib/sbi/objects.mk
> @@ -11,6 +11,7 @@ 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-y += sbi_unit.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..16d0a00 100644
> --- a/lib/sbi/sbi_init.c
> +++ b/lib/sbi/sbi_init.c
> @@ -30,6 +30,10 @@
>  #include <sbi/sbi_tlb.h>
>  #include <sbi/sbi_version.h>
>  
> +#ifdef CONFIG_SBIUNIT
> +#include <sbi/sbi_unit.h>
> +#endif

We can move the '#ifdef CONFIG_SBIUNIT' into the header and then always
include it.

> +
>  #define BANNER                                              \
>  	"   ____                    _____ ____ _____\n"     \
>  	"  / __ \\                  / ____|  _ \\_   _|\n"  \
> @@ -398,6 +402,10 @@ static void __noreturn init_coldboot(struct sbi_scratch *scratch, u32 hartid)
>  
>  	sbi_boot_print_hart(scratch, hartid);
>  
> +#ifdef CONFIG_SBIUNIT
> +	run_all_tests();
> +#endif

I was going to suggest that we could provide a stub for
!defined(CONFIG_SBIUNIT), but I sort like that the #ifdef annotates that
this function is only called with that config enabled. So, either way.

> +
>  	/*
>  	 * 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.c b/lib/sbi/sbi_unit.c
> new file mode 100644
> index 0000000..9c7efec
> --- /dev/null
> +++ b/lib/sbi/sbi_unit.c
> @@ -0,0 +1,44 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
> + */
> +#include <sbi/sbi_unit.h>
> +#include <sbi/sbi_console.h>
> +
> +static struct sbiunit_test_suite *test_suites[] = {
> +};
> +
> +static void run_test_suite(struct sbiunit_test_suite *suite)
> +{
> +	struct sbiunit_test_case *s_case;
> +	u32 count_pass, count_fail;
> +
> +	sbi_printf("## Running test suite: %s\n", suite->name);
> +	count_pass = 0;
> +	count_fail = 0;

nit: Can set these zero in the declarations above.

> +
> +	s_case = suite->cases;
> +	while (s_case->should_run) {
> +		s_case->result = 1;

Why initialize result to pass?

> +		s_case->test_func(s_case);
> +		if (s_case->result)
> +			count_pass++;
> +		else
> +			count_fail++;
> +		sbi_printf("[%s] %s\n", s_case->result ? "OK" : "FAIL", s_case->name);

Is OK/FAIL what KUnit outputs? I'd expect OK/NOK or PASS/FAIL. We'll also
probably want a SKIP sooner or later.

> +		s_case++;
> +	}
> +	sbi_printf("%u SUCCESS / %u FAIL / %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 < sizeof(test_suites) / sizeof(test_suites[0]); i++)

We have array_size() in include/sbi/sbi_types.h

> +		run_test_suite(test_suites[i]);
> +}
> -- 
> 2.34.1
> 
>

Thanks,
drew

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

[Andrew Jones]

On Thu, Feb 08, 2024 at 09:50:49AM +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.
> 
> This patch depends on the previous patches in this series as it uses
> SBIUnit macros, functions and definitions

No need to state that a patch depends on a previous patch, as that's
generally the case.

> 
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> ---
>  lib/sbi/objects.mk        |   2 +
>  lib/sbi/sbi_bitmap_test.c | 104 ++++++++++++++++++++++++++++++++++++++
>  lib/sbi/sbi_unit.c        |   2 +
>  3 files changed, 108 insertions(+)
>  create mode 100644 lib/sbi/sbi_bitmap_test.c
> 
> diff --git a/lib/sbi/objects.mk b/lib/sbi/objects.mk
> index 0a2318b..b29e63f 100644
> --- a/lib/sbi/objects.mk
> +++ b/lib/sbi/objects.mk
> @@ -56,6 +56,8 @@ libsbi-objs-$(CONFIG_SBI_ECALL_VENDOR) += sbi_ecall_vendor.o
>  carray-sbi_ecall_exts-$(CONFIG_SBI_ECALL_DBTR) += ecall_dbtr
>  libsbi-objs-$(CONFIG_SBI_ECALL_DBTR) += sbi_ecall_dbtr.o
>  
> +libsbi-objs-$(CONFIG_SBIUNIT) += sbi_bitmap_test.o
> +
>  libsbi-objs-y += sbi_bitmap.o
>  libsbi-objs-y += sbi_bitops.o
>  libsbi-objs-y += sbi_console.o
> diff --git a/lib/sbi/sbi_bitmap_test.c b/lib/sbi/sbi_bitmap_test.c
> new file mode 100644
> index 0000000..0fd0091
> --- /dev/null
> +++ b/lib/sbi/sbi_bitmap_test.c
> @@ -0,0 +1,104 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
> + */
> +#include <sbi/sbi_bitmap.h>
> +#include <sbi/sbi_unit.h>
> +#include <sbi/sbi_console.h>
> +
> +static u64 data_a[] = { 0xDEADBEEF, 0x00BAB10C, 0x1BADB002, 0xABADBABE };
> +static u64 data_b[] = { 0xC00010FF, 0x00BAB10C, 0xBAAAAAAD, 0xBADDCAFE };
> +static u64 data_zero[] = { 0, 0, 0, 0 };
> +

nit: Remove one blank line and the below defines up above the globals.

> +
> +#define DATA_SIZE sizeof(data_zero)
> +#define DATA_BIT_SIZE (DATA_SIZE * 8)
> +
> +static void bitmap_and_test(struct sbiunit_test_case *test)
> +{
> +	u64 res[DATA_SIZE];
> +	u64 a_and_b[] = { 0xDEADBEEF & 0xC00010FF, 0x00BAB10C & 0x00BAB10C,
> +			  0x1BADB002 & 0xBAAAAAAD, 0xABADBABE & 0xBADDCAFE };

Should refer to the test data by name, e.g. data_a[0] or with a define,
'#define DATA_A0 0xDEADBEEFULL'

> +
> +	__bitmap_and(res, data_a, data_b, DATA_BIT_SIZE);
> +	SBIUNIT_EXPECT_MEMEQ(test, res, a_and_b, DATA_SIZE);
> +
> +	// a & a = a

Should use /**/ comment format.

> +	__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[] = { 0xDEADBEEF | 0xC00010FF, 0x00BAB10C | 0x00BAB10C,
> +		       0x1BADB002 | 0xBAAAAAAD, 0xABADBABE | 0xBADDCAFE };
> +
> +	__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[] = { 0xDEADBEEF ^ 0xC00010FF, 0x00BAB10C ^ 0x00BAB10C,
> +			  0x1BADB002 ^ 0xBAAAAAAD, 0xABADBABE ^ 0xBADDCAFE };
> +
> +	__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);
> diff --git a/lib/sbi/sbi_unit.c b/lib/sbi/sbi_unit.c
> index 9c7efec..2d3da7f 100644
> --- a/lib/sbi/sbi_unit.c
> +++ b/lib/sbi/sbi_unit.c
> @@ -6,7 +6,9 @@
>  #include <sbi/sbi_unit.h>
>  #include <sbi/sbi_console.h>
>  
> +extern struct sbiunit_test_suite bitmap_test_suite;
>  static struct sbiunit_test_suite *test_suites[] = {
> +	&bitmap_test_suite,
>  };
>  
>  static void run_test_suite(struct sbiunit_test_suite *suite)
> -- 
> 2.34.1
> 
>

Thanks,
drew

[PATCH 4/4] lib: tests: Add sbi_console test

[Andrew Jones]

On Thu, Feb 08, 2024 at 09:50:50AM +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.
> 
> This patch depends on the previous patches from the series as it uses
> SBIUnit macros, functions and definitions.

Again, this last sentence isn't necessary to write in a commit message.

> 
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> ---
>  lib/sbi/sbi_console.c      |   4 ++
>  lib/sbi/sbi_console_test.c | 104 +++++++++++++++++++++++++++++++++++++
>  lib/sbi/sbi_unit.c         |   2 +
>  3 files changed, 110 insertions(+)
>  create mode 100644 lib/sbi/sbi_console_test.c
> 
> 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..8c3b215
> --- /dev/null
> +++ b/lib/sbi/sbi_console_test.c
> @@ -0,0 +1,104 @@
> +/*
> + * SPDX-License-Identifier: BSD-2-Clause
> + *
> + * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
> + */
> +#include <sbi/sbi_unit.h>
> +#include <sbi/sbi_heap.h>
> +
> +/*
> + * Console functions are wrapped in order to mock the console object and don't affect the actual
> + * console output
> + */
> +#define CONSOLE_DO(action) ({		\
> +	old_dev = console_dev;		\
> +	console_dev = &new_dev;		\
> +	action;				\
> +	console_dev = old_dev;		\
> +})

Why not use a local variable for old_dev and pass 'new_dev' in as an
argument?

> +
> +// We are using a GCC extension here, which allows us to return a value from a block

No need for this comment.

> +#define CONSOLE_DO_RET(action) ({	\
> +	old_dev = console_dev;		\
> +	console_dev = &new_dev;		\
> +	u64 res = action;		\

Please use names like __res inside macros to avoid shadowing concerns.

> +	console_dev = old_dev;		\
> +	res;				\
> +})

We only need CONSOLE_DO_RET(). The caller can ignore the return value.
And its name should include an TEST_ prefix, because this file is
included by other files.

> +
> +#define BUF_LEN 1024

TEST_CONSOLE_BUF_LEN

> +
> +static const struct sbi_console_device *old_dev;
> +
> +static char buf[BUF_LEN];
> +static u32 pos;

All the above need a test_console_ prefix

> +static void test_console_putc(char c)
> +{
> +	buf[pos] = c;
> +	pos = (pos + 1) % BUF_LEN;
> +}
> +
> +static void clear_buf(void)
> +{
> +	pos = 0;
> +	sbi_memset(buf, 0, BUF_LEN);

I guess buf[0] = '\0' should be sufficient.

> +}
> +
> +// Mocking the console

/**/ comment style

> +static const struct sbi_console_device new_dev = {
> +	.name = "Test console device",
> +	.console_putc = test_console_putc,
> +};
> +
> +static void putc_test(struct sbiunit_test_case *test)
> +{
> +	clear_buf();
> +
> +	CONSOLE_DO(sbi_putc('a'));

I think I would replace the CONSOLE_DO macro with a _begin() / _end()
pair.

 test_console_begin();
 sbi_putc('a');
 test_console_end();

> +	SBIUNIT_ASSERT_EQ(test, buf[0], 'a');
> +}
> +
> +#define PUTS_TEST(test, expected, param) do {				\

param is the string, so it should be named 's' or 'str' or similar.

> +	clear_buf();							\
> +	CONSOLE_DO(sbi_puts(param));					\
> +	SBIUNIT_ASSERT_STREQ(test, buf, 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!");
> +	PUTS_TEST(test, "Hello,", "Hello,\0OpenSBI!");
> +}
> +
> +#define PRINTF_TEST(test, expected, format, ...) do {					\
> +	clear_buf();									\
> +	SBIUNIT_ASSERT_EQ(test, CONSOLE_DO_RET(sbi_printf(format, ##__VA_ARGS__)),	\
> +			  sbi_strlen(expected));					\
> +	SBIUNIT_ASSERT_STREQ(test, buf, 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);
> diff --git a/lib/sbi/sbi_unit.c b/lib/sbi/sbi_unit.c
> index 2d3da7f..b2f2063 100644
> --- a/lib/sbi/sbi_unit.c
> +++ b/lib/sbi/sbi_unit.c
> @@ -7,8 +7,10 @@
>  #include <sbi/sbi_console.h>
>  
>  extern struct sbiunit_test_suite bitmap_test_suite;
> +extern struct sbiunit_test_suite console_test_suite;
>  static struct sbiunit_test_suite *test_suites[] = {
>  	&bitmap_test_suite,
> +	&console_test_suite,
>  };
>  
>  static void run_test_suite(struct sbiunit_test_suite *suite)
> -- 
> 2.34.1
> 
>

Thanks,
drew

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

[Ivan Orlov]

On 2/12/24 17:20, Andrew Jones wrote:
> The above should be in the cover letter, which this series is missing.
> Cover letters are nice since they give reviewers a place to comment on the
> overall series.
> 

Alright, will be moved to the cover letter in V2.
> The above paragraph belongs in the cover letter along with elaboration
> on how only certain parts of KUnit were reimplemented and why those
> certain parts, and not others, were chosen.
> 

Makes sense, I'll extend the description.

>>
>> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
>> ---
>>   docs/writing_tests.md | 143 ++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 143 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..481f2ee
>> --- /dev/null
>> +++ b/docs/writing_tests.md
>> @@ -0,0 +1,143 @@
>> +Writing tests for OpenSBI
>> +=========================
>> +
>> +SBIUnit
>> +-------
>> +SBIUnit is a set of macros and functions which simplify the test development and automate the
> 
> We aim for 80 char line length, especially in documentation.
> 
>> +test execution and evaluation. All of the SBIUnit definitions could be found in
> 
> s/could be found in/are in the/
> 
> 
>> +`include/sbi/sbi_unit.h` header file, and implementations are available in `lib/sbi/sbi_unit.c`.
> 
> I think the string 'test' should be somewhere in the filenames. 'unit' is
> too generic.
> 
>> +
>> +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;
>> +}
>> +```
>> +
>> +Apparently, it calculates the string length.
> 
> s/Apparently, it/which/
> 
>> +
>> +Create the file `lib/sbi/sbi_string_test.c` with the following content:
>> +
>> +```c
>> +#include <sbi/sbi_unit.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);
>> +```
>> +
>> +After that, add the corresponding entry to `lib/sbi/sbi_unit.c` and update the `test_suites` array:
> 
> s/After that,/Then,/
> 
> But I think we should be able to add the test suite pointer to an elf
> section with the SBIUNIT_TEST_SUITE() macro to avoid this step.
> 

That was an initial idea, however I faced some obstacles during the 
implementation.

I believe we would like to cover the static functions, as well as use 
static variables in the tests. In this case, we would include the test 
in the source we are covering (for instance, include 
"sbi_console_test.c" in "sbi_console.c"). If we use OpenSBI 
(libplatsbi.a) as a library when linking firmware, and firmware refers 
to a symbol from "sbi_console.h", it will automatically link the test 
code too. This means that firmware should have the test ELF section as 
well. Manual registration of the tests in 'sbi_unit.c', on the other 
hand, would not require any effort from the firmware developers if they 
decide to enable tests for OpenSBI.

Moreover, manual test declaration will make sure that we included all of 
the tests. Otherwise, if we define the test in a separate file, it will 
be linked out unless we refer to symbols from it somewhere.

What do you think of it?

>> +```c
>> +...
>> +extern struct sbiunit_test_suite string_test_suite;
>> +...
>> +static struct sbiunit_test_suite *test_suites[] = {
>> +    ...
>> +    &string_test_suite,
>> +};
>> +...
>> +```
>> +
>> +Add the corresponding Makefile entry to `lib/sbi/objects.mk`:
>> +```lang-makefile
>> +...
>> +libsbi-objs-$(CONFIG_SBIUNIT) += sbi_string_test.o
>> +```
>> +
>> +Now 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
>> +[OK] strlen_test
>> +1 SUCCESS / 0 FAIL / 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);
>> +```
>> +
>> +Compile and run it again:
>> +```
>> +...
>> +# Running SBIUNIT tests #
>> +...
>> +## Running test suite: string_test_suite
>> +strlen_test: Condition "sbi_strlen("Hello") == 100" expected to be true!
>> +[FAIL] strlen_test
>> +0 SUCCESS / 1 FAIL / 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 not add a new entry to `lib/sbi/objects.mk`, 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.
> 
> The distinction between SBIUNIT_EXPECT* and SBIUNIT_ASSERT* is good to
> document, but I don't think we want to list the functions as is done
> below. We should just reference the header from here, since we'll be
> lucky to keep the header filename in sync, let alone each function...
> 

I agree, it is definitely redundant.

Thank you so much for the review!

-- 
Kind regards,
Ivan Orlov

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

[Ivan Orlov]

On 2/12/24 17:36, Andrew Jones wrote:
> On Thu, Feb 08, 2024 at 09:50:48AM +0000, Ivan Orlov wrote:
>> 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 and the subsequent patches in this series introduce SBIUnit - the
>> set of macros and functions which simplify the unit test development for
>> OpenSBI and automate tests execution and evaluation.
> 
> The above paragraphs should be dropped.
> 
>>
>> 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 should be excluded completely on the
>> compilation stage (and, apparently, it works in this way).
> 
> The '(and, apparently, it works in this way)' should be dropped.
> 

Thanks, I'll put this information in the cover letter of V2.
>>
>> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
>> ---
>>   include/sbi/sbi_unit.h | 69 ++++++++++++++++++++++++++++++++++++++++++
>>   lib/sbi/Kconfig        |  4 +++
>>   lib/sbi/objects.mk     |  1 +
>>   lib/sbi/sbi_init.c     |  8 +++++
>>   lib/sbi/sbi_unit.c     | 44 +++++++++++++++++++++++++++
>>   5 files changed, 126 insertions(+)
>>   create mode 100644 include/sbi/sbi_unit.h
>>   create mode 100644 lib/sbi/sbi_unit.c
>>
>> diff --git a/include/sbi/sbi_unit.h b/include/sbi/sbi_unit.h
>> new file mode 100644
>> index 0000000..685e58d
>> --- /dev/null
>> +++ b/include/sbi/sbi_unit.h
>> @@ -0,0 +1,69 @@
>> +/*
>> + * SPDX-License-Identifier: BSD-2-Clause
>> + *
>> + * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
>> + */
>> +#ifndef __SBI_UNIT_H__
>> +#define __SBI_UNIT_H__
>> +
>> +extern struct sbiunit_test_suite *__start_sbiunit_test_suites;
>> +extern struct sbiunit_test_suite *__end_sbiunit_test_suites;
> 
> These start/end pointers don't appear to be used?
> 

Ah, I just forgot to remove them after implementing the SBIUnit version 
with a dedicated ELF section for the tests... Thanks!

>> +
>> +#include <sbi/sbi_types.h>
>> +#include <sbi/sbi_console.h>
>> +#include <sbi/sbi_string.h>
>> +
>> +#define _CONCAT(a, b) a ## b
>> +#define CONCAT(a, b) _CONCAT(a, b)
> 
> This CONCAT macro isn't used.
> 
>> +
>> +struct sbiunit_test_case {
>> +	char *name;
> 
> const char *name ?
> 
>> +	bool should_run;
> 
> Rename 'should_run' to 'enabled'
> 

Alright, will be done.

>> +	bool result;
>> +	void (*test_func)(struct sbiunit_test_case *test);
>> +	void (*onerr)(struct sbiunit_test_case *test);
>> +};
>> +
>> +struct sbiunit_test_suite {
>> +	struct sbiunit_test_case *cases;
>> +	const char *name;
> 
> nit: I'd put name first.
> 
>> +};
>> +
>> +#define SBIUNIT_TEST_CASE(func)	\
>> +	{ .name = #func, .should_run = 1, .result = 0, .test_func = &func }
> 
> nit: Should use true/false instead of 1/0.
> nit: No need for the '&' in front of func.
> 
>> +
>> +#define SBIUNIT_TEST_SUITE(suite_name, cases_arr) \
>> +	struct sbiunit_test_suite suite_name = { .name = #suite_name, .cases = cases_arr }
>> +
>> +#define SBIUNIT_INFO(test, msg) sbi_printf("%s: %s", test->name, msg)
> 
> Maybe some sort of prefix like "SBIUNIT" should be on this line.
> 
>> +
>> +#define SBIUNIT_EXPECT(test, cond) do {							\
>> +	if (!(cond)) {									\
>> +		test->result = 0;							\
>> +		SBIUNIT_INFO(test, "Condition \"" #cond "\" expected to be true!\n");	\
>> +	}										\
>> +} while (0)
>> +
>> +#define SBIUNIT_ASSERT(test, cond) do {						\
>> +	if (!(cond)) {								\
>> +		test->result = 0;						\
>> +		SBIUNIT_INFO(test, "Condition \"" #cond "\" must be true!\n");	\
>> +		return;								\
> 
> Unnecessary 'return'. It's strange that an ASSERT macro doesn't result in
> an sbi_panic().
> 

I thought it would be bad if test results in hang, but if it is ok I 
will rewrite it.

>> +	}									\
>> +} 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)
> 
> These macros need () around their arguments, e.g.
> 
> #define SBIUNIT_EXPECT_EQ(test, a, b) SBIUNIT_EXPECT(test, (a) == (b))
> 
>> +#define SBIUNIT_EXPECT_MEMEQ(test, a, b, len) \
>> +	SBIUNIT_EXPECT(test, sbi_memcmp(a, b, len) == 0)
>> +#define SBIUNIT_ASSERT_MEMEQ(test, a, b, len) \
>> +	SBIUNIT_ASSERT(test, sbi_memcmp(a, b, len) == 0)
>> +#define SBIUNIT_EXPECT_STREQ(test, a, b) \
>> +	SBIUNIT_EXPECT(test, sbi_strcmp(a, b) == 0)
>> +#define SBIUNIT_ASSERT_STREQ(test, a, b) \
>> +	SBIUNIT_ASSERT(test, sbi_strcmp(a, b) == 0)
>> +
>> +void run_all_tests(void);
>> +#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..0a2318b 100644
>> --- a/lib/sbi/objects.mk
>> +++ b/lib/sbi/objects.mk
>> @@ -11,6 +11,7 @@ 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-y += sbi_unit.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..16d0a00 100644
>> --- a/lib/sbi/sbi_init.c
>> +++ b/lib/sbi/sbi_init.c
>> @@ -30,6 +30,10 @@
>>   #include <sbi/sbi_tlb.h>
>>   #include <sbi/sbi_version.h>
>>   
>> +#ifdef CONFIG_SBIUNIT
>> +#include <sbi/sbi_unit.h>
>> +#endif
> 
> We can move the '#ifdef CONFIG_SBIUNIT' into the header and then always
> include it.
> 

Yeah, makes sense.

>> +
>>   #define BANNER                                              \
>>   	"   ____                    _____ ____ _____\n"     \
>>   	"  / __ \\                  / ____|  _ \\_   _|\n"  \
>> @@ -398,6 +402,10 @@ static void __noreturn init_coldboot(struct sbi_scratch *scratch, u32 hartid)
>>   
>>   	sbi_boot_print_hart(scratch, hartid);
>>   
>> +#ifdef CONFIG_SBIUNIT
>> +	run_all_tests();
>> +#endif
> 
> I was going to suggest that we could provide a stub for
> !defined(CONFIG_SBIUNIT), but I sort like that the #ifdef annotates that
> this function is only called with that config enabled. So, either way.
> 

I think your suggestion is better, it will help avoiding #ifdef 
"boilerplate" and make the code cleaner.

>> +
>>   	/*
>>   	 * 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.c b/lib/sbi/sbi_unit.c
>> new file mode 100644
>> index 0000000..9c7efec
>> --- /dev/null
>> +++ b/lib/sbi/sbi_unit.c
>> @@ -0,0 +1,44 @@
>> +/*
>> + * SPDX-License-Identifier: BSD-2-Clause
>> + *
>> + * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
>> + */
>> +#include <sbi/sbi_unit.h>
>> +#include <sbi/sbi_console.h>
>> +
>> +static struct sbiunit_test_suite *test_suites[] = {
>> +};
>> +
>> +static void run_test_suite(struct sbiunit_test_suite *suite)
>> +{
>> +	struct sbiunit_test_case *s_case;
>> +	u32 count_pass, count_fail;
>> +
>> +	sbi_printf("## Running test suite: %s\n", suite->name);
>> +	count_pass = 0;
>> +	count_fail = 0;
> 
> nit: Can set these zero in the declarations above.
> 
>> +
>> +	s_case = suite->cases;
>> +	while (s_case->should_run) {
>> +		s_case->result = 1;
> 
> Why initialize result to pass?

The logic was to set the 'result' to 0 in the SBIUNIT_ASSERT or 
SBIUNIT_EXPECT, but it really seems confusing so I agree that it might 
be better to create a 'failed' field and set it to 'true' in the 
assert/expect.

> 
>> +		s_case->test_func(s_case);
>> +		if (s_case->result)
>> +			count_pass++;
>> +		else
>> +			count_fail++;
>> +		sbi_printf("[%s] %s\n", s_case->result ? "OK" : "FAIL", s_case->name);
> 
> Is OK/FAIL what KUnit outputs? I'd expect OK/NOK or PASS/FAIL. We'll also
> probably want a SKIP sooner or later.
> 

It looks like KUnit uses 'FAILED' or 'PASSED'. So I will update the 
messages correspondingly.

>> +		s_case++;
>> +	}
>> +	sbi_printf("%u SUCCESS / %u FAIL / %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 < sizeof(test_suites) / sizeof(test_suites[0]); i++)
> 
> We have array_size() in include/sbi/sbi_types.h
> 

Cool, I'll use it instead.

Thank you!
-- 
Kind regards,
Ivan Orlov

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

[Andrew Jones]

On Mon, Feb 12, 2024 at 09:48:02PM +0000, Ivan Orlov wrote:
> On 2/12/24 17:20, Andrew Jones wrote:
...
> > But I think we should be able to add the test suite pointer to an elf
> > section with the SBIUNIT_TEST_SUITE() macro to avoid this step.
> > 
> 
> That was an initial idea, however I faced some obstacles during the
> implementation.
> 
> I believe we would like to cover the static functions, as well as use static
> variables in the tests. In this case, we would include the test in the
> source we are covering (for instance, include "sbi_console_test.c" in
> "sbi_console.c"). If we use OpenSBI (libplatsbi.a) as a library when linking
> firmware, and firmware refers to a symbol from "sbi_console.h", it will
> automatically link the test code too. This means that firmware should have
> the test ELF section as well. Manual registration of the tests in
> 'sbi_unit.c', on the other hand, would not require any effort from the
> firmware developers if they decide to enable tests for OpenSBI.

Hmm, I see. So maybe we can use the build system's carray?

> 
> Moreover, manual test declaration will make sure that we included all of the
> tests. Otherwise, if we define the test in a separate file, it will be
> linked out unless we refer to symbols from it somewhere.

This could possibly be worked around with the linker's --whole-archive
option.

Thanks,
drew

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

[Andrew Jones]

On Mon, Feb 12, 2024 at 10:03:52PM +0000, Ivan Orlov wrote:
> On 2/12/24 17:36, Andrew Jones wrote:
> > On Thu, Feb 08, 2024 at 09:50:48AM +0000, Ivan Orlov wrote:
...
> > > +#define SBIUNIT_ASSERT(test, cond) do {						\
> > > +	if (!(cond)) {								\
> > > +		test->result = 0;						\
> > > +		SBIUNIT_INFO(test, "Condition \"" #cond "\" must be true!\n");	\
> > > +		return;								\
> > 
> > Unnecessary 'return'. It's strange that an ASSERT macro doesn't result in
> > an sbi_panic().
> > 
> 
> I thought it would be bad if test results in hang, but if it is ok I will
> rewrite it.

I think an assert should output its message and then stop all execution.
Asserts should only be used in situations where failures mean the
test programmer made an improper assumption. If we don't halt the moment
we determine the assumption is wrong, then we'll potentially generate
passing test results, but the tests may not be testing what the test
programmer thought they were.

Thanks,
drew

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

[Ivan Orlov]

On 2/13/24 13:51, Andrew Jones wrote:
> On Mon, Feb 12, 2024 at 09:48:02PM +0000, Ivan Orlov wrote:
>> On 2/12/24 17:20, Andrew Jones wrote:
> ...
>>> But I think we should be able to add the test suite pointer to an elf
>>> section with the SBIUNIT_TEST_SUITE() macro to avoid this step.
>>>
>>
>> That was an initial idea, however I faced some obstacles during the
>> implementation.
>>
>> I believe we would like to cover the static functions, as well as use static
>> variables in the tests. In this case, we would include the test in the
>> source we are covering (for instance, include "sbi_console_test.c" in
>> "sbi_console.c"). If we use OpenSBI (libplatsbi.a) as a library when linking
>> firmware, and firmware refers to a symbol from "sbi_console.h", it will
>> automatically link the test code too. This means that firmware should have
>> the test ELF section as well. Manual registration of the tests in
>> 'sbi_unit.c', on the other hand, would not require any effort from the
>> firmware developers if they decide to enable tests for OpenSBI.
> 
> Hmm, I see. So maybe we can use the build system's carray?
> 

Hmmm, I've never heard about this thing before, could you please point 
me to a reference or source where I could read about it?

>>
>> Moreover, manual test declaration will make sure that we included all of the
>> tests. Otherwise, if we define the test in a separate file, it will be
>> linked out unless we refer to symbols from it somewhere.
> 
> This could possibly be worked around with the linker's --whole-archive
> option.

In this case we would have to update the makefile logic, because now 
OpenSBI uses 'compile_elf' makefile procedure for the test payload and 
the firmware elf files both, and we would not like to use 
--whole-archive for the test payload. I guess it is an overcomplication...

-- 
Kind regards,
Ivan Orlov

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

[Ivan Orlov]

On 2/13/24 14:00, Andrew Jones wrote:
> On Mon, Feb 12, 2024 at 10:03:52PM +0000, Ivan Orlov wrote:
>> On 2/12/24 17:36, Andrew Jones wrote:
>>> On Thu, Feb 08, 2024 at 09:50:48AM +0000, Ivan Orlov wrote:
> ...
>>>> +#define SBIUNIT_ASSERT(test, cond) do {						\
>>>> +	if (!(cond)) {								\
>>>> +		test->result = 0;						\
>>>> +		SBIUNIT_INFO(test, "Condition \"" #cond "\" must be true!\n");	\
>>>> +		return;								\
>>>
>>> Unnecessary 'return'. It's strange that an ASSERT macro doesn't result in
>>> an sbi_panic().
>>>
>>
>> I thought it would be bad if test results in hang, but if it is ok I will
>> rewrite it.
> 
> I think an assert should output its message and then stop all execution.
> Asserts should only be used in situations where failures mean the
> test programmer made an improper assumption. If we don't halt the moment
> we determine the assumption is wrong, then we'll potentially generate
> passing test results, but the tests may not be testing what the test
> programmer thought they were.
> 

Makes sense, so let's add the sbi_panic in version 2.

Thanks!

-- 
Kind regards,
Ivan Orlov

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

[Andrew Jones]

On Tue, Feb 13, 2024 at 02:54:17PM +0000, Ivan Orlov wrote:
> On 2/13/24 13:51, Andrew Jones wrote:
> > On Mon, Feb 12, 2024 at 09:48:02PM +0000, Ivan Orlov wrote:
> > > On 2/12/24 17:20, Andrew Jones wrote:
> > ...
> > > > But I think we should be able to add the test suite pointer to an elf
> > > > section with the SBIUNIT_TEST_SUITE() macro to avoid this step.
> > > > 
> > > 
> > > That was an initial idea, however I faced some obstacles during the
> > > implementation.
> > > 
> > > I believe we would like to cover the static functions, as well as use static
> > > variables in the tests. In this case, we would include the test in the
> > > source we are covering (for instance, include "sbi_console_test.c" in
> > > "sbi_console.c"). If we use OpenSBI (libplatsbi.a) as a library when linking
> > > firmware, and firmware refers to a symbol from "sbi_console.h", it will
> > > automatically link the test code too. This means that firmware should have
> > > the test ELF section as well. Manual registration of the tests in
> > > 'sbi_unit.c', on the other hand, would not require any effort from the
> > > firmware developers if they decide to enable tests for OpenSBI.
> > 
> > Hmm, I see. So maybe we can use the build system's carray?
> > 
> 
> Hmmm, I've never heard about this thing before, could you please point me to
> a reference or source where I could read about it?

It's used in OpenSBI code for arrays such as sbi_ecall_exts. See commit
56bed1a0fe39 ("lib: sbi_ecall: Generate extensions list with carray") for
that example.

> 
> > > 
> > > Moreover, manual test declaration will make sure that we included all of the
> > > tests. Otherwise, if we define the test in a separate file, it will be
> > > linked out unless we refer to symbols from it somewhere.
> > 
> > This could possibly be worked around with the linker's --whole-archive
> > option.
> 
> In this case we would have to update the makefile logic, because now OpenSBI
> uses 'compile_elf' makefile procedure for the test payload and the firmware
> elf files both, and we would not like to use --whole-archive for the test
> payload. I guess it is an overcomplication...

Maybe, but I'm not sure how much we'd care about having bloated binaries
when building with CONFIG_SBIUNIT.

Thanks,
drew

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

[Ivan Orlov]

On 2/13/24 15:21, Andrew Jones wrote:
>> Hmmm, I've never heard about this thing before, could you please point me to
>> a reference or source where I could read about it?
> 
> It's used in OpenSBI code for arrays such as sbi_ecall_exts. See commit
> 56bed1a0fe39 ("lib: sbi_ecall: Generate extensions list with carray") for
> that example.
> 

Thanks, I'll take a look and use them in the V2 if it is possible!

>>
>> In this case we would have to update the makefile logic, because now OpenSBI
>> uses 'compile_elf' makefile procedure for the test payload and the firmware
>> elf files both, and we would not like to use --whole-archive for the test
>> payload. I guess it is an overcomplication...
> 
> Maybe, but I'm not sure how much we'd care about having bloated binaries
> when building with CONFIG_SBIUNIT.
>

The problem is not with the bloated payload binaries: if we use 
--whole-archive when linking the test payload, it starts giving the 
'undefined reference' errors, because some of the object files refer to 
symbols which don't exist in the test payload. Like:

```
opensbi/platform/generic/andes/sleep.S:63: undefined reference to 
`_start_warm'
```

_start_warm is defined in fw_base, but it's not accessible when building 
the test payload (firmware/payloads/).

Probably, we could just disable the test payload building when 
CONFIG_SBIUNIT is enabled, but I'm not sure if it is a correct 
approach... Another option would be optional --whole-archive use, so we 
would use it for linking the fw_* ELF files only.

-- 
Kind regards,
Ivan Orlov

[PATCH 4/4] lib: tests: Add sbi_console test

[Ivan Orlov]

On 2/12/24 18:24, Andrew Jones wrote:
> On Thu, Feb 08, 2024 at 09:50:50AM +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.
>>
>> This patch depends on the previous patches from the series as it uses
>> SBIUnit macros, functions and definitions.
> 
> Again, this last sentence isn't necessary to write in a commit message.
> 
>>
>> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
>> ---
>>   lib/sbi/sbi_console.c      |   4 ++
>>   lib/sbi/sbi_console_test.c | 104 +++++++++++++++++++++++++++++++++++++
>>   lib/sbi/sbi_unit.c         |   2 +
>>   3 files changed, 110 insertions(+)
>>   create mode 100644 lib/sbi/sbi_console_test.c
>>
>> 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..8c3b215
>> --- /dev/null
>> +++ b/lib/sbi/sbi_console_test.c
>> @@ -0,0 +1,104 @@
>> +/*
>> + * SPDX-License-Identifier: BSD-2-Clause
>> + *
>> + * Author: Ivan Orlov <ivan.orlov0322@gmail.com>
>> + */
>> +#include <sbi/sbi_unit.h>
>> +#include <sbi/sbi_heap.h>
>> +
>> +/*
>> + * Console functions are wrapped in order to mock the console object and don't affect the actual
>> + * console output
>> + */
>> +#define CONSOLE_DO(action) ({		\
>> +	old_dev = console_dev;		\
>> +	console_dev = &new_dev;		\
>> +	action;				\
>> +	console_dev = old_dev;		\
>> +})
> 
> Why not use a local variable for old_dev and pass 'new_dev' in as an
> argument?
> 
>> +
>> +// We are using a GCC extension here, which allows us to return a value from a block
> 
> No need for this comment.
> 
>> +#define CONSOLE_DO_RET(action) ({	\
>> +	old_dev = console_dev;		\
>> +	console_dev = &new_dev;		\
>> +	u64 res = action;		\
> 
> Please use names like __res inside macros to avoid shadowing concerns.
> 
>> +	console_dev = old_dev;		\
>> +	res;				\
>> +})
> 
> We only need CONSOLE_DO_RET(). The caller can ignore the return value.
> And its name should include an TEST_ prefix, because this file is
> included by other files.
> 
>> +
>> +#define BUF_LEN 1024
> 
> TEST_CONSOLE_BUF_LEN
> 
>> +
>> +static const struct sbi_console_device *old_dev;
>> +
>> +static char buf[BUF_LEN];
>> +static u32 pos;
> 
> All the above need a test_console_ prefix
> 
>> +static void test_console_putc(char c)
>> +{
>> +	buf[pos] = c;
>> +	pos = (pos + 1) % BUF_LEN;
>> +}
>> +
>> +static void clear_buf(void)
>> +{
>> +	pos = 0;
>> +	sbi_memset(buf, 0, BUF_LEN);
> 
> I guess buf[0] = '\0' should be sufficient.
> 

Sorry for the late reply, I agree on all of the points you mentioned 
except this one. The 'puts' test stops me from clearing the buffer by 
setting the first char to zero:

```
PUTS_TEST(test, "Hello,", "Hello,\0OpenSBI!");
```

This test checks if 'puts' stops printing after facing \0. 'puts' won't 
print out the \0 to the buffer after printing "Hello,", so in case of 
the 'lazy' buffer clearing there might be other characters in buffer 
after 'Hello,'. In this case, the 'sbi_strcmp' won't stop comparing the 
strings causing the test to fail (despite the behavior is correct).

I reckon that clearing the buffer completely will help avoiding such 
tricky issues by making the experiment as pure as possible :)

Thank you!
-- 
Kind regards,
Ivan Orlov

[PATCH 4/4] lib: tests: Add sbi_console test

[Andrew Jones]

On Thu, Feb 15, 2024 at 02:44:15PM +0000, Ivan Orlov wrote:
> On 2/12/24 18:24, Andrew Jones wrote:
> > On Thu, Feb 08, 2024 at 09:50:50AM +0000, Ivan Orlov wrote:
...
> > > +static void clear_buf(void)
> > > +{
> > > +	pos = 0;
> > > +	sbi_memset(buf, 0, BUF_LEN);
> > 
> > I guess buf[0] = '\0' should be sufficient.
> > 
> 
> Sorry for the late reply, I agree on all of the points you mentioned except
> this one. The 'puts' test stops me from clearing the buffer by setting the
> first char to zero:
> 
> ```
> PUTS_TEST(test, "Hello,", "Hello,\0OpenSBI!");
> ```
> 
> This test checks if 'puts' stops printing after facing \0. 'puts' won't
> print out the \0 to the buffer after printing "Hello,", so in case of the
> 'lazy' buffer clearing there might be other characters in buffer after
> 'Hello,'. In this case, the 'sbi_strcmp' won't stop comparing the strings
> causing the test to fail (despite the behavior is correct).
> 
> I reckon that clearing the buffer completely will help avoiding such tricky
> issues by making the experiment as pure as possible :)

Since the mock console device is a ring buffer it doesn't guarantee zeros
will follow the printed strings. I suggest writing tests which only
confirm the characters printed are in the stream and in the right order,
but not check for anything console device implementation specific.

Thanks,
drew

[PATCH 4/4] lib: tests: Add sbi_console test

[Ivan Orlov]

On 2/15/24 15:13, Andrew Jones wrote:
> On Thu, Feb 15, 2024 at 02:44:15PM +0000, Ivan Orlov wrote:
>> On 2/12/24 18:24, Andrew Jones wrote:
>>> On Thu, Feb 08, 2024 at 09:50:50AM +0000, Ivan Orlov wrote:
> ...
>>>> +static void clear_buf(void)
>>>> +{
>>>> +	pos = 0;
>>>> +	sbi_memset(buf, 0, BUF_LEN);
>>>
>>> I guess buf[0] = '\0' should be sufficient.
>>>
>>
>> Sorry for the late reply, I agree on all of the points you mentioned except
>> this one. The 'puts' test stops me from clearing the buffer by setting the
>> first char to zero:
>>
>> ```
>> PUTS_TEST(test, "Hello,", "Hello,\0OpenSBI!");
>> ```
>>
>> This test checks if 'puts' stops printing after facing \0. 'puts' won't
>> print out the \0 to the buffer after printing "Hello,", so in case of the
>> 'lazy' buffer clearing there might be other characters in buffer after
>> 'Hello,'. In this case, the 'sbi_strcmp' won't stop comparing the strings
>> causing the test to fail (despite the behavior is correct).
>>
>> I reckon that clearing the buffer completely will help avoiding such tricky
>> issues by making the experiment as pure as possible :)
> 
> Since the mock console device is a ring buffer it doesn't guarantee zeros
> will follow the printed strings. I suggest writing tests which only
> confirm the characters printed are in the stream and in the right order,
> but not check for anything console device implementation specific.
> 

Ah, alright, I see. Moreover, this particular test case is just useless 
since it doesn't increase the coverage. So I guess I will just use the 
"lazy" buffer clearing and remove this particular 'PUTS_TEST'. Thanks 
for the clarification!

-- 
Kind regards,
Ivan Orlov