[PATCH 1/3] kunit: rename print_subtest_{start,end} for clarity (s/subtest/suite)

[PATCH 1/3] kunit: rename print_subtest_{start,end} for clarity (s/subtest/suite)

[Daniel Latypov]

These names sound more general than they are.

The _end() function increments a `static int kunit_suite_counter`, so it
can only safely be called on suites, aka top-level subtests.
It would need to have a separate counter for each level of subtest to be
generic enough.

So rename it to make it clear it's only appropriate for suites.

Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
 lib/kunit/test.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 0f66c13d126e..64ee6a9d8003 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -134,7 +134,7 @@ size_t kunit_suite_num_test_cases(struct kunit_suite *suite)
 }
 EXPORT_SYMBOL_GPL(kunit_suite_num_test_cases);
 
-static void kunit_print_subtest_start(struct kunit_suite *suite)
+static void kunit_print_suite_start(struct kunit_suite *suite)
 {
 	kunit_log(KERN_INFO, suite, KUNIT_SUBTEST_INDENT "# Subtest: %s",
 		  suite->name);
@@ -192,7 +192,7 @@ EXPORT_SYMBOL_GPL(kunit_suite_has_succeeded);
 
 static size_t kunit_suite_counter = 1;
 
-static void kunit_print_subtest_end(struct kunit_suite *suite)
+static void kunit_print_suite_end(struct kunit_suite *suite)
 {
 	kunit_print_ok_not_ok((void *)suite, false,
 			      kunit_suite_has_succeeded(suite),
@@ -498,7 +498,7 @@ int kunit_run_tests(struct kunit_suite *suite)
 	struct kunit_result_stats suite_stats = { 0 };
 	struct kunit_result_stats total_stats = { 0 };
 
-	kunit_print_subtest_start(suite);
+	kunit_print_suite_start(suite);
 
 	kunit_suite_for_each_test_case(suite, test_case) {
 		struct kunit test = { .param_value = NULL, .param_index = 0 };
@@ -552,7 +552,7 @@ int kunit_run_tests(struct kunit_suite *suite)
 	}
 
 	kunit_print_suite_stats(suite, suite_stats, total_stats);
-	kunit_print_subtest_end(suite);
+	kunit_print_suite_end(suite);
 
 	return 0;
 }

base-commit: 59729170afcd4900e08997a482467ffda8d88c7f
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[PATCH 2/3] kunit: add ability to specify suite-level init and exit functions

[Daniel Latypov]

KUnit has support for setup/cleanup logic for each test case in a suite.
But it lacks the ability to specify setup/cleanup for the entire suite
itself.

This can be used to do setup that is too expensive or cumbersome to do
for each test.
Or it can be used to do simpler things like log debug information after
the suite completes.
It's a fairly common feature, so the lack of it is noticeable.

Some examples in other frameworks and languages:
* https://docs.python.org/3/library/unittest.html#setupclass-and-teardownclass
* https://google.github.io/googletest/reference/testing.html#Test::SetUpTestSuite

Meta:
This is very similar to this patch here: https://lore.kernel.org/linux-kselftest/20210805043503.20252-3-bvanassche@acm.org/
The changes from that patch:
* pass in `struct kunit *` so users can do stuff like
  `kunit_info(suite, "debug message")`
* makes sure the init failure is bubbled up as a failure
* updates kunit-example-test.c to use a suite init
* Updates kunit/usage.rst to mention the new support
* some minor cosmetic things
  * use `suite_{init,exit}` instead of `{init/exit}_suite`
  * make suite init error message more consistent w/ test init
  * etc.

Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
 Documentation/dev-tools/kunit/usage.rst | 19 +++++++++++--------
 include/kunit/test.h                    |  4 ++++
 lib/kunit/kunit-example-test.c          | 14 ++++++++++++++
 lib/kunit/test.c                        | 23 ++++++++++++++++++++---
 4 files changed, 49 insertions(+), 11 deletions(-)

diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
index 1c83e7d60a8a..d62a04255c2e 100644
--- a/Documentation/dev-tools/kunit/usage.rst
+++ b/Documentation/dev-tools/kunit/usage.rst
@@ -125,8 +125,8 @@ We need many test cases covering all the unit's behaviors. It is common to have
 many similar tests. In order to reduce duplication in these closely related
 tests, most unit testing frameworks (including KUnit) provide the concept of a
 *test suite*. A test suite is a collection of test cases for a unit of code
-with a setup function that gets invoked before every test case and then a tear
-down function that gets invoked after every test case completes. For example:
+with optional setup and teardown functions that run before/after the whole
+suite and/or every test case. For example:
 
 .. code-block:: c
 
@@ -141,16 +141,19 @@ down function that gets invoked after every test case completes. For example:
 		.name = "example",
 		.init = example_test_init,
 		.exit = example_test_exit,
+		.suite_init = example_suite_init,
+		.suite_exit = example_suite_exit,
 		.test_cases = example_test_cases,
 	};
 	kunit_test_suite(example_test_suite);
 
-In the above example, the test suite ``example_test_suite`` would run the test
-cases ``example_test_foo``, ``example_test_bar``, and ``example_test_baz``. Each
-would have ``example_test_init`` called immediately before it and
-``example_test_exit`` called immediately after it.
-``kunit_test_suite(example_test_suite)`` registers the test suite with the
-KUnit test framework.
+In the above example, the test suite ``example_test_suite`` would first run
+``example_suite_init``, then run the test cases ``example_test_foo``,
+``example_test_bar``, and ``example_test_baz``. Each would have
+``example_test_init`` called immediately before it and ``example_test_exit``
+called immediately after it. Finally, ``example_suite_exit`` would be called
+after everything else. ``kunit_test_suite(example_test_suite)`` registers the
+test suite with the KUnit test framework.
 
 .. note::
    A test case will only run if it is associated with a test suite.
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 97cd76461886..5d288f3d8f68 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -153,6 +153,8 @@ static inline char *kunit_status_to_ok_not_ok(enum kunit_status status)
  * struct kunit_suite - describes a related collection of &struct kunit_case
  *
  * @name:	the name of the test. Purely informational.
+ * @suite_init:	called once per test suite before the test cases.
+ * @suite_exit:	called once per test suite after all test cases.
  * @init:	called before every test case.
  * @exit:	called after every test case.
  * @test_cases:	a null terminated array of test cases.
@@ -167,6 +169,8 @@ static inline char *kunit_status_to_ok_not_ok(enum kunit_status status)
  */
 struct kunit_suite {
 	const char name[256];
+	int (*suite_init)(struct kunit_suite *suite);
+	void (*suite_exit)(struct kunit_suite *suite);
 	int (*init)(struct kunit *test);
 	void (*exit)(struct kunit *test);
 	struct kunit_case *test_cases;
diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
index 91b1df7f59ed..f8fe582c9e36 100644
--- a/lib/kunit/kunit-example-test.c
+++ b/lib/kunit/kunit-example-test.c
@@ -40,6 +40,17 @@ static int example_test_init(struct kunit *test)
 	return 0;
 }
 
+/*
+ * This is run once before all test cases in the suite.
+ * See the comment on example_test_suite for more information.
+ */
+static int example_test_init_suite(struct kunit_suite *suite)
+{
+	kunit_info(suite, "initializing suite\n");
+
+	return 0;
+}
+
 /*
  * This test should always be skipped.
  */
@@ -142,17 +153,20 @@ static struct kunit_case example_test_cases[] = {
  * may be specified which runs after every test case and can be used to for
  * cleanup. For clarity, running tests in a test suite would behave as follows:
  *
+ * suite.suite_init(suite);
  * suite.init(test);
  * suite.test_case[0](test);
  * suite.exit(test);
  * suite.init(test);
  * suite.test_case[1](test);
  * suite.exit(test);
+ * suite.suite_exit(suite);
  * ...;
  */
 static struct kunit_suite example_test_suite = {
 	.name = "example",
 	.init = example_test_init,
+	.suite_init = example_test_init_suite,
 	.test_cases = example_test_cases,
 };
 
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 64ee6a9d8003..b66e395c795a 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -192,10 +192,13 @@ EXPORT_SYMBOL_GPL(kunit_suite_has_succeeded);
 
 static size_t kunit_suite_counter = 1;
 
-static void kunit_print_suite_end(struct kunit_suite *suite)
+static void kunit_print_suite_end(struct kunit_suite *suite, int init_err)
 {
+	enum kunit_status status =
+		init_err ? KUNIT_FAILURE : kunit_suite_has_succeeded(suite);
+
 	kunit_print_ok_not_ok((void *)suite, false,
-			      kunit_suite_has_succeeded(suite),
+			      status,
 			      kunit_suite_counter++,
 			      suite->name,
 			      suite->status_comment);
@@ -497,6 +500,16 @@ int kunit_run_tests(struct kunit_suite *suite)
 	struct kunit_case *test_case;
 	struct kunit_result_stats suite_stats = { 0 };
 	struct kunit_result_stats total_stats = { 0 };
+	int suite_init_err = 0;
+
+	if (suite->suite_init) {
+		suite_init_err = suite->suite_init(suite);
+		if (suite_init_err != 0) {
+			kunit_err(suite, KUNIT_SUBTEST_INDENT
+				  "# failed to initialize (%d)", suite_init_err);
+			goto suite_end;
+		}
+	}
 
 	kunit_print_suite_start(suite);
 
@@ -551,8 +564,12 @@ int kunit_run_tests(struct kunit_suite *suite)
 		kunit_accumulate_stats(&total_stats, param_stats);
 	}
 
+	if (suite->suite_exit)
+		suite->suite_exit(suite);
+
 	kunit_print_suite_stats(suite, suite_stats, total_stats);
-	kunit_print_suite_end(suite);
+suite_end:
+	kunit_print_suite_end(suite, suite_init_err);
 
 	return 0;
 }
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

[PATCH 3/3] kfence: test: use new suite_{init/exit} support, add .kunitconfig

[Daniel Latypov]

Currently, the kfence test suite could not run via "normal" means since
KUnit didn't support per-suite setup/teardown. So it manually called
internal kunit functions to run itself.
This has some downsides, like missing TAP headers => can't use kunit.py
to run or even parse the test results (w/o tweaks).

Use the newly added support and convert it over, adding a .kunitconfig
so it's even easier to run from kunit.py.

People can now run the test via
$ ./tools/testing/kunit/kunit.py run --kunitconfig=mm/kfence --arch=x86_64
...
[11:02:32] Testing complete. Passed: 23, Failed: 0, Crashed: 0, Skipped: 2, Errors: 0
[11:02:32] Elapsed time: 43.562s total, 0.003s configuring, 9.268s building, 34.281s running

Cc: kasan-dev@googlegroups.com
Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
 mm/kfence/.kunitconfig  |  6 ++++++
 mm/kfence/kfence_test.c | 31 +++++++++++++------------------
 2 files changed, 19 insertions(+), 18 deletions(-)
 create mode 100644 mm/kfence/.kunitconfig

diff --git a/mm/kfence/.kunitconfig b/mm/kfence/.kunitconfig
new file mode 100644
index 000000000000..f3d65e939bfa
--- /dev/null
+++ b/mm/kfence/.kunitconfig
@@ -0,0 +1,6 @@
+CONFIG_KUNIT=y
+CONFIG_KFENCE=y
+CONFIG_KFENCE_KUNIT_TEST=y
+
+# Additional dependencies.
+CONFIG_FTRACE=y
diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c
index 1b50f70a4c0f..96206a4ee9ab 100644
--- a/mm/kfence/kfence_test.c
+++ b/mm/kfence/kfence_test.c
@@ -826,14 +826,6 @@ static void test_exit(struct kunit *test)
 	test_cache_destroy();
 }
 
-static struct kunit_suite kfence_test_suite = {
-	.name = "kfence",
-	.test_cases = kfence_test_cases,
-	.init = test_init,
-	.exit = test_exit,
-};
-static struct kunit_suite *kfence_test_suites[] = { &kfence_test_suite, NULL };
-
 static void register_tracepoints(struct tracepoint *tp, void *ignore)
 {
 	check_trace_callback_type_console(probe_console);
@@ -847,11 +839,7 @@ static void unregister_tracepoints(struct tracepoint *tp, void *ignore)
 		tracepoint_probe_unregister(tp, probe_console, NULL);
 }
 
-/*
- * We only want to do tracepoints setup and teardown once, therefore we have to
- * customize the init and exit functions and cannot rely on kunit_test_suite().
- */
-static int __init kfence_test_init(void)
+static int kfence_suite_init(struct kunit_suite *suite)
 {
 	/*
 	 * Because we want to be able to build the test as a module, we need to
@@ -859,18 +847,25 @@ static int __init kfence_test_init(void)
 	 * won't work here.
 	 */
 	for_each_kernel_tracepoint(register_tracepoints, NULL);
-	return __kunit_test_suites_init(kfence_test_suites);
+	return 0;
 }
 
-static void kfence_test_exit(void)
+static void kfence_suite_exit(struct kunit_suite *suite)
 {
-	__kunit_test_suites_exit(kfence_test_suites);
 	for_each_kernel_tracepoint(unregister_tracepoints, NULL);
 	tracepoint_synchronize_unregister();
 }
 
-late_initcall_sync(kfence_test_init);
-module_exit(kfence_test_exit);
+static struct kunit_suite kfence_test_suite = {
+	.name = "kfence",
+	.test_cases = kfence_test_cases,
+	.init = test_init,
+	.exit = test_exit,
+	.suite_init = kfence_suite_init,
+	.suite_exit = kfence_suite_exit,
+};
+
+kunit_test_suites(&kfence_test_suite);
 
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Alexander Potapenko <glider@google.com>, Marco Elver <elver@google.com>");
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

Re: [PATCH 1/3] kunit: rename print_subtest_{start,end} for clarity (s/subtest/suite)

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 2376 bytes --]

On Wed, Apr 27, 2022 at 2:19 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> These names sound more general than they are.
>
> The _end() function increments a `static int kunit_suite_counter`, so it
> can only safely be called on suites, aka top-level subtests.
> It would need to have a separate counter for each level of subtest to be
> generic enough.
>
> So rename it to make it clear it's only appropriate for suites.
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---

Fair enough! This does make more sense now we have more complex hierarchies.

Reviewed-by: David Gow <davidgow@google.com>


>  lib/kunit/test.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 0f66c13d126e..64ee6a9d8003 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -134,7 +134,7 @@ size_t kunit_suite_num_test_cases(struct kunit_suite *suite)
>  }
>  EXPORT_SYMBOL_GPL(kunit_suite_num_test_cases);
>
> -static void kunit_print_subtest_start(struct kunit_suite *suite)
> +static void kunit_print_suite_start(struct kunit_suite *suite)
>  {
>         kunit_log(KERN_INFO, suite, KUNIT_SUBTEST_INDENT "# Subtest: %s",
>                   suite->name);
> @@ -192,7 +192,7 @@ EXPORT_SYMBOL_GPL(kunit_suite_has_succeeded);
>
>  static size_t kunit_suite_counter = 1;
>
> -static void kunit_print_subtest_end(struct kunit_suite *suite)
> +static void kunit_print_suite_end(struct kunit_suite *suite)
>  {
>         kunit_print_ok_not_ok((void *)suite, false,
>                               kunit_suite_has_succeeded(suite),
> @@ -498,7 +498,7 @@ int kunit_run_tests(struct kunit_suite *suite)
>         struct kunit_result_stats suite_stats = { 0 };
>         struct kunit_result_stats total_stats = { 0 };
>
> -       kunit_print_subtest_start(suite);
> +       kunit_print_suite_start(suite);
>
>         kunit_suite_for_each_test_case(suite, test_case) {
>                 struct kunit test = { .param_value = NULL, .param_index = 0 };
> @@ -552,7 +552,7 @@ int kunit_run_tests(struct kunit_suite *suite)
>         }
>
>         kunit_print_suite_stats(suite, suite_stats, total_stats);
> -       kunit_print_subtest_end(suite);
> +       kunit_print_suite_end(suite);
>
>         return 0;
>  }
>
> base-commit: 59729170afcd4900e08997a482467ffda8d88c7f
> --
> 2.36.0.rc2.479.g8af0fa9b8e-goog
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH 2/3] kunit: add ability to specify suite-level init and exit functions

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 8874 bytes --]

On Wed, Apr 27, 2022 at 2:19 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> KUnit has support for setup/cleanup logic for each test case in a suite.
> But it lacks the ability to specify setup/cleanup for the entire suite
> itself.
>
> This can be used to do setup that is too expensive or cumbersome to do
> for each test.
> Or it can be used to do simpler things like log debug information after
> the suite completes.
> It's a fairly common feature, so the lack of it is noticeable.
>
> Some examples in other frameworks and languages:
> * https://docs.python.org/3/library/unittest.html#setupclass-and-teardownclass
> * https://google.github.io/googletest/reference/testing.html#Test::SetUpTestSuite
>
> Meta:
> This is very similar to this patch here: https://lore.kernel.org/linux-kselftest/20210805043503.20252-3-bvanassche@acm.org/
> The changes from that patch:
> * pass in `struct kunit *` so users can do stuff like
>   `kunit_info(suite, "debug message")`
> * makes sure the init failure is bubbled up as a failure
> * updates kunit-example-test.c to use a suite init
> * Updates kunit/usage.rst to mention the new support
> * some minor cosmetic things
>   * use `suite_{init,exit}` instead of `{init/exit}_suite`
>   * make suite init error message more consistent w/ test init
>   * etc.
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---

Thanks for picking this up again: it's definitely something which has
been obviously missing for a while.

One comment below, but I don't mind if you'd prefer to leave things as-is.

Reviewed-by: David Gow <davidgow@google.com>

>  Documentation/dev-tools/kunit/usage.rst | 19 +++++++++++--------
>  include/kunit/test.h                    |  4 ++++
>  lib/kunit/kunit-example-test.c          | 14 ++++++++++++++
>  lib/kunit/test.c                        | 23 ++++++++++++++++++++---
>  4 files changed, 49 insertions(+), 11 deletions(-)
>
> diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
> index 1c83e7d60a8a..d62a04255c2e 100644
> --- a/Documentation/dev-tools/kunit/usage.rst
> +++ b/Documentation/dev-tools/kunit/usage.rst
> @@ -125,8 +125,8 @@ We need many test cases covering all the unit's behaviors. It is common to have
>  many similar tests. In order to reduce duplication in these closely related
>  tests, most unit testing frameworks (including KUnit) provide the concept of a
>  *test suite*. A test suite is a collection of test cases for a unit of code
> -with a setup function that gets invoked before every test case and then a tear
> -down function that gets invoked after every test case completes. For example:
> +with optional setup and teardown functions that run before/after the whole
> +suite and/or every test case. For example:
>
>  .. code-block:: c
>
> @@ -141,16 +141,19 @@ down function that gets invoked after every test case completes. For example:
>                 .name = "example",
>                 .init = example_test_init,
>                 .exit = example_test_exit,
> +               .suite_init = example_suite_init,
> +               .suite_exit = example_suite_exit,
>                 .test_cases = example_test_cases,
>         };
>         kunit_test_suite(example_test_suite);
>
> -In the above example, the test suite ``example_test_suite`` would run the test
> -cases ``example_test_foo``, ``example_test_bar``, and ``example_test_baz``. Each
> -would have ``example_test_init`` called immediately before it and
> -``example_test_exit`` called immediately after it.
> -``kunit_test_suite(example_test_suite)`` registers the test suite with the
> -KUnit test framework.
> +In the above example, the test suite ``example_test_suite`` would first run
> +``example_suite_init``, then run the test cases ``example_test_foo``,
> +``example_test_bar``, and ``example_test_baz``. Each would have
> +``example_test_init`` called immediately before it and ``example_test_exit``
> +called immediately after it. Finally, ``example_suite_exit`` would be called
> +after everything else. ``kunit_test_suite(example_test_suite)`` registers the
> +test suite with the KUnit test framework.
>
>  .. note::
>     A test case will only run if it is associated with a test suite.
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 97cd76461886..5d288f3d8f68 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -153,6 +153,8 @@ static inline char *kunit_status_to_ok_not_ok(enum kunit_status status)
>   * struct kunit_suite - describes a related collection of &struct kunit_case
>   *
>   * @name:      the name of the test. Purely informational.
> + * @suite_init:        called once per test suite before the test cases.
> + * @suite_exit:        called once per test suite after all test cases.
>   * @init:      called before every test case.
>   * @exit:      called after every test case.
>   * @test_cases:        a null terminated array of test cases.
> @@ -167,6 +169,8 @@ static inline char *kunit_status_to_ok_not_ok(enum kunit_status status)
>   */
>  struct kunit_suite {
>         const char name[256];
> +       int (*suite_init)(struct kunit_suite *suite);
> +       void (*suite_exit)(struct kunit_suite *suite);
>         int (*init)(struct kunit *test);
>         void (*exit)(struct kunit *test);
>         struct kunit_case *test_cases;
> diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
> index 91b1df7f59ed..f8fe582c9e36 100644
> --- a/lib/kunit/kunit-example-test.c
> +++ b/lib/kunit/kunit-example-test.c
> @@ -40,6 +40,17 @@ static int example_test_init(struct kunit *test)
>         return 0;
>  }
>
> +/*
> + * This is run once before all test cases in the suite.
> + * See the comment on example_test_suite for more information.
> + */
> +static int example_test_init_suite(struct kunit_suite *suite)
> +{
> +       kunit_info(suite, "initializing suite\n");
> +
> +       return 0;
> +}
> +
>  /*
>   * This test should always be skipped.
>   */
> @@ -142,17 +153,20 @@ static struct kunit_case example_test_cases[] = {
>   * may be specified which runs after every test case and can be used to for
>   * cleanup. For clarity, running tests in a test suite would behave as follows:
>   *
> + * suite.suite_init(suite);
>   * suite.init(test);
>   * suite.test_case[0](test);
>   * suite.exit(test);
>   * suite.init(test);
>   * suite.test_case[1](test);
>   * suite.exit(test);
> + * suite.suite_exit(suite);
>   * ...;
>   */
>  static struct kunit_suite example_test_suite = {
>         .name = "example",
>         .init = example_test_init,
> +       .suite_init = example_test_init_suite,
>         .test_cases = example_test_cases,
>  };
>
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 64ee6a9d8003..b66e395c795a 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -192,10 +192,13 @@ EXPORT_SYMBOL_GPL(kunit_suite_has_succeeded);
>
>  static size_t kunit_suite_counter = 1;
>
> -static void kunit_print_suite_end(struct kunit_suite *suite)
> +static void kunit_print_suite_end(struct kunit_suite *suite, int init_err)

A part of me feels that it'd be nicer to have the init_err be part of
struct kunit_suite, and have kunit_suite_has_succeeded() take it into
account. It could go either way, though -- WDYT?


>  {
> +       enum kunit_status status =
> +               init_err ? KUNIT_FAILURE : kunit_suite_has_succeeded(suite);
> +
>         kunit_print_ok_not_ok((void *)suite, false,
> -                             kunit_suite_has_succeeded(suite),
> +                             status,
>                               kunit_suite_counter++,
>                               suite->name,
>                               suite->status_comment);
> @@ -497,6 +500,16 @@ int kunit_run_tests(struct kunit_suite *suite)
>         struct kunit_case *test_case;
>         struct kunit_result_stats suite_stats = { 0 };
>         struct kunit_result_stats total_stats = { 0 };
> +       int suite_init_err = 0;
> +
> +       if (suite->suite_init) {
> +               suite_init_err = suite->suite_init(suite);
> +               if (suite_init_err != 0) {
> +                       kunit_err(suite, KUNIT_SUBTEST_INDENT
> +                                 "# failed to initialize (%d)", suite_init_err);
> +                       goto suite_end;
> +               }
> +       }
>
>         kunit_print_suite_start(suite);
>
> @@ -551,8 +564,12 @@ int kunit_run_tests(struct kunit_suite *suite)
>                 kunit_accumulate_stats(&total_stats, param_stats);
>         }
>
> +       if (suite->suite_exit)
> +               suite->suite_exit(suite);
> +
>         kunit_print_suite_stats(suite, suite_stats, total_stats);
> -       kunit_print_suite_end(suite);
> +suite_end:
> +       kunit_print_suite_end(suite, suite_init_err);
>
>         return 0;
>  }
> --
> 2.36.0.rc2.479.g8af0fa9b8e-goog
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH 3/3] kfence: test: use new suite_{init/exit} support, add .kunitconfig

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 4089 bytes --]

On Wed, Apr 27, 2022 at 2:19 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> Currently, the kfence test suite could not run via "normal" means since
> KUnit didn't support per-suite setup/teardown. So it manually called
> internal kunit functions to run itself.
> This has some downsides, like missing TAP headers => can't use kunit.py
> to run or even parse the test results (w/o tweaks).
>
> Use the newly added support and convert it over, adding a .kunitconfig
> so it's even easier to run from kunit.py.
>
> People can now run the test via
> $ ./tools/testing/kunit/kunit.py run --kunitconfig=mm/kfence --arch=x86_64
> ...
> [11:02:32] Testing complete. Passed: 23, Failed: 0, Crashed: 0, Skipped: 2, Errors: 0
> [11:02:32] Elapsed time: 43.562s total, 0.003s configuring, 9.268s building, 34.281s running
>
> Cc: kasan-dev@googlegroups.com
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---

This works for me: I'm very excited to see these tests run more nicely
with kunit_tool (and not break the TAP headers).

I guess the next one to tackle will be the Thunderbolt tests, though
those are more complicated still and need some module changes, IIRC.

Tested-by: David Gow <davidgow@google.com>

Cheers,
-- David


>  mm/kfence/.kunitconfig  |  6 ++++++
>  mm/kfence/kfence_test.c | 31 +++++++++++++------------------
>  2 files changed, 19 insertions(+), 18 deletions(-)
>  create mode 100644 mm/kfence/.kunitconfig
>
> diff --git a/mm/kfence/.kunitconfig b/mm/kfence/.kunitconfig
> new file mode 100644
> index 000000000000..f3d65e939bfa
> --- /dev/null
> +++ b/mm/kfence/.kunitconfig
> @@ -0,0 +1,6 @@
> +CONFIG_KUNIT=y
> +CONFIG_KFENCE=y
> +CONFIG_KFENCE_KUNIT_TEST=y
> +
> +# Additional dependencies.
> +CONFIG_FTRACE=y
> diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c
> index 1b50f70a4c0f..96206a4ee9ab 100644
> --- a/mm/kfence/kfence_test.c
> +++ b/mm/kfence/kfence_test.c
> @@ -826,14 +826,6 @@ static void test_exit(struct kunit *test)
>         test_cache_destroy();
>  }
>
> -static struct kunit_suite kfence_test_suite = {
> -       .name = "kfence",
> -       .test_cases = kfence_test_cases,
> -       .init = test_init,
> -       .exit = test_exit,
> -};
> -static struct kunit_suite *kfence_test_suites[] = { &kfence_test_suite, NULL };
> -
>  static void register_tracepoints(struct tracepoint *tp, void *ignore)
>  {
>         check_trace_callback_type_console(probe_console);
> @@ -847,11 +839,7 @@ static void unregister_tracepoints(struct tracepoint *tp, void *ignore)
>                 tracepoint_probe_unregister(tp, probe_console, NULL);
>  }
>
> -/*
> - * We only want to do tracepoints setup and teardown once, therefore we have to
> - * customize the init and exit functions and cannot rely on kunit_test_suite().
> - */
> -static int __init kfence_test_init(void)
> +static int kfence_suite_init(struct kunit_suite *suite)
>  {
>         /*
>          * Because we want to be able to build the test as a module, we need to
> @@ -859,18 +847,25 @@ static int __init kfence_test_init(void)
>          * won't work here.
>          */
>         for_each_kernel_tracepoint(register_tracepoints, NULL);
> -       return __kunit_test_suites_init(kfence_test_suites);
> +       return 0;
>  }
>
> -static void kfence_test_exit(void)
> +static void kfence_suite_exit(struct kunit_suite *suite)
>  {
> -       __kunit_test_suites_exit(kfence_test_suites);
>         for_each_kernel_tracepoint(unregister_tracepoints, NULL);
>         tracepoint_synchronize_unregister();
>  }
>
> -late_initcall_sync(kfence_test_init);
> -module_exit(kfence_test_exit);
> +static struct kunit_suite kfence_test_suite = {
> +       .name = "kfence",
> +       .test_cases = kfence_test_cases,
> +       .init = test_init,
> +       .exit = test_exit,
> +       .suite_init = kfence_suite_init,
> +       .suite_exit = kfence_suite_exit,
> +};
> +
> +kunit_test_suites(&kfence_test_suite);
>
>  MODULE_LICENSE("GPL v2");
>  MODULE_AUTHOR("Alexander Potapenko <glider@google.com>, Marco Elver <elver@google.com>");
> --
> 2.36.0.rc2.479.g8af0fa9b8e-goog
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH 2/3] kunit: add ability to specify suite-level init and exit functions

[Daniel Latypov]

On Tue, Apr 26, 2022 at 8:56 PM David Gow <davidgow@google.com> wrote:
> >
> >  static size_t kunit_suite_counter = 1;
> >
> > -static void kunit_print_suite_end(struct kunit_suite *suite)
> > +static void kunit_print_suite_end(struct kunit_suite *suite, int init_err)
>
> A part of me feels that it'd be nicer to have the init_err be part of
> struct kunit_suite, and have kunit_suite_has_succeeded() take it into
> account. It could go either way, though -- WDYT?

Yeah, passing it around as a parameter felt a bit icky.
But I think adding it in as a field feels worse.

Another thought: perhaps have this function take a `kunit_status`
parameter instead?
Moving the ?: expression below out into the caller isn't that bad, imo.

>
>
> >  {
> > +       enum kunit_status status =
> > +               init_err ? KUNIT_FAILURE : kunit_suite_has_succeeded(suite);
> > +

Re: [PATCH 3/3] kfence: test: use new suite_{init/exit} support, add .kunitconfig

[Marco Elver]

On Tue, Apr 26, 2022 at 11:19AM -0700, 'Daniel Latypov' via kasan-dev wrote:
> Currently, the kfence test suite could not run via "normal" means since
> KUnit didn't support per-suite setup/teardown. So it manually called
> internal kunit functions to run itself.
> This has some downsides, like missing TAP headers => can't use kunit.py
> to run or even parse the test results (w/o tweaks).
> 
> Use the newly added support and convert it over, adding a .kunitconfig
> so it's even easier to run from kunit.py.
> 
> People can now run the test via
> $ ./tools/testing/kunit/kunit.py run --kunitconfig=mm/kfence --arch=x86_64
> ...
> [11:02:32] Testing complete. Passed: 23, Failed: 0, Crashed: 0, Skipped: 2, Errors: 0
> [11:02:32] Elapsed time: 43.562s total, 0.003s configuring, 9.268s building, 34.281s running
> 
> Cc: kasan-dev@googlegroups.com
> Signed-off-by: Daniel Latypov <dlatypov@google.com>

Reviewed-by: Marco Elver <elver@google.com>

> ---
>  mm/kfence/.kunitconfig  |  6 ++++++
>  mm/kfence/kfence_test.c | 31 +++++++++++++------------------
>  2 files changed, 19 insertions(+), 18 deletions(-)
>  create mode 100644 mm/kfence/.kunitconfig
> 
> diff --git a/mm/kfence/.kunitconfig b/mm/kfence/.kunitconfig
> new file mode 100644
> index 000000000000..f3d65e939bfa
> --- /dev/null
> +++ b/mm/kfence/.kunitconfig
> @@ -0,0 +1,6 @@
> +CONFIG_KUNIT=y
> +CONFIG_KFENCE=y
> +CONFIG_KFENCE_KUNIT_TEST=y
> +
> +# Additional dependencies.
> +CONFIG_FTRACE=y
> diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c
> index 1b50f70a4c0f..96206a4ee9ab 100644
> --- a/mm/kfence/kfence_test.c
> +++ b/mm/kfence/kfence_test.c
> @@ -826,14 +826,6 @@ static void test_exit(struct kunit *test)
>  	test_cache_destroy();
>  }
>  
> -static struct kunit_suite kfence_test_suite = {
> -	.name = "kfence",
> -	.test_cases = kfence_test_cases,
> -	.init = test_init,
> -	.exit = test_exit,
> -};
> -static struct kunit_suite *kfence_test_suites[] = { &kfence_test_suite, NULL };
> -
>  static void register_tracepoints(struct tracepoint *tp, void *ignore)
>  {
>  	check_trace_callback_type_console(probe_console);
> @@ -847,11 +839,7 @@ static void unregister_tracepoints(struct tracepoint *tp, void *ignore)
>  		tracepoint_probe_unregister(tp, probe_console, NULL);
>  }
>  
> -/*
> - * We only want to do tracepoints setup and teardown once, therefore we have to
> - * customize the init and exit functions and cannot rely on kunit_test_suite().
> - */
> -static int __init kfence_test_init(void)
> +static int kfence_suite_init(struct kunit_suite *suite)
>  {
>  	/*
>  	 * Because we want to be able to build the test as a module, we need to
> @@ -859,18 +847,25 @@ static int __init kfence_test_init(void)
>  	 * won't work here.
>  	 */
>  	for_each_kernel_tracepoint(register_tracepoints, NULL);
> -	return __kunit_test_suites_init(kfence_test_suites);
> +	return 0;
>  }
>  
> -static void kfence_test_exit(void)
> +static void kfence_suite_exit(struct kunit_suite *suite)
>  {
> -	__kunit_test_suites_exit(kfence_test_suites);
>  	for_each_kernel_tracepoint(unregister_tracepoints, NULL);
>  	tracepoint_synchronize_unregister();
>  }
>  
> -late_initcall_sync(kfence_test_init);
> -module_exit(kfence_test_exit);
> +static struct kunit_suite kfence_test_suite = {
> +	.name = "kfence",
> +	.test_cases = kfence_test_cases,
> +	.init = test_init,
> +	.exit = test_exit,
> +	.suite_init = kfence_suite_init,
> +	.suite_exit = kfence_suite_exit,
> +};
> +
> +kunit_test_suites(&kfence_test_suite);

Much nicer!

Thanks,
-- Marco

Re: [PATCH 2/3] kunit: add ability to specify suite-level init and exit functions

[David Gow]

On Wed, Apr 27, 2022 at 11:06 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> On Tue, Apr 26, 2022 at 8:56 PM David Gow <davidgow@google.com> wrote:
> > >
> > >  static size_t kunit_suite_counter = 1;
> > >
> > > -static void kunit_print_suite_end(struct kunit_suite *suite)
> > > +static void kunit_print_suite_end(struct kunit_suite *suite, int init_err)
> >
> > A part of me feels that it'd be nicer to have the init_err be part of
> > struct kunit_suite, and have kunit_suite_has_succeeded() take it into
> > account. It could go either way, though -- WDYT?
>
> Yeah, passing it around as a parameter felt a bit icky.
> But I think adding it in as a field feels worse.

Personally, I don't have a problem with having it as a field (other
than the memory usage, which shouldn't be so much as to cause
problems). It seems that the suite result is logically part of the
suite, and given that status_comment is in struct kunit_suite and
there's a kunit_status field in kunit_case, having it as a field in
the suite seems the most logically consistent thing to do.

>
> Another thought: perhaps have this function take a `kunit_status`
> parameter instead?
> Moving the ?: expression below out into the caller isn't that bad, imo.

It still doesn't solve the fact that kunit_suite_has_succeeded() no
longer tells you if a suite has succeeded or not. If we stick with
this (with the conditional either here or in the caller), I think we
should at least rename kunit_suite_has_succeded() to something like
kunit_suite_subtests_status().

That being said, I do prefer passing a 'kunit_status' around to an 'int'.

>
> >
> >
> > >  {
> > > +       enum kunit_status status =
> > > +               init_err ? KUNIT_FAILURE : kunit_suite_has_succeeded(suite);
> > > +

Re: [PATCH 2/3] kunit: add ability to specify suite-level init and exit functions

[Daniel Latypov]

On Fri, Apr 29, 2022 at 1:01 AM David Gow <davidgow@google.com> wrote:
>
> On Wed, Apr 27, 2022 at 11:06 AM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > On Tue, Apr 26, 2022 at 8:56 PM David Gow <davidgow@google.com> wrote:
> > > >
> > > >  static size_t kunit_suite_counter = 1;
> > > >
> > > > -static void kunit_print_suite_end(struct kunit_suite *suite)
> > > > +static void kunit_print_suite_end(struct kunit_suite *suite, int init_err)
> > >
> > > A part of me feels that it'd be nicer to have the init_err be part of
> > > struct kunit_suite, and have kunit_suite_has_succeeded() take it into
> > > account. It could go either way, though -- WDYT?
> >
> > Yeah, passing it around as a parameter felt a bit icky.
> > But I think adding it in as a field feels worse.
>
> Personally, I don't have a problem with having it as a field (other
> than the memory usage, which shouldn't be so much as to cause
> problems). It seems that the suite result is logically part of the
> suite, and given that status_comment is in struct kunit_suite and
> there's a kunit_status field in kunit_case, having it as a field in
> the suite seems the most logically consistent thing to do.
>
> >
> > Another thought: perhaps have this function take a `kunit_status`
> > parameter instead?
> > Moving the ?: expression below out into the caller isn't that bad, imo.
>
> It still doesn't solve the fact that kunit_suite_has_succeeded() no
> longer tells you if a suite has succeeded or not. If we stick with

I forgot kunit_suite_has_succeeded() is called in the debugfs code.
So it looks like we need to track the init error in struct
kunit_suite, as you said.

It might be cleaner to just store a status in the suite eventually,
but I'll just add the int for now.

Sending a v2 series here:
https://lore.kernel.org/linux-kselftest/20220429181259.622060-1-dlatypov@google.com
I also added on a new patch to fix the type confusion in the debugfs
code (using bool instead of enum kunit_status).