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 <[email protected]>
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
On Fri, 11 Jun 2021 at 05:57, David Gow <[email protected]> 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 <[email protected]>
> Fixes: fadb08e7c750 ("kunit: Support for Parameterized Testing")
Reviewed-by: Marco Elver <[email protected]>
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
>
On 6/11/21 2:29 AM, Marco Elver wrote:
> On Fri, 11 Jun 2021 at 05:57, David Gow <[email protected]> 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 <[email protected]>
>> Fixes: fadb08e7c750 ("kunit: Support for Parameterized Testing")
>
> Reviewed-by: Marco Elver <[email protected]>
>
> 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
On Thu, Jun 10, 2021 at 8:57 PM David Gow <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
On Sat, Jun 12, 2021 at 4:26 AM Brendan Higgins
<[email protected]> wrote:
>
> On Thu, Jun 10, 2021 at 8:57 PM David Gow <[email protected]> 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 <[email protected]>
> > 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 <[email protected]>
On Sat, Jun 12, 2021 at 1:44 AM Shuah Khan <[email protected]> wrote:
>
> On 6/11/21 2:29 AM, Marco Elver wrote:
> > On Fri, 11 Jun 2021 at 05:57, David Gow <[email protected]> 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 <[email protected]>
> >> Fixes: fadb08e7c750 ("kunit: Support for Parameterized Testing")
> >
> > Reviewed-by: Marco Elver <[email protected]>
> >
> > 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/[email protected]/
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/[email protected]/
> thanks,
> -- Shuah