Re: [PATCH 1/2] kunit: Add ability to skip entire test suites

[Vaibhav Jain]

Hi David,

Thanks for looking into this patch. My responses inline below:


David Gow <david@xxxxxxxxxxxx> writes:

<snip>

>
> Thanks — this is great! There are a few cases it's not handling
> properly, though, particularly with respect to the debugfs support.
>
> I think we need to:
> - Reset suite->status to KUNIT_SUCCESS in kunit_init_suite(), so that a
> suite which is re-run via debugfs isn't automatically skipped again.
> - Fix the result handling in debugfs to handle skipped suites.
Agree, and I have addressed this in v2 of the patch using the
code-snippets you have shared.

> - (Optional) Maybe we could get rid of kunit_suite::suite_init_err now
> that there's a specific status value. That'd have to be done carefully
> to preserve all of the semantics, though.
Yes fully agree that kunit_suite::suite_init_err will be redundant after
introduction of kunit_suite::status. I have started looking into this
and will post a different patch{series} for this.

>
> Here's a quick (and slightly hacky) patch to fix the first couple of issues:
> ---
> diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c
> index 9c326f1837bd..23d34bfdba95 100644
> --- a/lib/kunit/debugfs.c
> +++ b/lib/kunit/debugfs.c
> @@ -58,6 +58,22 @@ static void debugfs_print_result(struct seq_file
> *seq, struct string_stream *log
>  	spin_unlock(&log->lock);
>  }
>
> +/* Print the result line for a suite. */
> +static void debugfs_print_ok_not_ok(struct seq_file *seq,
> +				    enum kunit_status status,
> +				    size_t test_number,
> +				    const char *description,
> +				    const char *directive)
> +{
> +	const char *directive_header = (status == KUNIT_SKIPPED) ? " # SKIP "
> : "";
> +	const char *directive_body = (status == KUNIT_SKIPPED) ? directive : "";
> +
> +	seq_printf(seq, "%s %zd %s%s%s\n",
> +		   kunit_status_to_ok_not_ok(status),
> +		   test_number, description, directive_header,
> +		   directive_body);
> +}
> +
>  /*
>   * /sys/kernel/debug/kunit/<testsuite>/results shows all results for
> testsuite.
>   */
> @@ -77,17 +93,17 @@ static int debugfs_print_results(struct seq_file
> *seq, void *v)
>  	seq_puts(seq, "1..1\n");
>
>  	/* Print suite header because it is not stored in the test logs. */
> -	seq_puts(seq, KUNIT_SUBTEST_INDENT "KTAP version 1\n");
> -	seq_printf(seq, KUNIT_SUBTEST_INDENT "# Subtest: %s\n", suite->name);
> -	seq_printf(seq, KUNIT_SUBTEST_INDENT "1..%zd\n",
> kunit_suite_num_test_cases(suite));
> -
> -	kunit_suite_for_each_test_case(suite, test_case)
> -		debugfs_print_result(seq, test_case->log);
> +	if (suite->status != KUNIT_SKIPPED) {
> +		seq_puts(seq, KUNIT_SUBTEST_INDENT "KTAP version 1\n");
> +		seq_printf(seq, KUNIT_SUBTEST_INDENT "# Subtest: %s\n", suite->name);
> +		seq_printf(seq, KUNIT_SUBTEST_INDENT "1..%zd\n",
> kunit_suite_num_test_cases(suite));
>
> +		kunit_suite_for_each_test_case(suite, test_case)
> +			debugfs_print_result(seq, test_case->log);
> +	}
>  	debugfs_print_result(seq, suite->log);
>
> -	seq_printf(seq, "%s %d %s\n",
> -		   kunit_status_to_ok_not_ok(success), 1, suite->name);
> +	debugfs_print_ok_not_ok(seq, success, 1, suite->name,
> suite->status_comment);
>  	return 0;
>  }
Thanks, have modified these proposed changes a bit for v2

>
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index c0ae45a22b2c..2ff145796450 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -798,9 +798,6 @@ int kunit_run_tests(struct kunit_suite *suite)
>  	/* Taint the kernel so we know we've run tests. */
>  	add_taint(TAINT_TEST, LOCKDEP_STILL_OK);
>
> -	if (suite->status == KUNIT_SKIPPED)
> -		goto suite_end;
> -
>  	if (suite->suite_init) {
>  		suite->suite_init_err = suite->suite_init(suite);
>  		if (suite->suite_init_err) {
> @@ -836,6 +833,7 @@ static void kunit_init_suite(struct kunit_suite *suite)
>  	kunit_debugfs_create_suite(suite);
>  	suite->status_comment[0] = '\0';
>  	suite->suite_init_err = 0;
> +	suite->status = KUNIT_SUCCESS;
Thanks, taken this change for v2.

>
>  	if (suite->log)
>  		string_stream_clear(suite->log);
> ---
>
>
>>  include/kunit/test.h |  1 +
>>  lib/kunit/test.c     | 11 +++++++++++
>>  2 files changed, 12 insertions(+)
>> 
>> diff --git a/include/kunit/test.h b/include/kunit/test.h
>> index ce0573e196ce..395221d623f7 100644
>> --- a/include/kunit/test.h
>> +++ b/include/kunit/test.h
>> @@ -285,6 +285,7 @@ struct kunit_suite {
>>  	struct string_stream *log;
>>  	int suite_init_err;
>>  	bool is_init;
>> +	enum kunit_status status;
>>  };
>>  
>>  /* Stores an array of suites, end points one past the end */
>> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
>> index 99773e000e1b..989acc770265 100644
>> --- a/lib/kunit/test.c
>> +++ b/lib/kunit/test.c
>> @@ -214,6 +214,9 @@ enum kunit_status kunit_suite_has_succeeded(struct kunit_suite *suite)
>>  	const struct kunit_case *test_case;
>>  	enum kunit_status status = KUNIT_SKIPPED;
>>  
>> +	if (suite->status == KUNIT_SKIPPED)
>> +		return KUNIT_SKIPPED;
>> +
>>  	if (suite->suite_init_err)
>>  		return KUNIT_FAILURE;
>>  
>> @@ -795,12 +798,20 @@ int kunit_run_tests(struct kunit_suite *suite)
>>  	/* Taint the kernel so we know we've run tests. */
>>  	add_taint(TAINT_TEST, LOCKDEP_STILL_OK);
>>  
>> +	if (suite->status == KUNIT_SKIPPED)
>> +		goto suite_end;
>> +
>
> Do we want this? If a test is run more than once, we probably want to
> re-run suite_init so that we can tell if we should still skip it.
This condition should prevent a 'kunit_suite' from running if its marked
skipped before suite_init() is called. 

>
> While I don't think it's likely that a test which was previously skipped
> will suddenly become available, it's not impossible with, e.g., CPU hotplug.
Agree, I have updated kunit_init_suite() to set 'kunit_suite.status' ==
KUNIT_SUCCESS. This should force the suite_init() to be calledback again
when the suite is re-run.


-- 
Cheers
~ Vaibhav