Re: [PATCH 4/4] kunit: Add example with alternate function redirection method

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 21.08.2024 23:22, Rae Moar wrote:
> On Wed, Aug 21, 2024 at 10:43 AM Michal Wajdeczko
> <michal.wajdeczko@intel.com> wrote:
>>
>> Add example how to use KUNIT_FIXED_STUB_REDIRECT and compare its
>> usage with the KUNIT_STATIC_STUB_REDIRECT. Also show how the
>> DECLARE_IF_KUNIT macro could be helpful in declaring test data in
>> the non-test data structures.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> 
> Hello!
> 
> I really like this test. It provides a great overview of this patch
> series. I just have a couple comments below.
> 
> Otherwise,
> Reviewed-by: Rae Moar <rmoar@google.com>
> 
> Thanks!
> -Rae
> 
>> ---
>> Cc: David Gow <davidgow@google.com>
>> Cc: Daniel Latypov <dlatypov@google.com>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  lib/kunit/kunit-example-test.c | 63 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 63 insertions(+)
>>
>> diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
>> index 3056d6bc705d..120e08d8899b 100644
>> --- a/lib/kunit/kunit-example-test.c
>> +++ b/lib/kunit/kunit-example-test.c
>> @@ -6,8 +6,10 @@
>>   * Author: Brendan Higgins <brendanhiggins@google.com>
>>   */
>>
>> +#include <linux/workqueue.h>
>>  #include <kunit/test.h>
>>  #include <kunit/static_stub.h>
>> +#include <kunit/visibility.h>
>>
>>  /*
>>   * This is the most fundamental element of KUnit, the test case. A test case
>> @@ -221,6 +223,66 @@ static void example_static_stub_using_fn_ptr_test(struct kunit *test)
>>         KUNIT_EXPECT_EQ(test, add_one(1), 2);
>>  }
>>
>> +/* This could be a location of various fixed stub functions. */
>> +static struct {
>> +       DECLARE_IF_KUNIT(int (*add_two)(int i));
> 
> Is the use of DECLARE_IF_KUNIT useful here? KUnit should always be
> enabled if this file is being compiled/run. Is the idea to show an
> example here that could be used outside of kunit test files?

yes, the idea was to show that 'stubs' declarations could be placed
anywhere, without any cost if compiled without KUNIT (I was trying to
mention that in commit message)

> 
> Additionally, would it make sense to call this add_two_stub instead to
> make it clear that this is not a definition of the add_two function?
> Or is it helpful for people to see this as an example of how to handle
> multiple stubs: struct of stubs with exact names? Let me know what you
> think.

the 'add_two' above is just a member name, and IMO we shouldn't repeat
that this is about 'stub' since the whole struct is for 'stubs'

and yes, the idea was also to show that if applicable, other function
stubs declarations could be either placed together

> 
>> +} stubs;
>> +
>> +/* This is a function we'll replace with stubs. */
>> +static int add_two(int i)
>> +{
>> +       /* This will trigger the stub if active. */
>> +       KUNIT_STATIC_STUB_REDIRECT(add_two, i);
>> +       KUNIT_FIXED_STUB_REDIRECT(stubs.add_two, i);
>> +
>> +       return i + 2;
>> +}
>> +
>> +struct add_two_async_work {
>> +       struct work_struct work;
>> +       int param;
>> +       int result;
>> +};
>> +
>> +static void add_two_async_func(struct work_struct *work)
>> +{
>> +       struct add_two_async_work *w = container_of(work, typeof(*w), work);
>> +
>> +       w->result = add_two(w->param);
>> +}
>> +
>> +static int add_two_async(int i)
>> +{
>> +       struct add_two_async_work w = { .param = i };
>> +
>> +       INIT_WORK_ONSTACK(&w.work, add_two_async_func);
>> +       schedule_work(&w.work);
>> +       flush_work(&w.work);
>> +       destroy_work_on_stack(&w.work);
>> +
>> +       return w.result;
>> +}
>> +
>> +/*
>> + */
> 
> It looks like the method description is missing here.
> 

ha, I missed to copy commit message here

> 
> 
> 
>> +static void example_fixed_stub_test(struct kunit *test)
>> +{
>> +       /* static stub redirection works only for KUnit thread */
>> +       kunit_activate_static_stub(test, add_two, subtract_one);
>> +       KUNIT_EXPECT_EQ(test, add_two(1), subtract_one(1));
>> +       KUNIT_EXPECT_NE_MSG(test, add_two_async(1), subtract_one(1),
>> +                           "stub shouldn't be active outside KUnit thread!");
>> +       kunit_deactivate_static_stub(test, add_two);
>> +       KUNIT_EXPECT_EQ(test, add_two(1), add_two(1));
>> +
>> +       /* fixed stub redirection works for KUnit and other threads */
>> +       kunit_activate_fixed_stub(test, stubs.add_two, subtract_one);
>> +       KUNIT_EXPECT_EQ(test, add_two(1), subtract_one(1));
>> +       KUNIT_EXPECT_EQ(test, add_two_async(1), subtract_one(1));
>> +       kunit_deactivate_fixed_stub(test, stubs.add_two);
>> +       KUNIT_EXPECT_EQ(test, add_two(1), add_two(1));
>> +}
>> +
>>  static const struct example_param {
>>         int value;
>>  } example_params_array[] = {
>> @@ -294,6 +356,7 @@ static struct kunit_case example_test_cases[] = {
>>         KUNIT_CASE(example_all_expect_macros_test),
>>         KUNIT_CASE(example_static_stub_test),
>>         KUNIT_CASE(example_static_stub_using_fn_ptr_test),
>> +       KUNIT_CASE(example_fixed_stub_test),
>>         KUNIT_CASE(example_priv_test),
>>         KUNIT_CASE_PARAM(example_params_test, example_gen_params),
>>         KUNIT_CASE_SLOW(example_slow_test),
>> --
>> 2.43.0
>>
>> --
>> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20240821144305.1958-5-michal.wajdeczko%40intel.com.