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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?


Ah, alright, it seems like GCC is going to complain on missing prototypes anyway, so we have to declare these static functions in the header file if CONFIG_KUNIT is defined.

--
Kind regards,
Ivan Orlov





[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux