Messages in this thread | | | Date | Tue, 14 May 2024 15:31:33 +0100 | Subject | Re: [PATCH v3] kunit: Cover 'assert.c' with tests | From | Ivan Orlov <> |
| |
On 5/14/24 01:17, Rae Moar wrote: > On Thu, May 9, 2024 at 5:05 AM Ivan Orlov <ivan.orlov0322@gmail.com> 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 mark the static functions in `assert.c` as 'VISIBLE_IF_KUNIT' >> and conditionally export them with EXPORT_SYMBOL_IF_KUNIT. Add the >> corresponding definitions to `assert.h`. >> >> Build the assert test when CONFIG_KUNIT_TEST is enabled, similar to >> how it is done for the string stream test. > > Hello! > > This looks great to me! Thanks for all your work on this! There is > just one comment I have below. Once that is fixed up, I am happy to > add a reviewed-by. > > Thanks! > -Rae > >> >> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com> >> --- >> V1 -> V2: >> - Check the output from the string stream for containing the key parts >> instead of comparing the results with expected strings char by char, as >> it was suggested by Rae Moar <rmoar@google.com>. Define two macros to >> make it possible (ASSERT_TEST_EXPECT_CONTAIN and >> ASSERT_TEST_EXPECT_NCONTAIN). >> - Mark the static functions in `assert.c` as VISIBLE_IF_KUNIT and export >> them conditionally if kunit is enabled instead of including the >> `assert_test.c` file in the end of `assert.c`. This way we will decouple >> the test from the implementation (SUT). >> - Update the kunit_assert_hexdump test: now it checks for presense of >> the brackets '<>' around the non-matching bytes, instead of comparing >> the kunit_assert_hexdump output char by char. >> V2 -> V3: >> - Make test case array and test suite definitions static >> - Change the condition in `assert.h`: we should declare VISIBLE_IF_KUNIT >> functions in the header file when CONFIG_KUNIT is enabled, not >> CONFIG_KUNIT_TEST. Otherwise, if CONFIG_KUNIT_TEST is disabled, >> VISIBLE_IF_KUNIT functions in the `assert.c` are not static, and >> prototypes for them can't be found. >> - Add MODULE_LICENSE and MODULE_DESCRIPTION macros >> >> include/kunit/assert.h | 11 ++ >> lib/kunit/Makefile | 1 + >> lib/kunit/assert.c | 24 ++- >> lib/kunit/assert_test.c | 391 ++++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 419 insertions(+), 8 deletions(-) >> create mode 100644 lib/kunit/assert_test.c >> >> diff --git a/include/kunit/assert.h b/include/kunit/assert.h >> index 24c2b9fa61e8..7e7490a74b13 100644 >> --- a/include/kunit/assert.h >> +++ b/include/kunit/assert.h >> @@ -218,4 +218,15 @@ void kunit_mem_assert_format(const struct kunit_assert *assert, >> const struct va_format *message, >> struct string_stream *stream); >> >> +#if IS_ENABLED(CONFIG_KUNIT) >> +void kunit_assert_print_msg(const struct va_format *message, >> + struct string_stream *stream); >> +bool is_literal(const char *text, long long value); >> +bool is_str_literal(const char *text, const char *value); >> +void kunit_assert_hexdump(struct string_stream *stream, >> + const void *buf, >> + const void *compared_buf, >> + const size_t len); >> +#endif >> + >> #endif /* _KUNIT_ASSERT_H */ >> diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile >> index 309659a32a78..be7c9903936f 100644 >> --- a/lib/kunit/Makefile >> +++ b/lib/kunit/Makefile >> @@ -18,6 +18,7 @@ endif >> obj-y += hooks.o >> >> obj-$(CONFIG_KUNIT_TEST) += kunit-test.o >> +obj-$(CONFIG_KUNIT_TEST) += assert_test.o >> >> # string-stream-test compiles built-in only. >> ifeq ($(CONFIG_KUNIT_TEST),y) >> diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c >> index dd1d633d0fe2..382eb409d34b 100644 >> --- a/lib/kunit/assert.c >> +++ b/lib/kunit/assert.c >> @@ -7,6 +7,7 @@ >> */ >> #include <kunit/assert.h> >> #include <kunit/test.h> >> +#include <kunit/visibility.h> >> >> #include "string-stream.h" >> >> @@ -30,12 +31,14 @@ void kunit_assert_prologue(const struct kunit_loc *loc, >> } >> EXPORT_SYMBOL_GPL(kunit_assert_prologue); >> >> -static void kunit_assert_print_msg(const struct va_format *message, >> - struct string_stream *stream) >> +VISIBLE_IF_KUNIT >> +void kunit_assert_print_msg(const struct va_format *message, >> + struct string_stream *stream) >> { >> if (message->fmt) >> string_stream_add(stream, "\n%pV", message); >> } >> +EXPORT_SYMBOL_IF_KUNIT(kunit_assert_print_msg); >> >> void kunit_fail_assert_format(const struct kunit_assert *assert, >> const struct va_format *message, >> @@ -89,7 +92,7 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert, >> EXPORT_SYMBOL_GPL(kunit_ptr_not_err_assert_format); >> >> /* Checks if `text` is a literal representing `value`, e.g. "5" and 5 */ >> -static bool is_literal(const char *text, long long value) >> +VISIBLE_IF_KUNIT bool is_literal(const char *text, long long value) >> { >> char *buffer; >> int len; >> @@ -110,6 +113,7 @@ static bool is_literal(const char *text, long long value) >> >> return ret; >> } >> +EXPORT_SYMBOL_IF_KUNIT(is_literal); >> >> void kunit_binary_assert_format(const struct kunit_assert *assert, >> const struct va_format *message, >> @@ -166,7 +170,7 @@ EXPORT_SYMBOL_GPL(kunit_binary_ptr_assert_format); >> /* Checks if KUNIT_EXPECT_STREQ() args were string literals. >> * Note: `text` will have ""s where as `value` will not. >> */ >> -static bool is_str_literal(const char *text, const char *value) >> +VISIBLE_IF_KUNIT bool is_str_literal(const char *text, const char *value) >> { >> int len; >> >> @@ -178,6 +182,7 @@ static bool is_str_literal(const char *text, const char *value) >> >> return strncmp(text + 1, value, len - 2) == 0; >> } >> +EXPORT_SYMBOL_IF_KUNIT(is_str_literal); >> >> void kunit_binary_str_assert_format(const struct kunit_assert *assert, >> const struct va_format *message, >> @@ -208,10 +213,11 @@ EXPORT_SYMBOL_GPL(kunit_binary_str_assert_format); >> /* Adds a hexdump of a buffer to a string_stream comparing it with >> * a second buffer. The different bytes are marked with <>. >> */ >> -static void kunit_assert_hexdump(struct string_stream *stream, >> - const void *buf, >> - const void *compared_buf, >> - const size_t len) >> +VISIBLE_IF_KUNIT >> +void kunit_assert_hexdump(struct string_stream *stream, >> + const void *buf, >> + const void *compared_buf, >> + const size_t len) >> { >> size_t i; >> const u8 *buf1 = buf; >> @@ -229,6 +235,7 @@ static void kunit_assert_hexdump(struct string_stream *stream, >> string_stream_add(stream, " %02x ", buf1[i]); >> } >> } >> +EXPORT_SYMBOL_IF_KUNIT(kunit_assert_hexdump); >> >> void kunit_mem_assert_format(const struct kunit_assert *assert, >> const struct va_format *message, >> @@ -269,4 +276,5 @@ void kunit_mem_assert_format(const struct kunit_assert *assert, >> kunit_assert_print_msg(message, stream); >> } >> } >> + >> EXPORT_SYMBOL_GPL(kunit_mem_assert_format); >> diff --git a/lib/kunit/assert_test.c b/lib/kunit/assert_test.c >> new file mode 100644 >> index 000000000000..1347a964204b >> --- /dev/null >> +++ b/lib/kunit/assert_test.c >> @@ -0,0 +1,391 @@ >> +// SPDX-License-Identifier: GPL-2.0-or-later >> +/* >> + * KUnit test for the assertion formatting functions. >> + * Author: Ivan Orlov <ivan.orlov0322@gmail.com> >> + */ >> +#include <kunit/test.h> >> +#include "string-stream.h" >> + >> +#define TEST_PTR_EXPECTED_BUF_SIZE 32 >> +#define HEXDUMP_TEST_BUF_LEN 5 >> +#define ASSERT_TEST_EXPECT_CONTAIN(test, str, substr) KUNIT_EXPECT_TRUE(test, strstr(str, substr)) >> +#define ASSERT_TEST_EXPECT_NCONTAIN(test, str, substr) KUNIT_EXPECT_FALSE(test, strstr(str, substr)) >> + >> +static void kunit_test_is_literal(struct kunit *test) >> +{ >> + KUNIT_EXPECT_TRUE(test, is_literal("5", 5)); >> + KUNIT_EXPECT_TRUE(test, is_literal("0", 0)); >> + KUNIT_EXPECT_TRUE(test, is_literal("1234567890", 1234567890)); >> + KUNIT_EXPECT_TRUE(test, is_literal("-1234567890", -1234567890)); >> + KUNIT_EXPECT_FALSE(test, is_literal("05", 5)); >> + KUNIT_EXPECT_FALSE(test, is_literal("", 0)); >> + KUNIT_EXPECT_FALSE(test, is_literal("-0", 0)); >> + KUNIT_EXPECT_FALSE(test, is_literal("12#45", 1245)); >> +} >> + >> +static void kunit_test_is_str_literal(struct kunit *test) >> +{ >> + KUNIT_EXPECT_TRUE(test, is_str_literal("\"Hello, World!\"", "Hello, World!")); >> + KUNIT_EXPECT_TRUE(test, is_str_literal("\"\"", "")); >> + KUNIT_EXPECT_TRUE(test, is_str_literal("\"\"\"", "\"")); >> + KUNIT_EXPECT_FALSE(test, is_str_literal("", "")); >> + KUNIT_EXPECT_FALSE(test, is_str_literal("\"", "\"")); >> + KUNIT_EXPECT_FALSE(test, is_str_literal("\"Abacaba", "Abacaba")); >> + KUNIT_EXPECT_FALSE(test, is_str_literal("Abacaba\"", "Abacaba")); >> + KUNIT_EXPECT_FALSE(test, is_str_literal("\"Abacaba\"", "\"Abacaba\"")); >> +} >> + >> +KUNIT_DEFINE_ACTION_WRAPPER(kfree_wrapper, kfree, const void *); >> + >> +/* this function is used to get a "char *" string from the string stream and defer its cleanup */ >> +static char *get_str_from_stream(struct kunit *test, struct string_stream *stream) >> +{ >> + char *str = string_stream_get_string(stream); >> + > > When trying to make the kernel with this test loaded in, I am getting > an error that string_stream_get_string, string_stream_clear, and > kunit_alloc_string_stream are undefined. > > So either these three methods will have to be exported using > EXPORT_SYMBOL_KUNIT or this test cannot be loaded and run as a module. > > But once this is fixed up this should be good to go.
Hi Rae,
Thank you so much for the review.
At the moment, I believe the best approach would be to make this test depend on CONFIG_KUNIT_TEST=y (as it is done for string-stream-test).
However, I assume that every (standalone) test should be able to run as a module, and I'd like to add EXPORT_SYMBOL_IF_KUNIT to all of the non-static string-stream functions in a separate patch series. It will require updating string-stream-test.c as well (adding MODULE_IMPORT_NS). What do you think?
Thank you once again, -- Kind regards, Ivan Orlov
| |