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

[Ivan Orlov]

On 5/16/24 19:57, Rae Moar wrote:
> On Wed, May 15, 2024 at 10:20 AM 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 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.
> >
> > Signed-off-by: Ivan Orlov <ivan.orlov0322@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>. 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
> > V3 -> V4:
> > - Compile the assertion test only when CONFIG_KUNIT_TEST is set to 'y',
> > as it is done for the string-stream test. It is necessary since
> > functions from the string-stream are not exported into the public
> > namespace, and therefore they are not accessible when linking and
> > loading the test module.
>
> Hi Ivan!
>
> This looks great! Just one last thing, since this test is no longer
> loadable as a module, could you remove the exporting of new symbols
> and adding of the module license. Those can be part of the next patch,
> where we convert these tests to modules.
>
> Thanks!
> -Rae

Hi Rae,

Ah, sorry, I completely forgot to remove the module-related part. Thank 
you for the review, and I'll try to be more attentive next time :)

--
Kind regards,
Ivan Orlov