Re: [PATCH] kunit: Cover 'assert.c' with tests

From: Ivan Orlov
Date: Fri May 03 2024 - 08:19:12 EST


On 5/3/24 12:10, Ivan Orlov wrote:
On 5/2/24 00:20, Rae Moar wrote:
On Sat, Apr 27, 2024 at 6:04 PM Ivan Orlov <ivan.orlov0322@xxxxxxxxx> wrote:

There are multiple assertion formatting functions in the `assert.c`
file, which are not covered with tests yet. Implement the KUnit test
for these functions.

The test consists of 11 test cases for the following functions:

1) 'is_literal'
2) 'is_str_literal'
3) 'kunit_assert_prologue', test case for multiple assert types
4) 'kunit_assert_print_msg'
5) 'kunit_unary_assert_format'
6) 'kunit_ptr_not_err_assert_format'
7) 'kunit_binary_assert_format'
8) 'kunit_binary_ptr_assert_format'
9) 'kunit_binary_str_assert_format'
10) 'kunit_assert_hexdump'
11) 'kunit_mem_assert_format'

The test aims at maximizing the branch coverage for the assertion
formatting functions. As you can see, it covers some of the static
helper functions as well, so we have to import the test source in the
`assert.c` file in order to be able to call and validate them.

Signed-off-by: Ivan Orlov <ivan.orlov0322@xxxxxxxxx>

Hello!

This is a great patch and addition of KUnit tests. Happy to see it.
Thank you very much!

I do have a few comments below. But none of them are deal breakers.


Hi Rae,

Thank you so much for the detailed review.


---
  lib/kunit/assert.c      |   4 +
  lib/kunit/assert_test.c | 416 ++++++++++++++++++++++++++++++++++++++++
  2 files changed, 420 insertions(+)
  create mode 100644 lib/kunit/assert_test.c

diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
index dd1d633d0fe2..ab68c6daf546 100644
--- a/lib/kunit/assert.c
+++ b/lib/kunit/assert.c
@@ -270,3 +270,7 @@ void kunit_mem_assert_format(const struct kunit_assert *assert,
         }
  }
  EXPORT_SYMBOL_GPL(kunit_mem_assert_format);
+
+#if IS_ENABLED(CONFIG_KUNIT_TEST)
+#include "assert_test.c"
+#endif

I might consider using the macro VISIBLE_IF_KUNIT macro, found in
include/kunit/visibility.h, to make the static functions in assert.c
visible only if KUnit is enabled. To avoid having to add the include
here. What do you think?


Wow, I haven't seen this macro before, thank you for the suggestion! I'll use it in the V2 of the patch.

I assume we need to use it in combination with EXPORT_SYMBOL_IF_KUNIT, otherwise GCC will complain on use of functions without definitions, right?


s/definitions/declarations/g :)

--
Kind regards,
Ivan Orlov