Re: [PATCH v2 linux-kselftest-test 1/3] kunit: allow kunit tests to be loaded as a module
From: Brendan Higgins
Date: Tue Oct 08 2019 - 17:35:44 EST
On Tue, Oct 08, 2019 at 03:55:44PM +0100, Alan Maguire wrote:
> as tests are added to kunit, it will become less feasible to execute
> all built tests together. By supporting modular tests we provide
> a simple way to do selective execution on a running system; specifying
>
> CONFIG_KUNIT=y
> CONFIG_KUNIT_EXAMPLE_TEST=m
>
> ...means we can simply "insmod example-test.ko" to run the tests.
>
> To achieve this we need to
>
> o export the required symbols in kunit
> o support a new way of declaring test suites. Because a module cannot
> do multiple late_initcall()s, we provide a kunit_test_suites() macro
> to declare multiple suites within the same module at once.
>
> Signed-off-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
> Signed-off-by: Knut Omang <knut.omang@xxxxxxxxxx>
>
> ---
> include/kunit/test.h | 30 +++++++++++++++++++++++-------
> kernel/sysctl-test.c | 6 +++++-
> lib/Kconfig.debug | 4 ++--
> lib/kunit/Kconfig | 4 ++--
> lib/kunit/assert.c | 8 ++++++++
> lib/kunit/example-test.c | 6 +++++-
> lib/kunit/string-stream-test.c | 9 +++++++--
> lib/kunit/string-stream.c | 7 +++++++
> lib/kunit/test-test.c | 8 ++++++--
> lib/kunit/test.c | 8 ++++++++
> lib/kunit/try-catch.c | 8 ++++++--
> 11 files changed, 79 insertions(+), 19 deletions(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index dba4830..9fc6c1b 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -12,6 +12,7 @@
> #include <kunit/assert.h>
> #include <kunit/try-catch.h>
> #include <linux/kernel.h>
> +#include <linux/module.h>
> #include <linux/slab.h>
> #include <linux/types.h>
>
> @@ -204,24 +205,39 @@ struct kunit {
> * Registers @suite with the test framework. See &struct kunit_suite for
> * more information.
> *
> - * NOTE: Currently KUnit tests are all run as late_initcalls; this means
> + * When builtin, KUnit tests are all run as late_initcalls; this means
> * that they cannot test anything where tests must run at a different init
> * phase. One significant restriction resulting from this is that KUnit
> * cannot reliably test anything that is initialize in the late_init phase;
> * another is that KUnit is useless to test things that need to be run in
> * an earlier init phase.
> *
> + * An alternative is to build the tests as a module. Because modules
> + * do not support multiple late_initcall()s, we need to initialize an
> + * array of suites for a module.
> + *
> * TODO(brendanhiggins@xxxxxxxxxx): Don't run all KUnit tests as
> * late_initcalls. I have some future work planned to dispatch all KUnit
> * tests from the same place, and at the very least to do so after
> * everything else is definitely initialized.
> */
> -#define kunit_test_suite(suite) \
> - static int kunit_suite_init##suite(void) \
> - { \
> - return kunit_run_tests(&suite); \
> - } \
> - late_initcall(kunit_suite_init##suite)
> +#define kunit_test_suites(...) \
> + static struct kunit_suite *suites[] = { __VA_ARGS__, NULL}; \
> + static int kunit_test_suites_init(void) \
> + { \
> + unsigned int i; \
> + for (i = 0; suites[i] != NULL; i++) \
> + kunit_run_tests(suites[i]); \
> + return 0; \
> + } \
> + late_initcall(kunit_test_suites_init); \
> + static void __exit kunit_test_suites_exit(void) \
> + { \
> + return; \
> + } \
> + module_exit(kunit_test_suites_exit)
> +
> +#define kunit_test_suite(suite) kunit_test_suites(suite)
I think it is fine to just rename this kunit_test_suites.
> /*
> * Like kunit_alloc_resource() below, but returns the struct kunit_resource
> diff --git a/kernel/sysctl-test.c b/kernel/sysctl-test.c
> index 2a63241..15161c5 100644
> --- a/kernel/sysctl-test.c
> +++ b/kernel/sysctl-test.c
> @@ -389,4 +389,8 @@ static void sysctl_test_api_dointvec_write_single_greater_int_max(
> .test_cases = sysctl_test_cases,
> };
>
> -kunit_test_suite(sysctl_test_suite);
> +kunit_test_suite(&sysctl_test_suite);
> +
> +#ifdef MODULE
> +MODULE_LICENSE("GPL");
> +#endif /* MODULE */
Here and elsewhere: the "ifdef/endif MODULE" should not be necessary.
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index a3017a5..f9f411a6 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1951,10 +1951,10 @@ config TEST_SYSCTL
> If unsure, say N.
>
> config SYSCTL_KUNIT_TEST
> - bool "KUnit test for sysctl"
> + tristate "KUnit test for sysctl"
> depends on KUNIT
> help
> - This builds the proc sysctl unit test, which runs on boot.
> + This builds the proc sysctl unit test, which runs on boot/module load.
> Tests the API contract and implementation correctness of sysctl.
> For more information on KUnit and unit tests in general please refer
> to the KUnit documentation in Documentation/dev-tools/kunit/.
[...]
> diff --git a/lib/kunit/example-test.c b/lib/kunit/example-test.c
> index f64a829..6c6a408 100644
> --- a/lib/kunit/example-test.c
> +++ b/lib/kunit/example-test.c
> @@ -85,4 +85,8 @@ static int example_test_init(struct kunit *test)
> * This registers the above test suite telling KUnit that this is a suite of
> * tests that need to be run.
> */
> -kunit_test_suite(example_test_suite);
> +kunit_test_suite(&example_test_suite);
> +
> +#ifdef MODULE
> +MODULE_LICENSE("GPL");
> +#endif /* MODULE */
nit: The "ifdef/endif MODULE" should not be necessary.
> diff --git a/lib/kunit/string-stream-test.c b/lib/kunit/string-stream-test.c
> index 76cc05e..7a3e7a0 100644
> --- a/lib/kunit/string-stream-test.c
> +++ b/lib/kunit/string-stream-test.c
> @@ -45,8 +45,13 @@ static void string_stream_test_get_string(struct kunit *test)
> {}
> };
>
> -static struct kunit_suite string_stream_test_suite = {
> +struct kunit_suite string_stream_test_suite = {
> .name = "string-stream-test",
> .test_cases = string_stream_test_cases
> };
> -kunit_test_suite(string_stream_test_suite);
> +
> +kunit_test_suite(&string_stream_test_suite);
> +
> +#ifdef MODULE
> +MODULE_LICENSE("GPL");
> +#endif /* MODULE */
> diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
> index e6d17aa..e4f3a97 100644
> --- a/lib/kunit/string-stream.c
> +++ b/lib/kunit/string-stream.c
> @@ -100,6 +100,7 @@ int string_stream_vadd(struct string_stream *stream,
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(string_stream_vadd);
Is this actually needed by anything other than lib/kunit/test.c right
now? Maybe we should move the include file into the kunit/ directory to
hide these so no one else can use them.
> int string_stream_add(struct string_stream *stream, const char *fmt, ...)
> {
> @@ -112,6 +113,7 @@ int string_stream_add(struct string_stream *stream, const char *fmt, ...)
>
> return result;
> }
> +EXPORT_SYMBOL_GPL(string_stream_add);
[...]
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index c83c0fa..e7896f1 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
[...]
> @@ -50,6 +51,7 @@ static unsigned long kunit_test_timeout(void)
> * For more background on this topic, see:
> * https://mike-bland.com/2011/11/01/small-medium-large.html
> */
> +#ifndef MODULE
Why is this block of code "ifndef MODULE"?
> if (sysctl_hung_task_timeout_secs) {
> /*
> * If sysctl_hung_task is active, just set the timeout to some
> @@ -60,9 +62,9 @@ static unsigned long kunit_test_timeout(void)
> */
> timeout_msecs = (sysctl_hung_task_timeout_secs - 1) *
> MSEC_PER_SEC;
> - } else {
> + } else
> +#endif
> timeout_msecs = 300 * MSEC_PER_SEC; /* 5 min */
> - }
>
> return timeout_msecs;
> }
> @@ -106,6 +108,7 @@ void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context)
>
> try_catch->catch(try_catch->context);
> }
> +EXPORT_SYMBOL_GPL(kunit_try_catch_run);
>
> void kunit_try_catch_init(struct kunit_try_catch *try_catch,
> struct kunit *test,
> @@ -116,3 +119,4 @@ void kunit_try_catch_init(struct kunit_try_catch *try_catch,
> try_catch->try = try;
> try_catch->catch = catch;
> }
> +EXPORT_SYMBOL_GPL(kunit_try_catch_init);
This code should also probably be hidden from outside of kunit/.