Re: [PATCH v2 3/4] kunit: Don't crash if no parameters are generated

Daniel Latypov
2021-10-27T19:00:13Z

On Tue, Oct 26, 2021 at 6:37 PM David Gow <davidgow@google.com> wrote:
>
> It's possible that a parameterised test could end up with zero
> parameters. At the moment, the test function will nevertheless be called
> with NULL as the parameter. Instead, don't try to run the test code, and
> just mark the test as SKIPped.
>
> Reported-by: Daniel Latypov <dlatypov@google.com>
> Signed-off-by: David Gow <davidgow@google.com>
> ---
> lib/kunit/test.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 3bd741e50a2d..e028d98e4f5b 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -500,7 +500,10 @@ int kunit_run_tests(struct kunit_suite *suite)
> kunit_print_subtest_start(suite);
>
> kunit_suite_for_each_test_case(suite, test_case) {
> - struct kunit test = { .param_value = NULL, .param_index = 0 };
> + /* The initial param value is nonzero, as we want
> + * non-parametrised tests to run once.
> + */
> + struct kunit test = { .param_value = (void *)-1, .param_index = 0 };

(Not a strong preference)

Hmm, I'd slightly prefer we don't set a dummy value of -1 for this.
I personally think something like this is a bit less subtle:

/* Run non-parameterised tests once */
while (!test_case->generate_param || test.param_value) {

if (!test_case->generate_param) break;
}

Alternatively, we don't need to share the loop

if (!test_case->generate_param) {
kunit_run_case_catch_errors(suite, test_case, &test);
kunit_update_stats(&param_stats, test.status);
} else while (test_param.value) {
kunit_run_case_catch_errors(suite, test_case, &test);
kunit_update_stats(&param_stats, test.status);
/* print subtest and advance next param */
}

}

> struct kunit_result_stats param_stats = { 0 };
> test_case->status = KUNIT_SKIPPED;
>
> @@ -510,7 +513,7 @@ int kunit_run_tests(struct kunit_suite *suite)
> test.param_value = test_case->generate_params(NULL, param_desc);
> }
>
> - do {
> + while (test.param_value) {
> kunit_run_case_catch_errors(suite, test_case, &test);
>
> if (test_case->generate_params) {
> @@ -530,11 +533,12 @@ int kunit_run_tests(struct kunit_suite *suite)
> param_desc[0] = '\0';
> test.param_value = test_case->generate_params(test.param_value, param_desc);
> test.param_index++;
> - }
> + } else
> + test.param_value = NULL;
>
> kunit_update_stats(&param_stats, test.status);
>
> - } while (test.param_value);
> + }
>
> kunit_print_test_stats(&test, param_stats);
>
> --
> 2.33.0.1079.g6e70778dc9-goog
>