[PATCH] kunit: Fix result propagation for parameterised tests

[PATCH] kunit: Fix result propagation for parameterised tests

[David Gow]

When one parameter of a parameterised test failed, its failure would be
propagated to the overall test, but not to the suite result (unless it
was the last parameter).

This is because test_case->success was being reset to the test->success
result after each parameter was used, so a failing test's result would
be overwritten by a non-failing result. The overall test result was
handled in a third variable, test_result, but this was disacarded after
the status line was printed.

Instead, just propagate the result after each parameter run.

Signed-off-by: David Gow <davidgow@google.com>
Fixes: fadb08e7c750 ("kunit: Support for Parameterized Testing")
---

This is fixing quite a serious bug where some test suites would appear
to succeed even if some of their component tests failed. It'd be nice to
get this into kunit-fixes ASAP.

(This will require a rework of some of the skip tests work, for which
I'll send out a new version soon.)

Cheers,
-- David

 lib/kunit/test.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 2f6cc0123232..17973a4a44c2 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -376,7 +376,7 @@ static void kunit_run_case_catch_errors(struct kunit_suite *suite,
 	context.test_case = test_case;
 	kunit_try_catch_run(try_catch, &context);
 
-	test_case->success = test->success;
+	test_case->success &= test->success;
 }
 
 int kunit_run_tests(struct kunit_suite *suite)
@@ -388,7 +388,7 @@ int kunit_run_tests(struct kunit_suite *suite)
 
 	kunit_suite_for_each_test_case(suite, test_case) {
 		struct kunit test = { .param_value = NULL, .param_index = 0 };
-		bool test_success = true;
+		test_case->success = true;
 
 		if (test_case->generate_params) {
 			/* Get initial param. */
@@ -398,7 +398,6 @@ int kunit_run_tests(struct kunit_suite *suite)
 
 		do {
 			kunit_run_case_catch_errors(suite, test_case, &test);
-			test_success &= test_case->success;
 
 			if (test_case->generate_params) {
 				if (param_desc[0] == '\0') {
@@ -420,7 +419,7 @@ int kunit_run_tests(struct kunit_suite *suite)
 			}
 		} while (test.param_value);
 
-		kunit_print_ok_not_ok(&test, true, test_success,
+		kunit_print_ok_not_ok(&test, true, test_case->success,
 				      kunit_test_case_num(suite, test_case),
 				      test_case->name);
 	}
-- 
2.32.0.272.g935e593368-goog

Re: [PATCH] kunit: Fix result propagation for parameterised tests

[Marco Elver]

On Fri, 11 Jun 2021 at 05:57, David Gow <davidgow@google.com> wrote:
>
> When one parameter of a parameterised test failed, its failure would be
> propagated to the overall test, but not to the suite result (unless it
> was the last parameter).
>
> This is because test_case->success was being reset to the test->success
> result after each parameter was used, so a failing test's result would
> be overwritten by a non-failing result. The overall test result was
> handled in a third variable, test_result, but this was disacarded after
> the status line was printed.
>
> Instead, just propagate the result after each parameter run.
>
> Signed-off-by: David Gow <davidgow@google.com>
> Fixes: fadb08e7c750 ("kunit: Support for Parameterized Testing")

Reviewed-by: Marco Elver <elver@google.com>

Would Cc: stable be appropriate?

Thanks,
-- Marco

> ---
>
> This is fixing quite a serious bug where some test suites would appear
> to succeed even if some of their component tests failed. It'd be nice to
> get this into kunit-fixes ASAP.
>
> (This will require a rework of some of the skip tests work, for which
> I'll send out a new version soon.)
>
> Cheers,
> -- David
>
>  lib/kunit/test.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 2f6cc0123232..17973a4a44c2 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -376,7 +376,7 @@ static void kunit_run_case_catch_errors(struct kunit_suite *suite,
>         context.test_case = test_case;
>         kunit_try_catch_run(try_catch, &context);
>
> -       test_case->success = test->success;
> +       test_case->success &= test->success;
>  }
>
>  int kunit_run_tests(struct kunit_suite *suite)
> @@ -388,7 +388,7 @@ int kunit_run_tests(struct kunit_suite *suite)
>
>         kunit_suite_for_each_test_case(suite, test_case) {
>                 struct kunit test = { .param_value = NULL, .param_index = 0 };
> -               bool test_success = true;
> +               test_case->success = true;
>
>                 if (test_case->generate_params) {
>                         /* Get initial param. */
> @@ -398,7 +398,6 @@ int kunit_run_tests(struct kunit_suite *suite)
>
>                 do {
>                         kunit_run_case_catch_errors(suite, test_case, &test);
> -                       test_success &= test_case->success;
>
>                         if (test_case->generate_params) {
>                                 if (param_desc[0] == '\0') {
> @@ -420,7 +419,7 @@ int kunit_run_tests(struct kunit_suite *suite)
>                         }
>                 } while (test.param_value);
>
> -               kunit_print_ok_not_ok(&test, true, test_success,
> +               kunit_print_ok_not_ok(&test, true, test_case->success,
>                                       kunit_test_case_num(suite, test_case),
>                                       test_case->name);
>         }
> --
> 2.32.0.272.g935e593368-goog
>

Re: [PATCH] kunit: Fix result propagation for parameterised tests

[Shuah Khan]

On 6/11/21 2:29 AM, Marco Elver wrote:
> On Fri, 11 Jun 2021 at 05:57, David Gow <davidgow@google.com> wrote:
>>
>> When one parameter of a parameterised test failed, its failure would be
>> propagated to the overall test, but not to the suite result (unless it
>> was the last parameter).
>>
>> This is because test_case->success was being reset to the test->success
>> result after each parameter was used, so a failing test's result would
>> be overwritten by a non-failing result. The overall test result was
>> handled in a third variable, test_result, but this was disacarded after
>> the status line was printed.
>>
>> Instead, just propagate the result after each parameter run.
>>
>> Signed-off-by: David Gow <davidgow@google.com>
>> Fixes: fadb08e7c750 ("kunit: Support for Parameterized Testing")
> 
> Reviewed-by: Marco Elver <elver@google.com>
> 
> Would Cc: stable be appropriate?
> 
> Thanks,
> -- Marco
> 
>> ---
>>
>> This is fixing quite a serious bug where some test suites would appear
>> to succeed even if some of their component tests failed. It'd be nice to
>> get this into kunit-fixes ASAP.
>>

Will apply this with cc stable.

>> (This will require a rework of some of the skip tests work, for which
>> I'll send out a new version soon.)
>>

Thanks for the heads up. I will wait for new version.

thanks,
-- Shuah

Re: [PATCH] kunit: Fix result propagation for parameterised tests

[Brendan Higgins]

On Thu, Jun 10, 2021 at 8:57 PM David Gow <davidgow@google.com> wrote:
>
> When one parameter of a parameterised test failed, its failure would be
> propagated to the overall test, but not to the suite result (unless it
> was the last parameter).
>
> This is because test_case->success was being reset to the test->success
> result after each parameter was used, so a failing test's result would
> be overwritten by a non-failing result. The overall test result was
> handled in a third variable, test_result, but this was disacarded after
> the status line was printed.

nit: s/disacarded/discarded/g

> Instead, just propagate the result after each parameter run.
>
> Signed-off-by: David Gow <davidgow@google.com>
> Fixes: fadb08e7c750 ("kunit: Support for Parameterized Testing")

I tried to reproduce the problem described and was unable to. Anyway,
from the code it definitely looks like there is a bug like you
describe. And it definitely looks like your change should fix it.

Anyway, I tried testing your fix, but given I was unable to reproduce
the failure, I am not super confident in my testing. Still,

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

Re: [PATCH] kunit: Fix result propagation for parameterised tests

[David Gow]

On Sat, Jun 12, 2021 at 4:26 AM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> On Thu, Jun 10, 2021 at 8:57 PM David Gow <davidgow@google.com> wrote:
> >
> > When one parameter of a parameterised test failed, its failure would be
> > propagated to the overall test, but not to the suite result (unless it
> > was the last parameter).
> >
> > This is because test_case->success was being reset to the test->success
> > result after each parameter was used, so a failing test's result would
> > be overwritten by a non-failing result. The overall test result was
> > handled in a third variable, test_result, but this was disacarded after
> > the status line was printed.
>
> nit: s/disacarded/discarded/g
>
> > Instead, just propagate the result after each parameter run.
> >
> > Signed-off-by: David Gow <davidgow@google.com>
> > Fixes: fadb08e7c750 ("kunit: Support for Parameterized Testing")
>
> I tried to reproduce the problem described and was unable to. Anyway,
> from the code it definitely looks like there is a bug like you
> describe. And it definitely looks like your change should fix it.

I was able to reproduce this again myself. Note that the kunit_tool
wrapper does its own result propagation which doesn't have a similar
bug, so you won't see the issue in its parsed results. (Using the
--raw_output flag does show it, though).

Here's the output from a patched UUID suite, which deliberately fails
the first parameter of the first two tests and passes the other
parameters and tests, which exhibits the issue:

TAP version 14
1..1
   # Subtest: uuid
   1..4
   # uuid_correct_be: ASSERTION FAILED at lib/test_uuid.c:57
   Expected uuid_parse(data->uuid, &be) == 0, but
       uuid_parse(data->uuid, &be) == -22

failed to parse 'c33fx4995-3701-450e-9fbf-206a2e98e576'
   # uuid_correct_be: not ok 1 - c33fx4995-3701-450e-9fbf-206a2e98e576
   # uuid_correct_be: ok 2 - 64b4371c-77c1-48f9-8221-29f054fc023b
   # uuid_correct_be: ok 3 - 0cb4ddff-a545-4401-9d06-688af53e7f84
   not ok 1 - uuid_correct_be
   # uuid_correct_le: ASSERTION FAILED at lib/test_uuid.c:46
   Expected guid_parse(data->uuid, &le) == 0, but
       guid_parse(data->uuid, &le) == -22

failed to parse 'c33fx4995-3701-450e-9fbf-206a2e98e576'
   # uuid_correct_le: not ok 1 - c33fx4995-3701-450e-9fbf-206a2e98e576
   # uuid_correct_le: ok 2 - 64b4371c-77c1-48f9-8221-29f054fc023b
   # uuid_correct_le: ok 3 - 0cb4ddff-a545-4401-9d06-688af53e7f84
   not ok 2 - uuid_correct_le
   # uuid_wrong_be: ok 1 - c33f4995-3701-450e-9fbf206a2e98e576
   # uuid_wrong_be: ok 2 - 64b4371c-77c1-48f9-8221-29f054XX023b
   # uuid_wrong_be: ok 3 - 0cb4ddff-a545-4401-9d06-688af53e
   ok 3 - uuid_wrong_be
   # uuid_wrong_le: ok 1 - c33f4995-3701-450e-9fbf206a2e98e576
   # uuid_wrong_le: ok 2 - 64b4371c-77c1-48f9-8221-29f054XX023b
   # uuid_wrong_le: ok 3 - 0cb4ddff-a545-4401-9d06-688af53e
   ok 4 - uuid_wrong_le
ok 1 - uuid


Note the "not ok 1 - uuid_correct_be" line, yet it ending in "ok 1 - uuid".

>
> Anyway, I tried testing your fix, but given I was unable to reproduce
> the failure, I am not super confident in my testing. Still,
>
> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

Re: [PATCH] kunit: Fix result propagation for parameterised tests

[David Gow]

On Sat, Jun 12, 2021 at 1:44 AM Shuah Khan <skhan@linuxfoundation.org> wrote:
>
> On 6/11/21 2:29 AM, Marco Elver wrote:
> > On Fri, 11 Jun 2021 at 05:57, David Gow <davidgow@google.com> wrote:
> >>
> >> When one parameter of a parameterised test failed, its failure would be
> >> propagated to the overall test, but not to the suite result (unless it
> >> was the last parameter).
> >>
> >> This is because test_case->success was being reset to the test->success
> >> result after each parameter was used, so a failing test's result would
> >> be overwritten by a non-failing result. The overall test result was
> >> handled in a third variable, test_result, but this was disacarded after
> >> the status line was printed.
> >>
> >> Instead, just propagate the result after each parameter run.
> >>
> >> Signed-off-by: David Gow <davidgow@google.com>
> >> Fixes: fadb08e7c750 ("kunit: Support for Parameterized Testing")
> >
> > Reviewed-by: Marco Elver <elver@google.com>
> >
> > Would Cc: stable be appropriate?
> >
> > Thanks,
> > -- Marco
> >
> >> ---
> >>
> >> This is fixing quite a serious bug where some test suites would appear
> >> to succeed even if some of their component tests failed. It'd be nice to
> >> get this into kunit-fixes ASAP.
> >>
>
> Will apply this with cc stable.
>

Thanks!

> >> (This will require a rework of some of the skip tests work, for which
> >> I'll send out a new version soon.)
> >>
>
> Thanks for the heads up. I will wait for new version.
>

Thanks: I've sent out v4 which fixes this:
https://lore.kernel.org/linux-kselftest/20210611070802.1318911-1-davidgow@google.com/

It's rebased on top of this patch, so depends on it, and also depends
on the first two patches in the "Do not typecheck binary assertions"
series:
https://lore.kernel.org/linux-kselftest/20210513193204.816681-1-davidgow@google.com/

> thanks,
> -- Shuah