[PATCH 0/3] kunit: Fix reporting of the skipped parameterized tests

[PATCH 0/3] kunit: Fix reporting of the skipped parameterized tests

[Michal Wajdeczko]

Due to the lack of the SKIP directive in the output, if any of the
parameterized test was skipped, the parser could not recognize that
correctly and was marking the test as PASSED.

This can easily be seen by running the new subtest from patch 1:

$ ./tools/testing/kunit/kunit.py run \
	--kunitconfig ./lib/kunit/.kunitconfig *.example_params*

  [ ] Starting KUnit Kernel (1/1)...
  [ ] ============================================================
  [ ] =================== example (1 subtest) ====================
  [ ] =================== example_params_test  ===================
  [ ] [PASSED] example value 2
  [ ] [PASSED] example value 1
  [ ] [PASSED] example value 0
  [ ] =============== [PASSED] example_params_test ===============
  [ ] ===================== [PASSED] example =====================
  [ ] ============================================================
  [ ] Testing complete. Ran 3 tests: passed: 3

$ ./tools/testing/kunit/kunit.py run \
	--kunitconfig ./lib/kunit/.kunitconfig *.example_params* \
	--raw_output

  [ ] Starting KUnit Kernel (1/1)...
  KTAP version 1
  1..1
      # example: initializing suite
      KTAP version 1
      # Subtest: example
      1..1
          KTAP version 1
          # Subtest: example_params_test
      # example_params_test: initializing
          ok 1 example value 2
      # example_params_test: initializing
          ok 2 example value 1
      # example_params_test: initializing
          ok 3 example value 0
      # example_params_test: pass:2 fail:0 skip:1 total:3
      ok 1 example_params_test
  # Totals: pass:2 fail:0 skip:1 total:3
  ok 1 example

After adding the SKIP directive, the report looks as expected:

  [ ] Starting KUnit Kernel (1/1)...
  [ ] ============================================================
  [ ] =================== example (1 subtest) ====================
  [ ] =================== example_params_test  ===================
  [ ] [PASSED] example value 2
  [ ] [PASSED] example value 1
  [ ] [SKIPPED] example value 0
  [ ] =============== [PASSED] example_params_test ===============
  [ ] ===================== [PASSED] example =====================
  [ ] ============================================================
  [ ] Testing complete. Ran 3 tests: passed: 2, skipped: 1

  [ ] Starting KUnit Kernel (1/1)...
  KTAP version 1
  1..1
      # example: initializing suite
      KTAP version 1
      # Subtest: example
      1..1
          KTAP version 1
          # Subtest: example_params_test
      # example_params_test: initializing
          ok 1 example value 2
      # example_params_test: initializing
          ok 2 example value 1
      # example_params_test: initializing
          ok 3 example value 0 # SKIP unsupported param value
      # example_params_test: pass:2 fail:0 skip:1 total:3
      ok 1 example_params_test
  # Totals: pass:2 fail:0 skip:1 total:3
  ok 1 example

Cc: David Gow <davidgow@google.com>

Michal Wajdeczko (3):
  kunit/test: Add example test showing parameterized testing
  kunit: Fix reporting of the skipped parameterized tests
  kunit: Update reporting function to support results from subtests

 lib/kunit/kunit-example-test.c | 34 ++++++++++++++++++++++++++++++++++
 lib/kunit/test.c               | 26 +++++++++++++++++---------
 2 files changed, 51 insertions(+), 9 deletions(-)

-- 
2.25.1

[PATCH 1/3] kunit/test: Add example test showing parameterized testing

[Michal Wajdeczko]

Use of parameterized testing is documented [1] but such use case
is not present in demo kunit test. Add small subtest for that.

[1] https://kernel.org/doc/html/latest/dev-tools/kunit/usage.html#parameterized-testing

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: David Gow <davidgow@google.com>
---
 lib/kunit/kunit-example-test.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
index cd8b7e51d02b..775443f77763 100644
--- a/lib/kunit/kunit-example-test.c
+++ b/lib/kunit/kunit-example-test.c
@@ -167,6 +167,39 @@ static void example_static_stub_test(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, add_one(1), 2);
 }
 
+static const struct example_param {
+	int value;
+} example_params_array[] = {
+	{ .value = 2, },
+	{ .value = 1, },
+	{ .value = 0, },
+};
+
+static void example_param_get_desc(const struct example_param *p, char *desc)
+{
+	snprintf(desc, KUNIT_PARAM_DESC_SIZE, "example value %d", p->value);
+}
+
+KUNIT_ARRAY_PARAM(example, example_params_array, example_param_get_desc);
+
+/*
+ * This test shows the use of params.
+ */
+static void example_params_test(struct kunit *test)
+{
+	const struct example_param *param = test->param_value;
+
+	/* By design, param pointer will not be NULL */
+	KUNIT_ASSERT_NOT_NULL(test, param);
+
+	/* Test can be skipped on unsupported param values */
+	if (!param->value)
+		kunit_skip(test, "unsupported param value");
+
+	/* You can use param values for parameterized testing */
+	KUNIT_EXPECT_EQ(test, param->value % param->value, 0);
+}
+
 /*
  * Here we make a list of all the test cases we want to add to the test suite
  * below.
@@ -183,6 +216,7 @@ static struct kunit_case example_test_cases[] = {
 	KUNIT_CASE(example_mark_skipped_test),
 	KUNIT_CASE(example_all_expect_macros_test),
 	KUNIT_CASE(example_static_stub_test),
+	KUNIT_CASE_PARAM(example_params_test, example_gen_params),
 	{}
 };
 
-- 
2.25.1

Re: [PATCH 1/3] kunit/test: Add example test showing parameterized testing

[Rae Moar]

On Tue, Apr 11, 2023 at 12:01 PM Michal Wajdeczko
<michal.wajdeczko@intel.com> wrote:
>
> Use of parameterized testing is documented [1] but such use case
> is not present in demo kunit test. Add small subtest for that.
>
> [1] https://kernel.org/doc/html/latest/dev-tools/kunit/usage.html#parameterized-testing
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: David Gow <davidgow@google.com>
> ---

Hello!

This looks all pretty good to me! I only have one comment. In the KTAP output:

        KTAP version 1
        # Subtest: example_params_test
    # example_params_test: initializing
        ok 1 example value 2
    # example_params_test: initializing
        ok 2 example value 1
    # example_params_test: initializing
       ok 3 example value 0 # SKIP unsupported param value
    # example_params_test: pass:2 fail:0 skip:1 total:3
    ok 6 example_params_test

The init method is causing the "# example_params_test: initializing"
to print lines for each case. However, since they are not inline with
the correct indentation and they don't include helpful test data, I
would consider finding a way to remove these.

We could consider removing these lines from the test suite as a whole.
However, they are helpful in that they show how to use the init
function. Maybe check if the test is a param test case in the init
function itself? Let me know what you think.

Thanks!

Rae

>  lib/kunit/kunit-example-test.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
> index cd8b7e51d02b..775443f77763 100644
> --- a/lib/kunit/kunit-example-test.c
> +++ b/lib/kunit/kunit-example-test.c
> @@ -167,6 +167,39 @@ static void example_static_stub_test(struct kunit *test)
>         KUNIT_EXPECT_EQ(test, add_one(1), 2);
>  }
>
> +static const struct example_param {
> +       int value;
> +} example_params_array[] = {
> +       { .value = 2, },
> +       { .value = 1, },
> +       { .value = 0, },
> +};
> +
> +static void example_param_get_desc(const struct example_param *p, char *desc)
> +{
> +       snprintf(desc, KUNIT_PARAM_DESC_SIZE, "example value %d", p->value);
> +}
> +
> +KUNIT_ARRAY_PARAM(example, example_params_array, example_param_get_desc);
> +
> +/*
> + * This test shows the use of params.
> + */
> +static void example_params_test(struct kunit *test)
> +{
> +       const struct example_param *param = test->param_value;
> +
> +       /* By design, param pointer will not be NULL */
> +       KUNIT_ASSERT_NOT_NULL(test, param);
> +
> +       /* Test can be skipped on unsupported param values */
> +       if (!param->value)
> +               kunit_skip(test, "unsupported param value");
> +
> +       /* You can use param values for parameterized testing */
> +       KUNIT_EXPECT_EQ(test, param->value % param->value, 0);
> +}
> +
>  /*
>   * Here we make a list of all the test cases we want to add to the test suite
>   * below.
> @@ -183,6 +216,7 @@ static struct kunit_case example_test_cases[] = {
>         KUNIT_CASE(example_mark_skipped_test),
>         KUNIT_CASE(example_all_expect_macros_test),
>         KUNIT_CASE(example_static_stub_test),
> +       KUNIT_CASE_PARAM(example_params_test, example_gen_params),
>         {}
>  };
>
> --
> 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@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230411160056.1586-2-michal.wajdeczko%40intel.com.

Re: [PATCH 1/3] kunit/test: Add example test showing parameterized testing

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 1953 bytes --]

On Thu, 13 Apr 2023 at 04:51, Rae Moar <rmoar@google.com> wrote:
>
> On Tue, Apr 11, 2023 at 12:01 PM Michal Wajdeczko
> <michal.wajdeczko@intel.com> wrote:
> >
> > Use of parameterized testing is documented [1] but such use case
> > is not present in demo kunit test. Add small subtest for that.
> >
> > [1] https://kernel.org/doc/html/latest/dev-tools/kunit/usage.html#parameterized-testing
> >
> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > Cc: David Gow <davidgow@google.com>
> > ---
>
> Hello!
>
> This looks all pretty good to me! I only have one comment. In the KTAP output:
>
>         KTAP version 1
>         # Subtest: example_params_test
>     # example_params_test: initializing
>         ok 1 example value 2
>     # example_params_test: initializing
>         ok 2 example value 1
>     # example_params_test: initializing
>        ok 3 example value 0 # SKIP unsupported param value
>     # example_params_test: pass:2 fail:0 skip:1 total:3
>     ok 6 example_params_test
>
> The init method is causing the "# example_params_test: initializing"
> to print lines for each case. However, since they are not inline with
> the correct indentation and they don't include helpful test data, I
> would consider finding a way to remove these.

I think this is probably a problem for the kunit_log() infrastructure,
rather than init functions in general.

I'm not worried about them in relation to this particular test.

>
> We could consider removing these lines from the test suite as a whole.
> However, they are helpful in that they show how to use the init
> function. Maybe check if the test is a param test case in the init
> function itself? Let me know what you think.
>

I'd rather keep these as-is: the idea is to have a very simple example
of an init function, and complicating further by checking which test
is running is needless complexity, IMO.

-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH 1/3] kunit/test: Add example test showing parameterized testing

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 2825 bytes --]

On Wed, 12 Apr 2023 at 00:01, Michal Wajdeczko
<michal.wajdeczko@intel.com> wrote:
>
> Use of parameterized testing is documented [1] but such use case
> is not present in demo kunit test. Add small subtest for that.
>
> [1] https://kernel.org/doc/html/latest/dev-tools/kunit/usage.html#parameterized-testing
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: David Gow <davidgow@google.com>
> ---

Thanks: this is a long-overdue addition to the KUnit examples.

Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David


>  lib/kunit/kunit-example-test.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
> index cd8b7e51d02b..775443f77763 100644
> --- a/lib/kunit/kunit-example-test.c
> +++ b/lib/kunit/kunit-example-test.c
> @@ -167,6 +167,39 @@ static void example_static_stub_test(struct kunit *test)
>         KUNIT_EXPECT_EQ(test, add_one(1), 2);
>  }
>
> +static const struct example_param {
> +       int value;
> +} example_params_array[] = {
> +       { .value = 2, },
> +       { .value = 1, },
> +       { .value = 0, },
> +};
> +
> +static void example_param_get_desc(const struct example_param *p, char *desc)
> +{
> +       snprintf(desc, KUNIT_PARAM_DESC_SIZE, "example value %d", p->value);
> +}
> +
> +KUNIT_ARRAY_PARAM(example, example_params_array, example_param_get_desc);
> +
> +/*
> + * This test shows the use of params.
> + */
> +static void example_params_test(struct kunit *test)
> +{
> +       const struct example_param *param = test->param_value;
> +
> +       /* By design, param pointer will not be NULL */
> +       KUNIT_ASSERT_NOT_NULL(test, param);
> +
> +       /* Test can be skipped on unsupported param values */
> +       if (!param->value)
> +               kunit_skip(test, "unsupported param value");
> +
> +       /* You can use param values for parameterized testing */
> +       KUNIT_EXPECT_EQ(test, param->value % param->value, 0);
> +}
> +
>  /*
>   * Here we make a list of all the test cases we want to add to the test suite
>   * below.
> @@ -183,6 +216,7 @@ static struct kunit_case example_test_cases[] = {
>         KUNIT_CASE(example_mark_skipped_test),
>         KUNIT_CASE(example_all_expect_macros_test),
>         KUNIT_CASE(example_static_stub_test),
> +       KUNIT_CASE_PARAM(example_params_test, example_gen_params),
>         {}
>  };
>
> --
> 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@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230411160056.1586-2-michal.wajdeczko%40intel.com.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

[PATCH 2/3] kunit: Fix reporting of the skipped parameterized tests

[Michal Wajdeczko]

Logs from the parameterized tests that were skipped don't include
SKIP directive thus they are displayed as PASSED. Fix that.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: David Gow <davidgow@google.com>
---
 lib/kunit/test.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index c9e15bb60058..5679197b5f8a 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -556,9 +556,11 @@ int kunit_run_tests(struct kunit_suite *suite)
 
 				kunit_log(KERN_INFO, &test,
 					  KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT
-					  "%s %d %s",
+					  "%s %d %s%s%s",
 					  kunit_status_to_ok_not_ok(test.status),
-					  test.param_index + 1, param_desc);
+					  test.param_index + 1, param_desc,
+					  test.status == KUNIT_SKIPPED ? " # SKIP " : "",
+					  test.status == KUNIT_SKIPPED ? test.status_comment : "");
 
 				/* Get next param. */
 				param_desc[0] = '\0';
-- 
2.25.1

Re: [PATCH 2/3] kunit: Fix reporting of the skipped parameterized tests

[Rae Moar]

On Tue, Apr 11, 2023 at 12:01 PM Michal Wajdeczko
<michal.wajdeczko@intel.com> wrote:
>
> Logs from the parameterized tests that were skipped don't include
> SKIP directive thus they are displayed as PASSED. Fix that.

Hi Michal!

This fix looks good to me. Thanks for fixing this!

The only comment I would have for this patch is if we should consider
using an altered version of kunit_print_ok_not_ok() here instead.
However, it seems you address this in the next patch.

Thanks again,
Rae

Reviewed-by: Rae Moar <rmoar@google.com>

>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: David Gow <davidgow@google.com>
> ---
>  lib/kunit/test.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index c9e15bb60058..5679197b5f8a 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -556,9 +556,11 @@ int kunit_run_tests(struct kunit_suite *suite)
>
>                                 kunit_log(KERN_INFO, &test,
>                                           KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT
> -                                         "%s %d %s",
> +                                         "%s %d %s%s%s",
>                                           kunit_status_to_ok_not_ok(test.status),
> -                                         test.param_index + 1, param_desc);
> +                                         test.param_index + 1, param_desc,
> +                                         test.status == KUNIT_SKIPPED ? " # SKIP " : "",
> +                                         test.status == KUNIT_SKIPPED ? test.status_comment : "");
>
>                                 /* Get next param. */
>                                 param_desc[0] = '\0';
> --
> 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@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230411160056.1586-3-michal.wajdeczko%40intel.com.

Re: [PATCH 2/3] kunit: Fix reporting of the skipped parameterized tests

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 1910 bytes --]

On Wed, 12 Apr 2023 at 00:01, Michal Wajdeczko
<michal.wajdeczko@intel.com> wrote:
>
> Logs from the parameterized tests that were skipped don't include
> SKIP directive thus they are displayed as PASSED. Fix that.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: David Gow <davidgow@google.com>
> ---

Nice catch, thanks!

Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David

>  lib/kunit/test.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index c9e15bb60058..5679197b5f8a 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -556,9 +556,11 @@ int kunit_run_tests(struct kunit_suite *suite)
>
>                                 kunit_log(KERN_INFO, &test,
>                                           KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT
> -                                         "%s %d %s",
> +                                         "%s %d %s%s%s",
>                                           kunit_status_to_ok_not_ok(test.status),
> -                                         test.param_index + 1, param_desc);
> +                                         test.param_index + 1, param_desc,
> +                                         test.status == KUNIT_SKIPPED ? " # SKIP " : "",
> +                                         test.status == KUNIT_SKIPPED ? test.status_comment : "");
>
>                                 /* Get next param. */
>                                 param_desc[0] = '\0';
> --
> 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@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230411160056.1586-3-michal.wajdeczko%40intel.com.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

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

[Michal Wajdeczko]

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@intel.com>
Cc: David Gow <davidgow@google.com>
---
 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,
+};
+
 static void kunit_print_ok_not_ok(void *test_or_suite,
-				  bool is_test,
+				  enum kunit_test_or_suite is_test,
 				  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,
+			  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

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

[Rae Moar]

On Tue, Apr 11, 2023 at 12:01 PM Michal Wajdeczko
<michal.wajdeczko@intel.com> 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@intel.com>
> Cc: David Gow <davidgow@google.com>
> ---
>  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@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230411160056.1586-4-michal.wajdeczko%40intel.com.

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

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 7468 bytes --]

On Thu, 13 Apr 2023 at 04:28, Rae Moar <rmoar@google.com> wrote:
>
> On Tue, Apr 11, 2023 at 12:01 PM Michal Wajdeczko
> <michal.wajdeczko@intel.com> 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@intel.com>
> > Cc: David Gow <davidgow@google.com>
> > ---
> >  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?

I agree with Rae that this is not the ideal long-term solution.

We're basically encoding two things here:
- Are we dealing with a 'struct kunit_suite' or a 'struct kunit'?
- How nested the test is.

Given KUnit originally only had a 2-level nesting (suite->test), and
now has 3-level nesting (always suite->test[->param]), this works, but
the KTAP format permits arbitrary nesting of tests, and we'll want to
have something like that in KUnit going forward. We don't have a
design for that yet, but it could conceivably allow nested suites,
thus breaking the rule that nesting level 0 is always a suite, and the
rest are all tests.

So there's definitely a part of me that would prefer to pass those two
pieces of information in separate arguments, rather than relying on an
enum like this.

That being said, this is all very fuzzy future plans, rather than
anything concrete, and this will all likely need reworking if we do
anything drastic anyway, so I'm not worried if we go with this for
now, and change it when we need to. I do think it's an improvement
over what we're doing currently.
(For example, another possible implementation for nested tests could
be to get rid of the distinction between tests and suites completely:
or at least have them share 'struct kunit', so this wouldn't need
passing in separately.)

Cheers,
-- David


>
> >                                   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@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230411160056.1586-4-michal.wajdeczko%40intel.com.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

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

[Michal Wajdeczko]

On 13.04.2023 08:31, David Gow wrote:
> On Thu, 13 Apr 2023 at 04:28, Rae Moar <rmoar@google.com> wrote:
>>
>> On Tue, Apr 11, 2023 at 12:01 PM Michal Wajdeczko
>> <michal.wajdeczko@intel.com> 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@intel.com>
>>> Cc: David Gow <davidgow@google.com>
>>> ---
>>>  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.

we can use integer as param but at the same time we can also have define
or anonymous enum as nice aliases to currently known/used levels:

/* Currently supported test levels */
enum {
	KUNIT_LEVEL_SUITE = 0,
	KUNIT_LEVEL_TEST,
	KUNIT_LEVEL_PARAMTEST,
};

/* Future levels are TBD */
#define KUNIT_LEVEL_SUBTEST(n)	(KUNIT_LEVEL_TEST + (n))

>>
>> 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?

ok

> 
> I agree with Rae that this is not the ideal long-term solution.
> 
> We're basically encoding two things here:
> - Are we dealing with a 'struct kunit_suite' or a 'struct kunit'?
> - How nested the test is.
> 
> Given KUnit originally only had a 2-level nesting (suite->test), and
> now has 3-level nesting (always suite->test[->param]), this works, but
> the KTAP format permits arbitrary nesting of tests, and we'll want to
> have something like that in KUnit going forward. We don't have a
> design for that yet, but it could conceivably allow nested suites,
> thus breaking the rule that nesting level 0 is always a suite, and the
> rest are all tests.

I guess "We don't have a design for that yet" is a key here

> 
> So there's definitely a part of me that would prefer to pass those two
> pieces of information in separate arguments, rather than relying on an
> enum like this.
> 
> That being said, this is all very fuzzy future plans, rather than
> anything concrete, and this will all likely need reworking if we do
> anything drastic anyway, so I'm not worried if we go with this for
> now, and change it when we need to. I do think it's an improvement
> over what we're doing currently.
> (For example, another possible implementation for nested tests could
> be to get rid of the distinction between tests and suites completely:
> or at least have them share 'struct kunit', so this wouldn't need
> passing in separately.)

maybe the problem will go away once we just replace this untyped param:

	kunit_print_ok_not_ok(void *test_or_suite, ...

with proper type:

	kunit_print_ok_not_ok(struct kunit *test, ...

and treat NULL as indication that we just want to print raw results (as
it looks function is not using any suite attributes directly, all input
is passed explicitly and kunit_log() expects test only)

Thanks,
Michal

> 
> Cheers,
> -- David
> 
> 
>>
>>>                                   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@googlegroups.com.
>>> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230411160056.1586-4-michal.wajdeczko%40intel.com.

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

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 5528 bytes --]

On Thu, 13 Apr 2023 at 21:15, Michal Wajdeczko
<michal.wajdeczko@intel.com> wrote:
>
>
>
> On 13.04.2023 08:31, David Gow wrote:
> > On Thu, 13 Apr 2023 at 04:28, Rae Moar <rmoar@google.com> wrote:
> >>
> >> On Tue, Apr 11, 2023 at 12:01 PM Michal Wajdeczko
> >> <michal.wajdeczko@intel.com> 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@intel.com>
> >>> Cc: David Gow <davidgow@google.com>
> >>> ---
> >>>  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.
>
> we can use integer as param but at the same time we can also have define
> or anonymous enum as nice aliases to currently known/used levels:
>
> /* Currently supported test levels */
> enum {
>         KUNIT_LEVEL_SUITE = 0,
>         KUNIT_LEVEL_TEST,
>         KUNIT_LEVEL_PARAMTEST,
> };
>
> /* Future levels are TBD */
> #define KUNIT_LEVEL_SUBTEST(n)  (KUNIT_LEVEL_TEST + (n))
>

This sounds good to me!

> >>
> >> 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?
>
> ok
>
> >
> > I agree with Rae that this is not the ideal long-term solution.
> >
> > We're basically encoding two things here:
> > - Are we dealing with a 'struct kunit_suite' or a 'struct kunit'?
> > - How nested the test is.
> >
> > Given KUnit originally only had a 2-level nesting (suite->test), and
> > now has 3-level nesting (always suite->test[->param]), this works, but
> > the KTAP format permits arbitrary nesting of tests, and we'll want to
> > have something like that in KUnit going forward. We don't have a
> > design for that yet, but it could conceivably allow nested suites,
> > thus breaking the rule that nesting level 0 is always a suite, and the
> > rest are all tests.
>
> I guess "We don't have a design for that yet" is a key here

Yeah: I wouldn't worry too much about what the future design would
bring, tbh. As long as we don't totally paint ourselves into a corner
(and I don't think anything in this patch is at risk of doing that),
then doing whatever is sensible for the way things are now works.

>
> >
> > So there's definitely a part of me that would prefer to pass those two
> > pieces of information in separate arguments, rather than relying on an
> > enum like this.
> >
> > That being said, this is all very fuzzy future plans, rather than
> > anything concrete, and this will all likely need reworking if we do
> > anything drastic anyway, so I'm not worried if we go with this for
> > now, and change it when we need to. I do think it's an improvement
> > over what we're doing currently.
> > (For example, another possible implementation for nested tests could
> > be to get rid of the distinction between tests and suites completely:
> > or at least have them share 'struct kunit', so this wouldn't need
> > passing in separately.)
>
> maybe the problem will go away once we just replace this untyped param:
>
>         kunit_print_ok_not_ok(void *test_or_suite, ...
>
> with proper type:
>
>         kunit_print_ok_not_ok(struct kunit *test, ...
>
> and treat NULL as indication that we just want to print raw results (as
> it looks function is not using any suite attributes directly, all input
> is passed explicitly and kunit_log() expects test only)
>

Yeah: I think that makes sense. Suite results are still handled
separately in debugfs as well.

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

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

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 5739 bytes --]

On Wed, 12 Apr 2023 at 00:01, Michal Wajdeczko
<michal.wajdeczko@intel.com> 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@intel.com>
> Cc: David Gow <davidgow@google.com>
> ---

Thanks: this is definitely an improvement on how we handle this.

There's definitely more we can do, particularly looking forward to
supporting more complex test hierarchies in the future, but getting
everything under kunit_print_ok_not_ok is an improvement regardless of
when happens down the line.

My only real concern is that the way the indent is printed is a bit
subtle and difficult to understand fully on first glance. I've added
some notes below.

>  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,
> +};
> +

As Rae notes, this probably won't be how this code eventually evolves.
I don't think it's a problem to have it now, though.

>  static void kunit_print_ok_not_ok(void *test_or_suite,
> -                                 bool is_test,
> +                                 enum kunit_test_or_suite is_test,
>                                   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,
> +                         KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT,

This feels a little bit _too_ clever here: I feel it at the very least
needs a comment, and maybe it'd make more sense to either:
- Make is_test explicitly a "nesting depth" integer, and calculate the
indent based on that.
- Have is_test as an enum, and then just explicitly handle each value
separately. (Like we do with suite vs test).

I think that the former is probably the right long-term solution (it's
much more extensible to more levels of nesting), but the latter is
definitely easier given the differences between suites and tests at
the moment.

If we do continue to share a codepath between tests and subtests, I'd
prefer it if we either didn't use strlen(), or went to some greater
effort to document how that works (hopefully we can guarantee that the
compiler will treat this as a constant). Equally, a comment or
something noting that this will read invalid memory if is_test > 2,
due to the hardcoded two KUNIT_SUBTEST_INDENT, would be nice.


>                           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
>

Otherwise, this all looks good to me. Thanks very much!

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

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

[Michal Wajdeczko]

On 13.04.2023 08:38, David Gow wrote:
> On Wed, 12 Apr 2023 at 00:01, Michal Wajdeczko
> <michal.wajdeczko@intel.com> 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@intel.com>
>> Cc: David Gow <davidgow@google.com>
>> ---
> 
> Thanks: this is definitely an improvement on how we handle this.
> 
> There's definitely more we can do, particularly looking forward to
> supporting more complex test hierarchies in the future, but getting
> everything under kunit_print_ok_not_ok is an improvement regardless of
> when happens down the line.
> 
> My only real concern is that the way the indent is printed is a bit
> subtle and difficult to understand fully on first glance. I've added
> some notes below.
> 
>>  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,
>> +};
>> +
> 
> As Rae notes, this probably won't be how this code eventually evolves.
> I don't think it's a problem to have it now, though.
> 
>>  static void kunit_print_ok_not_ok(void *test_or_suite,
>> -                                 bool is_test,
>> +                                 enum kunit_test_or_suite is_test,
>>                                   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,
>> +                         KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT,
> 
> This feels a little bit _too_ clever here: I feel it at the very least
> needs a comment, and maybe it'd make more sense to either:
> - Make is_test explicitly a "nesting depth" integer, and calculate the
> indent based on that.
> - Have is_test as an enum, and then just explicitly handle each value
> separately. (Like we do with suite vs test).
> 
> I think that the former is probably the right long-term solution (it's
> much more extensible to more levels of nesting), but the latter is
> definitely easier given the differences between suites and tests at
> the moment.
> 
> If we do continue to share a codepath between tests and subtests, I'd
> prefer it if we either didn't use strlen(), or went to some greater

Rae suggested to define KUNIT_INDENT_LEN 4 and I will put it in test.h

> effort to document how that works (hopefully we can guarantee that the
> compiler will treat this as a constant). Equally, a comment or
> something noting that this will read invalid memory if is_test > 2,
> due to the hardcoded two KUNIT_SUBTEST_INDENT, would be nice.

that shouldn't happen as %.*s specifies precision and it's used here to
clamp string with more than needed indents, it will not try to read
beyond terminating \0

but since you plan for arbitrary level of testing I could change that to
a little simpler variant %*s which should always with any level:

		kunit_log(KERN_INFO, test,
			  "%*s%s %zd %s%s%s",
			  KUNIT_INDENT_LEN * test_level, "",

will that be _simple_ enough ?

Thanks,
Michal

> 
> 
>>                           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
>>
> 
> Otherwise, this all looks good to me. Thanks very much!
> 
> Cheers,
> -- David