RE: [RFC PATCH] kunit: Support skipped tests

[Bird, Tim]

> -----Original Message-----
> From: David Gow
> 
> This is a proof-of-concept to support "skipping" tests.
> 
> The kunit_mark_skipped() macro marks the current test as "skipped", with
> the provided reason. The kunit_skip() macro will mark the test as
> skipped, and abort the test.
> 
> The TAP specification supports this "SKIP directive" as a comment after
> the "ok" / "not ok" for a test. See the "Directives" section of the TAP
> spec for details:
> https://testanything.org/tap-specification.html#directives

Just a comment here.

That spec does not specify whether the "# SKIP" directive should come
before or after the test description.  I see by looking at the code below
you have placed the directive AFTER the test description.  That is, IMHO,
good, since that seems to be the standard adopted by other kselftest
programs for their output; and it would be good to have this be consistent
throughout the kernel testing regime.

Just my 2 cents for keeping the element ordering the way you have
it currently in the patch.  Thanks!

Regards,
 -- Tim

> 
> kunit_tool will parse this SKIP directive, and renders skipped tests in
> yellow and counts them. Skipped tests do not affect the result for a
> suite.
> 
> Signed-off-by: David Gow <davidgow@xxxxxxxxxx>
> ---
> 
> Following on from discussions about the KCSAN test[1], which requires a
> multi-core/processor system to make sense, it would be useful for tests
> to be able to mark themselves as "skipped", where tests have runtime
> dependencies which aren't met.
> 
> As a proof-of-concept, this patch doesn't implement some things which
> we'd ideally like to have (e.g., non-static "reasons" for skipping the
> test, maybe some SKIP macros akin to the EXPECT and ASSERT ones), and
> the implementation is still pretty hacky, but I though I'd put this out
> there to see if there are any thoughts on the concept in general.
> 
> Cheers,
> -- David
> 
> [1]: https://lkml.org/lkml/2020/5/5/31
> 
>  include/kunit/test.h                | 12 ++++++++++++
>  lib/kunit/kunit-example-test.c      |  7 +++++++
>  lib/kunit/test.c                    | 23 ++++++++++++++++-------
>  tools/testing/kunit/kunit_parser.py | 21 +++++++++++++++++----
>  4 files changed, 52 insertions(+), 11 deletions(-)
> 
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 9b0c46a6ca1f..7817c5580b2c 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -178,6 +178,7 @@ struct kunit_suite {
>  	/* private - internal use only */
>  	struct dentry *debugfs;
>  	char *log;
> +	const char *skip_directive;
>  };
> 
>  /**
> @@ -213,6 +214,8 @@ struct kunit {
>  	 * protect it with some type of lock.
>  	 */
>  	struct list_head resources; /* Protected by lock. */
> +
> +	const char *skip_directive;
>  };
> 
>  void kunit_init_test(struct kunit *test, const char *name, char *log);
> @@ -391,6 +394,15 @@ void kunit_cleanup(struct kunit *test);
> 
>  void kunit_log_append(char *log, const char *fmt, ...);
> 
> +#define kunit_mark_skipped(test_or_suite, reason)			\
> +	(test_or_suite)->skip_directive = "SKIP " reason
> +
> +#define kunit_skip(test_or_suite, reason)				\
> +	do {								\
> +		kunit_mark_skipped(test_or_suite, reason);		\
> +		kunit_try_catch_throw(&((test_or_suite)->try_catch));	\
> +	} while (0)
> +
>  /*
>   * printk and log to per-test or per-suite log buffer.  Logging only done
>   * if CONFIG_KUNIT_DEBUGFS is 'y'; if it is 'n', no log is allocated/used.
> diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
> index be1164ecc476..998401a61458 100644
> --- a/lib/kunit/kunit-example-test.c
> +++ b/lib/kunit/kunit-example-test.c
> @@ -29,6 +29,12 @@ static void example_simple_test(struct kunit *test)
>  	KUNIT_EXPECT_EQ(test, 1 + 1, 2);
>  }
> 
> +static void example_skip_test(struct kunit *test)
> +{
> +	kunit_skip(test, "this test should be skipped");
> +	KUNIT_EXPECT_EQ(test, 1 + 1, 2);
> +}
> +
>  /*
>   * This is run once before each test case, see the comment on
>   * example_test_suite for more information.
> @@ -52,6 +58,7 @@ static struct kunit_case example_test_cases[] = {
>  	 * test suite.
>  	 */
>  	KUNIT_CASE(example_simple_test),
> +	KUNIT_CASE(example_skip_test),
>  	{}
>  };
> 
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index ccb2ffad8dcf..84b9be3a8da7 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -79,10 +79,12 @@ static void kunit_print_ok_not_ok(void *test_or_suite,
>  				  bool is_test,
>  				  bool is_ok,
>  				  size_t test_number,
> -				  const char *description)
> +				  const char *description,
> +				  const char *directive)
>  {
>  	struct kunit_suite *suite = is_test ? NULL : test_or_suite;
>  	struct kunit *test = is_test ? test_or_suite : NULL;
> +	const char *directive_header = directive ? " # " : "";
> 
>  	/*
>  	 * We do not log the test suite results as doing so would
> @@ -93,13 +95,16 @@ static void kunit_print_ok_not_ok(void *test_or_suite,
>  	 * representation.
>  	 */
>  	if (suite)
> -		pr_info("%s %zd - %s\n",
> +		pr_info("%s %zd - %s%s%s\n",
>  			kunit_status_to_string(is_ok),
> -			test_number, description);
> +			test_number, description,
> +			directive_header, directive ? directive : "");
>  	else
> -		kunit_log(KERN_INFO, test, KUNIT_SUBTEST_INDENT "%s %zd - %s",
> +		kunit_log(KERN_INFO, test,
> +			  KUNIT_SUBTEST_INDENT "%s %zd - %s%s%s",
>  			  kunit_status_to_string(is_ok),
> -			  test_number, description);
> +			  test_number, description,
> +			  directive_header, directive ? directive : "");
>  }
> 
>  bool kunit_suite_has_succeeded(struct kunit_suite *suite)
> @@ -122,7 +127,8 @@ static void kunit_print_subtest_end(struct kunit_suite *suite)
>  	kunit_print_ok_not_ok((void *)suite, false,
>  			      kunit_suite_has_succeeded(suite),
>  			      kunit_suite_counter++,
> -			      suite->name);
> +			      suite->name,
> +			      suite->skip_directive);
>  }
> 
>  unsigned int kunit_test_case_num(struct kunit_suite *suite,
> @@ -232,6 +238,7 @@ void kunit_init_test(struct kunit *test, const char *name, char *log)
>  	if (test->log)
>  		test->log[0] = '\0';
>  	test->success = true;
> +	test->skip_directive = NULL;
>  }
>  EXPORT_SYMBOL_GPL(kunit_init_test);
> 
> @@ -357,7 +364,8 @@ static void kunit_run_case_catch_errors(struct kunit_suite *suite,
> 
>  	kunit_print_ok_not_ok(&test, true, test_case->success,
>  			      kunit_test_case_num(suite, test_case),
> -			      test_case->name);
> +			      test_case->name,
> +			      test.skip_directive);
>  }
> 
>  int kunit_run_tests(struct kunit_suite *suite)
> @@ -378,6 +386,7 @@ EXPORT_SYMBOL_GPL(kunit_run_tests);
>  static void kunit_init_suite(struct kunit_suite *suite)
>  {
>  	kunit_debugfs_create_suite(suite);
> +	suite->skip_directive = NULL;
>  }
> 
>  int __kunit_test_suites_init(struct kunit_suite **suites)
> diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
> index 64aac9dcd431..ecfc8ee1da2f 100644
> --- a/tools/testing/kunit/kunit_parser.py
> +++ b/tools/testing/kunit/kunit_parser.py
> @@ -43,6 +43,7 @@ class TestCase(object):
>  class TestStatus(Enum):
>  	SUCCESS = auto()
>  	FAILURE = auto()
> +	SKIPPED = auto()
>  	TEST_CRASHED = auto()
>  	NO_TESTS = auto()
> 
> @@ -107,6 +108,8 @@ def save_non_diagnositic(lines: List[str], test_case: TestCase) -> None:
> 
>  OkNotOkResult = namedtuple('OkNotOkResult', ['is_ok','description', 'text'])
> 
> +OK_NOT_OK_SKIP = re.compile(r'^[\s]*(ok|not ok) [0-9]+ - (.*) # SKIP (.*)$')
> +
>  OK_NOT_OK_SUBTEST = re.compile(r'^[\s]+(ok|not ok) [0-9]+ - (.*)$')
> 
>  OK_NOT_OK_MODULE = re.compile(r'^(ok|not ok) [0-9]+ - (.*)$')
> @@ -124,6 +127,10 @@ def parse_ok_not_ok_test_case(lines: List[str], test_case: TestCase) -> bool:
>  	if match:
>  		test_case.log.append(lines.pop(0))
>  		test_case.name = match.group(2)
> +		skip_match = OK_NOT_OK_SKIP.match(line)
> +		if skip_match:
> +			test_case.status = TestStatus.SKIPPED
> +			return True
>  		if test_case.status == TestStatus.TEST_CRASHED:
>  			return True
>  		if match.group(1) == 'ok':
> @@ -190,9 +197,9 @@ def max_status(left: TestStatus, right: TestStatus) -> TestStatus:
>  		return TestStatus.TEST_CRASHED
>  	elif left == TestStatus.FAILURE or right == TestStatus.FAILURE:
>  		return TestStatus.FAILURE
> -	elif left != TestStatus.SUCCESS:
> +	elif left != TestStatus.SUCCESS and left != TestStatus.SKIPPED:
>  		return left
> -	elif right != TestStatus.SUCCESS:
> +	elif right != TestStatus.SUCCESS and right != TestStatus.SKIPPED:
>  		return right
>  	else:
>  		return TestStatus.SUCCESS
> @@ -281,10 +288,13 @@ def parse_run_tests(kernel_output) -> TestResult:
>  	total_tests = 0
>  	failed_tests = 0
>  	crashed_tests = 0
> +	skipped_tests = 0
>  	test_result = parse_test_result(list(isolate_kunit_output(kernel_output)))
>  	for test_suite in test_result.suites:
>  		if test_suite.status == TestStatus.SUCCESS:
>  			print_suite_divider(green('[PASSED] ') + test_suite.name)
> +		elif test_suite.status == TestStatus.SKIPPED:
> +			print_suite_divider(yellow('[SKIPPED] ') + test_suite.name)
>  		elif test_suite.status == TestStatus.TEST_CRASHED:
>  			print_suite_divider(red('[CRASHED] ' + test_suite.name))
>  		else:
> @@ -293,6 +303,9 @@ def parse_run_tests(kernel_output) -> TestResult:
>  			total_tests += 1
>  			if test_case.status == TestStatus.SUCCESS:
>  				print_with_timestamp(green('[PASSED] ') + test_case.name)
> +			elif test_case.status == TestStatus.SKIPPED:
> +				skipped_tests += 1
> +				print_with_timestamp(yellow('[SKIPPED] ') + test_case.name)
>  			elif test_case.status == TestStatus.TEST_CRASHED:
>  				crashed_tests += 1
>  				print_with_timestamp(red('[CRASHED] ' + test_case.name))
> @@ -306,6 +319,6 @@ def parse_run_tests(kernel_output) -> TestResult:
>  	print_with_timestamp(DIVIDER)
>  	fmt = green if test_result.status == TestStatus.SUCCESS else red
>  	print_with_timestamp(
> -		fmt('Testing complete. %d tests run. %d failed. %d crashed.' %
> -		    (total_tests, failed_tests, crashed_tests)))
> +		fmt('Testing complete. %d tests run. %d failed. %d crashed. %d skipped.' %
> +		    (total_tests, failed_tests, crashed_tests, skipped_tests)))
>  	return test_result
> --
> 2.26.2.645.ge9eca65c58-goog