Message-ID: <alpine.LRH.2.21.2005131039280.16613@localhost>
Date:   Wed, 13 May 2020 11:07:44 +0100 (BST)
From:   Alan Maguire <alan.maguire@...cle.com>
To:     David Gow <davidgow@...gle.com>
cc:     Brendan Higgins <brendanhiggins@...gle.com>,
        Alan Maguire <alan.maguire@...cle.com>,
        Marco Elver <elver@...gle.com>,
        linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] kunit: Support skipped tests

On Tue, 12 May 2020, David Gow wrote:

> This is a proof-of-concept to support "skipping" tests.
>

Really glad to see skip support; nice work!
 
> 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
>

My first thought before consulting this was the right answer is
to expand the kunit status from a bool to include skipped as
a possiblility (making it an "enum kunit_status") but the the,
above seems to classify skipping as a directive with a reason,
while the test status is still reported as "ok".

Despite that, I wonder if having test status as an enum still
might make sense?  kunit_status_to_string() would still render
KUNIT_SKIPPED as "ok" in such a case, but having the skipped
state in status might make computing if the plan line should
print "skipped" easier (if all subtests have status == KUNIT_SKIPPED).
Taking that approach might also be more extensible; implementing
TODO and other directives would become much easier.

So instead of having a kunit_skip() macro, we could possibly
rework kunit_print_ok_not_ok() to be something like
kunit_print_status(), with a status enum value and an optional
directive string.  That way, adding new status values would
be simply a case of adding to the enum and specifying 
an associated string value ("ok" for skip, "not ok" for
TODO, etc). Plus a KUNIT_SKIPPED status could easily
progagate up from the tests to the plan result line
(all tests have status == SKIPPED => plan status = SKIPPED).

Would that work? Thanks!

Alan

> 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@...gle.com>
> ---
> 
> 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
> 
> 

Powered by blists - more mailing lists