Re: [PATCH 3/3] kunit: Update reporting function to support results from subtests

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

 



On Tue, Apr 11, 2023 at 12:01 PM Michal Wajdeczko
<michal.wajdeczko@xxxxxxxxx> wrote:
>
> There is function to report status of either suite or test, but it
> doesn't support parameterized subtests that have to log report on
> its own. Extend it to also accept subtest level results to avoid
> code duplication.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> Cc: David Gow <davidgow@xxxxxxxxxx>
> ---
>  lib/kunit/test.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
>
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 5679197b5f8a..692fce258c5b 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -154,8 +154,14 @@ static void kunit_print_suite_start(struct kunit_suite *suite)
>                   kunit_suite_num_test_cases(suite));
>  }
>
> +enum kunit_test_or_suite {
> +       KUNIT_SUITE = 0,
> +       KUNIT_TEST,
> +       KUNIT_SUBTEST,
> +};
> +

Hi Michal!

Since KUnit's goal is to progress toward supporting arbitrary levels
of testing, I like the idea of starting to adjust these helper
functions to allow for greater levels of testing.

However, I'm not sure about this kunit_test_or_suite enum. If our goal
is to support an arbitrary number of levels of tests then this enum
still limits us to a finite number of levels. However, if we only want
to focus on supporting parameterized tests (which is our direct goal),
this could be the right solution.

Maybe instead we could use an integer denoting the test level instead?
This would remove the limit but would also remove the nice names of
the levels.

I'm curious what others opinions are on these ideas?

A bit of a nit: if we do use this enum I wonder if we could clarify
the name to be kunit_test_level as the current name of
kunit_test_or_suite seems to indicate to me a binary of two options
rather than three.

>  static void kunit_print_ok_not_ok(void *test_or_suite,
> -                                 bool is_test,
> +                                 enum kunit_test_or_suite is_test,

Similar to above, I think the name of is_test could be clarified. It
is currently a bit confusing to me as they are all tests. Maybe
test_level?

>                                   enum kunit_status status,
>                                   size_t test_number,
>                                   const char *description,
> @@ -180,7 +186,9 @@ static void kunit_print_ok_not_ok(void *test_or_suite,
>                         (status == KUNIT_SKIPPED) ? directive : "");
>         else
>                 kunit_log(KERN_INFO, test,
> -                         KUNIT_SUBTEST_INDENT "%s %zd %s%s%s",
> +                         "%.*s%s %zd %s%s%s",
> +                         (int) strlen(KUNIT_SUBTEST_INDENT) * is_test,

I would consider saving the length of KUNIT_SUBTEST_INDENT as a macro.
Maybe KUNIT_INDENT_LEN?

> +                         KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT,
>                           kunit_status_to_ok_not_ok(status),
>                           test_number, description, directive_header,
>                           (status == KUNIT_SKIPPED) ? directive : "");
> @@ -209,7 +217,7 @@ static size_t kunit_suite_counter = 1;
>
>  static void kunit_print_suite_end(struct kunit_suite *suite)
>  {
> -       kunit_print_ok_not_ok((void *)suite, false,
> +       kunit_print_ok_not_ok((void *)suite, KUNIT_SUITE,
>                               kunit_suite_has_succeeded(suite),
>                               kunit_suite_counter++,
>                               suite->name,
> @@ -554,13 +562,11 @@ int kunit_run_tests(struct kunit_suite *suite)
>                                                  "param-%d", test.param_index);
>                                 }
>
> -                               kunit_log(KERN_INFO, &test,
> -                                         KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT
> -                                         "%s %d %s%s%s",
> -                                         kunit_status_to_ok_not_ok(test.status),
> -                                         test.param_index + 1, param_desc,
> -                                         test.status == KUNIT_SKIPPED ? " # SKIP " : "",
> -                                         test.status == KUNIT_SKIPPED ? test.status_comment : "");
> +                               kunit_print_ok_not_ok(&test, KUNIT_SUBTEST,
> +                                                     test.status,
> +                                                     test.param_index + 1,
> +                                                     param_desc,
> +                                                     test.status_comment);
>
>                                 /* Get next param. */
>                                 param_desc[0] = '\0';
> @@ -574,7 +580,7 @@ int kunit_run_tests(struct kunit_suite *suite)
>
>                 kunit_print_test_stats(&test, param_stats);
>
> -               kunit_print_ok_not_ok(&test, true, test_case->status,
> +               kunit_print_ok_not_ok(&test, KUNIT_TEST, test_case->status,
>                                       kunit_test_case_num(suite, test_case),
>                                       test_case->name,
>                                       test.status_comment);
> --
> 2.25.1
>
> --
> 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@xxxxxxxxxxxxxxxx.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230411160056.1586-4-michal.wajdeczko%40intel.com.




[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