linux-arch.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 0/5] kunit: create a centralized executor to dispatch all KUnit tests
@ 2020-08-04 20:47 Brendan Higgins
  2020-08-04 20:47 ` Brendan Higgins
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Brendan Higgins @ 2020-08-04 20:47 UTC (permalink / raw)
  To: jdike, richard, anton.ivanov, arnd, keescook, skhan,
	alan.maguire, yzaikin, davidgow, akpm, rppt, frowand.list
  Cc: gregkh, sboyd, logang, mcgrof, linux-arch, linux-kselftest,
	kunit-dev, linux-kernel, linux-doc, Brendan Higgins

## TL;DR

This patchset adds a centralized executor to dispatch tests rather than
relying on late_initcall to schedule each test suite separately along
with a couple of new features that depend on it.

## What am I trying to do?

Conceptually, I am trying to provide a mechanism by which test suites
can be grouped together so that they can be reasoned about collectively.
The second to last patch in this series add features which depend on
this:

PATCH 04/05 Prints out a test plan[1] right before KUnit tests are run;
            this is valuable because it makes it possible for a test
            harness to detect whether the number of tests run matches
            the number of tests expected to be run, ensuring that no
            tests silently failed. The test plan includes a count of
            tests that will run. With the centralized executor, the
            tests are located in a single data structure and thus can be
            counted.

In addition, by dispatching tests from a single location, we can
guarantee that all KUnit tests run after late_init is complete, which
was a concern during the initial KUnit patchset review (this has not
been a problem in practice, but resolving with certainty is nevertheless
desirable).

Other use cases for this exist, but the above features should provide an
idea of the value that this could provide.

## Changes since last revision:
 - Renamed the KUNIT_TEST_SUITES the KUNIT_TABLE section and moved it
   from INIT_DATA_SECTION to INIT_DATA; this had the additional
   consequence of making the first several architecture specific patches
   unnecessary - suggested by Kees.
 - Dropped the kunit_shutdown patches; I think it makes more sense to
   reintroduce them in a later patchset.

Alan Maguire (1):
  kunit: test: create a single centralized executor for all tests

Brendan Higgins (4):
  vmlinux.lds.h: add linker section for KUnit test suites
  init: main: add KUnit to kernel init
  kunit: test: add test plan to KUnit TAP format
  Documentation: kunit: add a brief blurb about kunit_test_suite

 Documentation/dev-tools/kunit/usage.rst       |   5 ++
 include/asm-generic/vmlinux.lds.h             |  10 ++-
 include/kunit/test.h                          |  76 +++++++++++++-----
 init/main.c                                   |   4 +
 lib/kunit/Makefile                            |   3 +-
 lib/kunit/executor.c                          |  43 ++++++++++
 lib/kunit/test.c                              |  13 +--
 tools/testing/kunit/kunit_parser.py           |  76 ++++++++++++++----
 .../test_is_test_passed-all_passed.log        | Bin 1562 -> 1567 bytes
 .../test_data/test_is_test_passed-crash.log   | Bin 3016 -> 3021 bytes
 .../test_data/test_is_test_passed-failure.log | Bin 1700 -> 1705 bytes
 11 files changed, 180 insertions(+), 50 deletions(-)
 create mode 100644 lib/kunit/executor.c


base-commit: 145ff1ec090dce9beb5a9590b5dc288e7bb2e65d
-- 
2.28.0.163.g6104cc2f0b6-goog

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v6 0/5] kunit: create a centralized executor to dispatch all KUnit tests
  2020-08-04 20:47 [PATCH v6 0/5] kunit: create a centralized executor to dispatch all KUnit tests Brendan Higgins
@ 2020-08-04 20:47 ` Brendan Higgins
  2020-08-04 20:47 ` [PATCH v6 1/5] vmlinux.lds.h: add linker section for KUnit test suites Brendan Higgins
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Brendan Higgins @ 2020-08-04 20:47 UTC (permalink / raw)
  To: jdike, richard, anton.ivanov, arnd, keescook, skhan,
	alan.maguire, yzaikin, davidgow, akpm, rppt, frowand.list
  Cc: gregkh, sboyd, logang, mcgrof, linux-arch, linux-kselftest,
	kunit-dev, linux-kernel, linux-doc, Brendan Higgins

## TL;DR

This patchset adds a centralized executor to dispatch tests rather than
relying on late_initcall to schedule each test suite separately along
with a couple of new features that depend on it.

## What am I trying to do?

Conceptually, I am trying to provide a mechanism by which test suites
can be grouped together so that they can be reasoned about collectively.
The second to last patch in this series add features which depend on
this:

PATCH 04/05 Prints out a test plan[1] right before KUnit tests are run;
            this is valuable because it makes it possible for a test
            harness to detect whether the number of tests run matches
            the number of tests expected to be run, ensuring that no
            tests silently failed. The test plan includes a count of
            tests that will run. With the centralized executor, the
            tests are located in a single data structure and thus can be
            counted.

In addition, by dispatching tests from a single location, we can
guarantee that all KUnit tests run after late_init is complete, which
was a concern during the initial KUnit patchset review (this has not
been a problem in practice, but resolving with certainty is nevertheless
desirable).

Other use cases for this exist, but the above features should provide an
idea of the value that this could provide.

## Changes since last revision:
 - Renamed the KUNIT_TEST_SUITES the KUNIT_TABLE section and moved it
   from INIT_DATA_SECTION to INIT_DATA; this had the additional
   consequence of making the first several architecture specific patches
   unnecessary - suggested by Kees.
 - Dropped the kunit_shutdown patches; I think it makes more sense to
   reintroduce them in a later patchset.

Alan Maguire (1):
  kunit: test: create a single centralized executor for all tests

Brendan Higgins (4):
  vmlinux.lds.h: add linker section for KUnit test suites
  init: main: add KUnit to kernel init
  kunit: test: add test plan to KUnit TAP format
  Documentation: kunit: add a brief blurb about kunit_test_suite

 Documentation/dev-tools/kunit/usage.rst       |   5 ++
 include/asm-generic/vmlinux.lds.h             |  10 ++-
 include/kunit/test.h                          |  76 +++++++++++++-----
 init/main.c                                   |   4 +
 lib/kunit/Makefile                            |   3 +-
 lib/kunit/executor.c                          |  43 ++++++++++
 lib/kunit/test.c                              |  13 +--
 tools/testing/kunit/kunit_parser.py           |  76 ++++++++++++++----
 .../test_is_test_passed-all_passed.log        | Bin 1562 -> 1567 bytes
 .../test_data/test_is_test_passed-crash.log   | Bin 3016 -> 3021 bytes
 .../test_data/test_is_test_passed-failure.log | Bin 1700 -> 1705 bytes
 11 files changed, 180 insertions(+), 50 deletions(-)
 create mode 100644 lib/kunit/executor.c


base-commit: 145ff1ec090dce9beb5a9590b5dc288e7bb2e65d
-- 
2.28.0.163.g6104cc2f0b6-goog

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v6 1/5] vmlinux.lds.h: add linker section for KUnit test suites
  2020-08-04 20:47 [PATCH v6 0/5] kunit: create a centralized executor to dispatch all KUnit tests Brendan Higgins
  2020-08-04 20:47 ` Brendan Higgins
@ 2020-08-04 20:47 ` Brendan Higgins
  2020-08-04 20:47 ` [PATCH v6 2/5] kunit: test: create a single centralized executor for all tests Brendan Higgins
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Brendan Higgins @ 2020-08-04 20:47 UTC (permalink / raw)
  To: jdike, richard, anton.ivanov, arnd, keescook, skhan,
	alan.maguire, yzaikin, davidgow, akpm, rppt, frowand.list
  Cc: gregkh, sboyd, logang, mcgrof, linux-arch, linux-kselftest,
	kunit-dev, linux-kernel, linux-doc, Brendan Higgins

Add a linker section where KUnit can put references to its test suites.
This patch is the first step in transitioning to dispatching all KUnit
tests from a centralized executor rather than having each as its own
separate late_initcall.

Co-developed-by: Iurii Zaikin <yzaikin@google.com>
Signed-off-by: Iurii Zaikin <yzaikin@google.com>
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
---
 include/asm-generic/vmlinux.lds.h | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 052e0f05a9841..75130a4d92ef0 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -692,7 +692,8 @@
 	THERMAL_TABLE(governor)						\
 	EARLYCON_TABLE()						\
 	LSM_TABLE()							\
-	EARLY_LSM_TABLE()
+	EARLY_LSM_TABLE()						\
+	KUNIT_TABLE()
 
 #define INIT_TEXT							\
 	*(.init.text .init.text.*)					\
@@ -884,6 +885,13 @@
 		KEEP(*(.con_initcall.init))				\
 		__con_initcall_end = .;
 
+/* Alignment must be consistent with (kunit_suite *) in include/kunit/test.h */
+#define KUNIT_TABLE()							\
+		. = ALIGN(8);						\
+		__kunit_suites_start = .;				\
+		KEEP(*(.kunit_test_suites))				\
+		__kunit_suites_end = .;
+
 #ifdef CONFIG_BLK_DEV_INITRD
 #define INIT_RAM_FS							\
 	. = ALIGN(4);							\
-- 
2.28.0.163.g6104cc2f0b6-goog

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v6 2/5] kunit: test: create a single centralized executor for all tests
  2020-08-04 20:47 [PATCH v6 0/5] kunit: create a centralized executor to dispatch all KUnit tests Brendan Higgins
  2020-08-04 20:47 ` Brendan Higgins
  2020-08-04 20:47 ` [PATCH v6 1/5] vmlinux.lds.h: add linker section for KUnit test suites Brendan Higgins
@ 2020-08-04 20:47 ` Brendan Higgins
  2020-10-13 11:09   ` Mauro Carvalho Chehab
  2020-08-04 20:47 ` [PATCH v6 3/5] init: main: add KUnit to kernel init Brendan Higgins
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Brendan Higgins @ 2020-08-04 20:47 UTC (permalink / raw)
  To: jdike, richard, anton.ivanov, arnd, keescook, skhan,
	alan.maguire, yzaikin, davidgow, akpm, rppt, frowand.list
  Cc: gregkh, sboyd, logang, mcgrof, linux-arch, linux-kselftest,
	kunit-dev, linux-kernel, linux-doc, Brendan Higgins

From: Alan Maguire <alan.maguire@oracle.com>

Add a centralized executor to dispatch tests rather than relying on
late_initcall to schedule each test suite separately. Centralized
execution is for built-in tests only; modules will execute tests when
loaded.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Co-developed-by: Iurii Zaikin <yzaikin@google.com>
Signed-off-by: Iurii Zaikin <yzaikin@google.com>
Co-developed-by: Brendan Higgins <brendanhiggins@google.com>
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
---
 include/kunit/test.h | 67 +++++++++++++++++++++++++++++---------------
 lib/kunit/Makefile   |  3 +-
 lib/kunit/executor.c | 28 ++++++++++++++++++
 lib/kunit/test.c     |  2 +-
 4 files changed, 76 insertions(+), 24 deletions(-)
 create mode 100644 lib/kunit/executor.c

diff --git a/include/kunit/test.h b/include/kunit/test.h
index 47e61e1d53370..f3e86c3953a2b 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -224,7 +224,7 @@ size_t kunit_suite_num_test_cases(struct kunit_suite *suite);
 unsigned int kunit_test_case_num(struct kunit_suite *suite,
 				 struct kunit_case *test_case);
 
-int __kunit_test_suites_init(struct kunit_suite **suites);
+int __kunit_test_suites_init(struct kunit_suite * const * const suites);
 
 void __kunit_test_suites_exit(struct kunit_suite **suites);
 
@@ -237,34 +237,57 @@ void __kunit_test_suites_exit(struct kunit_suite **suites);
  * Registers @suites_list with the test framework. See &struct kunit_suite for
  * more information.
  *
- * 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@google.com): 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.
+ * If a test suite is built-in, module_init() gets translated into
+ * an initcall which we don't want as the idea is that for builtins
+ * the executor will manage execution.  So ensure we do not define
+ * module_{init|exit} functions for the builtin case when registering
+ * suites via kunit_test_suites() below.
  */
-#define kunit_test_suites(suites_list...)				\
-	static struct kunit_suite *suites[] = {suites_list, NULL};	\
-	static int kunit_test_suites_init(void)				\
+#ifdef MODULE
+#define kunit_test_suites_for_module(__suites)				\
+	static int __init kunit_test_suites_init(void)			\
 	{								\
-		return __kunit_test_suites_init(suites);		\
+		return __kunit_test_suites_init(__suites);		\
 	}								\
-	late_initcall(kunit_test_suites_init);				\
+	module_init(kunit_test_suites_init);				\
+									\
 	static void __exit kunit_test_suites_exit(void)			\
 	{								\
-		return __kunit_test_suites_exit(suites);		\
+		return __kunit_test_suites_exit(__suites);		\
 	}								\
 	module_exit(kunit_test_suites_exit)
+#else
+#define kunit_test_suites_for_module(__suites)
+#endif /* MODULE */
+
+#define __kunit_test_suites(unique_array, unique_suites, ...)		       \
+	static struct kunit_suite *unique_array[] = { __VA_ARGS__, NULL };     \
+	kunit_test_suites_for_module(unique_array);			       \
+	static struct kunit_suite **unique_suites			       \
+	__used __section(.kunit_test_suites) = unique_array
+
+/**
+ * kunit_test_suites() - used to register one or more &struct kunit_suite
+ *			 with KUnit.
+ *
+ * @suites: a statically allocated list of &struct kunit_suite.
+ *
+ * Registers @suites with the test framework. See &struct kunit_suite for
+ * more information.
+ *
+ * When builtin,  KUnit tests are all run via executor; this is done
+ * by placing the array of struct kunit_suite * in the .kunit_test_suites
+ * ELF section.
+ *
+ * An alternative is to build the tests as a module.  Because modules do not
+ * support multiple initcall()s, we need to initialize an array of suites for a
+ * module.
+ *
+ */
+#define kunit_test_suites(...)						\
+	__kunit_test_suites(__UNIQUE_ID(array),				\
+			    __UNIQUE_ID(suites),			\
+			    __VA_ARGS__)
 
 #define kunit_test_suite(suite)	kunit_test_suites(&suite)
 
diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
index 724b94311ca36..c49f4ffb6273a 100644
--- a/lib/kunit/Makefile
+++ b/lib/kunit/Makefile
@@ -3,7 +3,8 @@ obj-$(CONFIG_KUNIT) +=			kunit.o
 kunit-objs +=				test.o \
 					string-stream.o \
 					assert.o \
-					try-catch.o
+					try-catch.o \
+					executor.o
 
 ifeq ($(CONFIG_KUNIT_DEBUGFS),y)
 kunit-objs +=				debugfs.o
diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
new file mode 100644
index 0000000000000..7015e7328dce7
--- /dev/null
+++ b/lib/kunit/executor.c
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <kunit/test.h>
+
+/*
+ * These symbols point to the .kunit_test_suites section and are defined in
+ * include/asm-generic/vmlinux.lds.h, and consequently must be extern.
+ */
+extern struct kunit_suite * const * const __kunit_suites_start[];
+extern struct kunit_suite * const * const __kunit_suites_end[];
+
+#if IS_BUILTIN(CONFIG_KUNIT)
+
+static int kunit_run_all_tests(void)
+{
+	struct kunit_suite * const * const *suites;
+
+	for (suites = __kunit_suites_start;
+	     suites < __kunit_suites_end;
+	     suites++)
+			__kunit_test_suites_init(*suites);
+
+	return 0;
+}
+
+late_initcall(kunit_run_all_tests);
+
+#endif /* IS_BUILTIN(CONFIG_KUNIT) */
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index ccb2ffad8dcfa..918dff400a9d7 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -380,7 +380,7 @@ static void kunit_init_suite(struct kunit_suite *suite)
 	kunit_debugfs_create_suite(suite);
 }
 
-int __kunit_test_suites_init(struct kunit_suite **suites)
+int __kunit_test_suites_init(struct kunit_suite * const * const suites)
 {
 	unsigned int i;
 
-- 
2.28.0.163.g6104cc2f0b6-goog

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v6 3/5] init: main: add KUnit to kernel init
  2020-08-04 20:47 [PATCH v6 0/5] kunit: create a centralized executor to dispatch all KUnit tests Brendan Higgins
                   ` (2 preceding siblings ...)
  2020-08-04 20:47 ` [PATCH v6 2/5] kunit: test: create a single centralized executor for all tests Brendan Higgins
@ 2020-08-04 20:47 ` Brendan Higgins
  2020-08-04 20:47 ` [PATCH v6 4/5] kunit: test: add test plan to KUnit TAP format Brendan Higgins
  2020-08-04 20:47 ` [PATCH v6 5/5] Documentation: kunit: add a brief blurb about kunit_test_suite Brendan Higgins
  5 siblings, 0 replies; 11+ messages in thread
From: Brendan Higgins @ 2020-08-04 20:47 UTC (permalink / raw)
  To: jdike, richard, anton.ivanov, arnd, keescook, skhan,
	alan.maguire, yzaikin, davidgow, akpm, rppt, frowand.list
  Cc: gregkh, sboyd, logang, mcgrof, linux-arch, linux-kselftest,
	kunit-dev, linux-kernel, linux-doc, Brendan Higgins

Although we have not seen any actual examples where KUnit doesn't work
because it runs in the late init phase of the kernel, it has been a
concern for some time that this could potentially be an issue in the
future. So, remove KUnit from init calls entirely, instead call directly
from kernel_init() so that KUnit runs after late init.

Co-developed-by: Alan Maguire <alan.maguire@oracle.com>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>
---
 include/kunit/test.h | 9 +++++++++
 init/main.c          | 4 ++++
 lib/kunit/executor.c | 4 +---
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index f3e86c3953a2b..795039d9f1917 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -228,6 +228,15 @@ int __kunit_test_suites_init(struct kunit_suite * const * const suites);
 
 void __kunit_test_suites_exit(struct kunit_suite **suites);
 
+#if IS_BUILTIN(CONFIG_KUNIT)
+int kunit_run_all_tests(void);
+#else
+static inline int kunit_run_all_tests(void)
+{
+	return 0;
+}
+#endif /* IS_BUILTIN(CONFIG_KUNIT) */
+
 /**
  * kunit_test_suites() - used to register one or more &struct kunit_suite
  *			 with KUnit.
diff --git a/init/main.c b/init/main.c
index 0ead83e86b5aa..d3101d8874dea 100644
--- a/init/main.c
+++ b/init/main.c
@@ -106,6 +106,8 @@
 #define CREATE_TRACE_POINTS
 #include <trace/events/initcall.h>
 
+#include <kunit/test.h>
+
 static int kernel_init(void *);
 
 extern void init_IRQ(void);
@@ -1504,6 +1506,8 @@ static noinline void __init kernel_init_freeable(void)
 
 	do_basic_setup();
 
+	kunit_run_all_tests();
+
 	console_on_rootfs();
 
 	/*
diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index 7015e7328dce7..4aab7f70a88c3 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c
@@ -11,7 +11,7 @@ extern struct kunit_suite * const * const __kunit_suites_end[];
 
 #if IS_BUILTIN(CONFIG_KUNIT)
 
-static int kunit_run_all_tests(void)
+int kunit_run_all_tests(void)
 {
 	struct kunit_suite * const * const *suites;
 
@@ -23,6 +23,4 @@ static int kunit_run_all_tests(void)
 	return 0;
 }
 
-late_initcall(kunit_run_all_tests);
-
 #endif /* IS_BUILTIN(CONFIG_KUNIT) */
-- 
2.28.0.163.g6104cc2f0b6-goog

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v6 4/5] kunit: test: add test plan to KUnit TAP format
  2020-08-04 20:47 [PATCH v6 0/5] kunit: create a centralized executor to dispatch all KUnit tests Brendan Higgins
                   ` (3 preceding siblings ...)
  2020-08-04 20:47 ` [PATCH v6 3/5] init: main: add KUnit to kernel init Brendan Higgins
@ 2020-08-04 20:47 ` Brendan Higgins
  2020-08-04 20:47   ` Brendan Higgins
  2020-08-04 20:47 ` [PATCH v6 5/5] Documentation: kunit: add a brief blurb about kunit_test_suite Brendan Higgins
  5 siblings, 1 reply; 11+ messages in thread
From: Brendan Higgins @ 2020-08-04 20:47 UTC (permalink / raw)
  To: jdike, richard, anton.ivanov, arnd, keescook, skhan,
	alan.maguire, yzaikin, davidgow, akpm, rppt, frowand.list
  Cc: gregkh, sboyd, logang, mcgrof, linux-arch, linux-kselftest,
	kunit-dev, linux-kernel, linux-doc, Brendan Higgins

TAP 14 allows an optional test plan to be emitted before the start of
the start of testing[1]; this is valuable because it makes it possible
for a test harness to detect whether the number of tests run matches the
number of tests expected to be run, ensuring that no tests silently
failed.

Link[1]: https://github.com/isaacs/testanything.github.io/blob/tap14/tap-version-14-specification.md#the-plan
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
---
 lib/kunit/executor.c                          |  17 ++++
 lib/kunit/test.c                              |  11 ---
 tools/testing/kunit/kunit_parser.py           |  76 ++++++++++++++----
 .../test_is_test_passed-all_passed.log        | Bin 1562 -> 1567 bytes
 .../test_data/test_is_test_passed-crash.log   | Bin 3016 -> 3021 bytes
 .../test_data/test_is_test_passed-failure.log | Bin 1700 -> 1705 bytes
 6 files changed, 79 insertions(+), 25 deletions(-)

diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index 4aab7f70a88c3..a95742a4ece73 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c
@@ -11,10 +11,27 @@ extern struct kunit_suite * const * const __kunit_suites_end[];
 
 #if IS_BUILTIN(CONFIG_KUNIT)
 
+static void kunit_print_tap_header(void)
+{
+	struct kunit_suite * const * const *suites, * const *subsuite;
+	int num_of_suites = 0;
+
+	for (suites = __kunit_suites_start;
+	     suites < __kunit_suites_end;
+	     suites++)
+		for (subsuite = *suites; *subsuite != NULL; subsuite++)
+			num_of_suites++;
+
+	pr_info("TAP version 14\n");
+	pr_info("1..%d\n", num_of_suites);
+}
+
 int kunit_run_all_tests(void)
 {
 	struct kunit_suite * const * const *suites;
 
+	kunit_print_tap_header();
+
 	for (suites = __kunit_suites_start;
 	     suites < __kunit_suites_end;
 	     suites++)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 918dff400a9d7..b1835ccb3fce2 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -19,16 +19,6 @@ static void kunit_set_failure(struct kunit *test)
 	WRITE_ONCE(test->success, false);
 }
 
-static void kunit_print_tap_version(void)
-{
-	static bool kunit_has_printed_tap_version;
-
-	if (!kunit_has_printed_tap_version) {
-		pr_info("TAP version 14\n");
-		kunit_has_printed_tap_version = true;
-	}
-}
-
 /*
  * Append formatted message to log, size of which is limited to
  * KUNIT_LOG_SIZE bytes (including null terminating byte).
@@ -68,7 +58,6 @@ EXPORT_SYMBOL_GPL(kunit_suite_num_test_cases);
 
 static void kunit_print_subtest_start(struct kunit_suite *suite)
 {
-	kunit_print_tap_version();
 	kunit_log(KERN_INFO, suite, KUNIT_SUBTEST_INDENT "# Subtest: %s",
 		  suite->name);
 	kunit_log(KERN_INFO, suite, KUNIT_SUBTEST_INDENT "1..%zd",
diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
index f13e0c0d66639..8019e3dd4c32f 100644
--- a/tools/testing/kunit/kunit_parser.py
+++ b/tools/testing/kunit/kunit_parser.py
@@ -45,10 +45,11 @@ class TestStatus(Enum):
 	FAILURE = auto()
 	TEST_CRASHED = auto()
 	NO_TESTS = auto()
+	FAILURE_TO_PARSE_TESTS = auto()
 
 kunit_start_re = re.compile(r'TAP version [0-9]+$')
 kunit_end_re = re.compile('(List of all partitions:|'
-			  'Kernel panic - not syncing: VFS:|reboot: System halted)')
+			  'Kernel panic - not syncing: VFS:)')
 
 def isolate_kunit_output(kernel_output):
 	started = False
@@ -109,7 +110,7 @@ OkNotOkResult = namedtuple('OkNotOkResult', ['is_ok','description', 'text'])
 
 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]+ - (.*)$')
+OK_NOT_OK_MODULE = re.compile(r'^(ok|not ok) ([0-9]+) - (.*)$')
 
 def parse_ok_not_ok_test_case(lines: List[str], test_case: TestCase) -> bool:
 	save_non_diagnositic(lines, test_case)
@@ -197,7 +198,9 @@ def max_status(left: TestStatus, right: TestStatus) -> TestStatus:
 	else:
 		return TestStatus.SUCCESS
 
-def parse_ok_not_ok_test_suite(lines: List[str], test_suite: TestSuite) -> bool:
+def parse_ok_not_ok_test_suite(lines: List[str],
+			       test_suite: TestSuite,
+			       expected_suite_index: int) -> bool:
 	consume_non_diagnositic(lines)
 	if not lines:
 		test_suite.status = TestStatus.TEST_CRASHED
@@ -210,6 +213,12 @@ def parse_ok_not_ok_test_suite(lines: List[str], test_suite: TestSuite) -> bool:
 			test_suite.status = TestStatus.SUCCESS
 		else:
 			test_suite.status = TestStatus.FAILURE
+		suite_index = int(match.group(2))
+		if suite_index != expected_suite_index:
+			print_with_timestamp(
+				red('[ERROR] ') + 'expected_suite_index ' +
+				str(expected_suite_index) + ', but got ' +
+				str(suite_index))
 		return True
 	else:
 		return False
@@ -222,7 +231,7 @@ def bubble_up_test_case_errors(test_suite: TestSuite) -> TestStatus:
 	max_test_case_status = bubble_up_errors(lambda x: x.status, test_suite.cases)
 	return max_status(max_test_case_status, test_suite.status)
 
-def parse_test_suite(lines: List[str]) -> TestSuite:
+def parse_test_suite(lines: List[str], expected_suite_index: int) -> TestSuite:
 	if not lines:
 		return None
 	consume_non_diagnositic(lines)
@@ -241,7 +250,7 @@ def parse_test_suite(lines: List[str]) -> TestSuite:
 			break
 		test_suite.cases.append(test_case)
 		expected_test_case_num -= 1
-	if parse_ok_not_ok_test_suite(lines, test_suite):
+	if parse_ok_not_ok_test_suite(lines, test_suite, expected_suite_index):
 		test_suite.status = bubble_up_test_case_errors(test_suite)
 		return test_suite
 	elif not lines:
@@ -261,6 +270,17 @@ def parse_tap_header(lines: List[str]) -> bool:
 	else:
 		return False
 
+TEST_PLAN = re.compile(r'[0-9]+\.\.([0-9]+)')
+
+def parse_test_plan(lines: List[str]) -> int:
+	consume_non_diagnositic(lines)
+	match = TEST_PLAN.match(lines[0])
+	if match:
+		lines.pop(0)
+		return int(match.group(1))
+	else:
+		return None
+
 def bubble_up_suite_errors(test_suite_list: List[TestSuite]) -> TestStatus:
 	return bubble_up_errors(lambda x: x.status, test_suite_list)
 
@@ -268,20 +288,33 @@ def parse_test_result(lines: List[str]) -> TestResult:
 	consume_non_diagnositic(lines)
 	if not lines or not parse_tap_header(lines):
 		return TestResult(TestStatus.NO_TESTS, [], lines)
+	expected_test_suite_num = parse_test_plan(lines)
+	if not expected_test_suite_num:
+		return TestResult(TestStatus.FAILURE_TO_PARSE_TESTS, [], lines)
 	test_suites = []
-	test_suite = parse_test_suite(lines)
-	while test_suite:
-		test_suites.append(test_suite)
-		test_suite = parse_test_suite(lines)
-	return TestResult(bubble_up_suite_errors(test_suites), test_suites, lines)
+	for i in range(1, expected_test_suite_num + 1):
+		test_suite = parse_test_suite(lines, i)
+		if test_suite:
+			test_suites.append(test_suite)
+		else:
+			print_with_timestamp(
+				red('[ERROR] ') + ' expected ' +
+				str(expected_test_suite_num) +
+				' test suites, but got ' + str(i - 2))
+			break
+	test_suite = parse_test_suite(lines, -1)
+	if test_suite:
+		print_with_timestamp(red('[ERROR] ') +
+			'got unexpected test suite: ' + test_suite.name)
+	if test_suites:
+		return TestResult(bubble_up_suite_errors(test_suites), test_suites, lines)
+	else:
+		return TestResult(TestStatus.NO_TESTS, [], lines)
 
-def parse_run_tests(kernel_output) -> TestResult:
+def print_and_count_results(test_result: TestResult) -> None:
 	total_tests = 0
 	failed_tests = 0
 	crashed_tests = 0
-	test_result = parse_test_result(list(isolate_kunit_output(kernel_output)))
-	if test_result.status == TestStatus.NO_TESTS:
-		print_with_timestamp(red('[ERROR] ') + 'no kunit output detected')
 	for test_suite in test_result.suites:
 		if test_suite.status == TestStatus.SUCCESS:
 			print_suite_divider(green('[PASSED] ') + test_suite.name)
@@ -303,6 +336,21 @@ def parse_run_tests(kernel_output) -> TestResult:
 				print_with_timestamp(red('[FAILED] ') + test_case.name)
 				print_log(map(yellow, test_case.log))
 				print_with_timestamp('')
+	return total_tests, failed_tests, crashed_tests
+
+def parse_run_tests(kernel_output) -> TestResult:
+	total_tests = 0
+	failed_tests = 0
+	crashed_tests = 0
+	test_result = parse_test_result(list(isolate_kunit_output(kernel_output)))
+	if test_result.status == TestStatus.NO_TESTS:
+		print(red('[ERROR] ') + yellow('no tests run!'))
+	elif test_result.status == TestStatus.FAILURE_TO_PARSE_TESTS:
+		print(red('[ERROR] ') + yellow('could not parse test results!'))
+	else:
+		(total_tests,
+		 failed_tests,
+		 crashed_tests) = print_and_count_results(test_result)
 	print_with_timestamp(DIVIDER)
 	fmt = green if test_result.status == TestStatus.SUCCESS else red
 	print_with_timestamp(
diff --git a/tools/testing/kunit/test_data/test_is_test_passed-all_passed.log b/tools/testing/kunit/test_data/test_is_test_passed-all_passed.log
index 62ebc0288355c4b122ccc18ae2505f971efa57bc..bc0dc8fe35b760b1feb74ec419818dbfae1adb5c 100644
GIT binary patch
delta 28
jcmbQmGoME|#4$jjEVZaOGe1wk(1goSPtRy09}gP<dC~`u

delta 23
ecmbQwGmD2W#4$jjEVZaOGe1wk&}5@94;uhhkp{*9

diff --git a/tools/testing/kunit/test_data/test_is_test_passed-crash.log b/tools/testing/kunit/test_data/test_is_test_passed-crash.log
index 0b249870c8be417a5865bd40a24c8597bb7f5ab1..4d97f6708c4a5ad5bb2ac879e12afca6e816d83d 100644
GIT binary patch
delta 15
WcmX>hepY;fFN>j`p3z318g2k9Uj*m?

delta 10
RcmX>renNbL@5Z2NZU7lr1S$Xk

diff --git a/tools/testing/kunit/test_data/test_is_test_passed-failure.log b/tools/testing/kunit/test_data/test_is_test_passed-failure.log
index 9e89d32d5667a59d137f8adacf3a88fdb7f88baf..7a416497e3bec044eefc1535f7d84ee85703ba97 100644
GIT binary patch
delta 28
jcmZ3&yOLKp#4$jjEVZaOGe1wk(1goSPtRy0-!wJ=eKrU$

delta 23
ecmZ3<yM&i7#4$jjEVZaOGe1wk&}5_VG&TTPhX-Z=

-- 
2.28.0.163.g6104cc2f0b6-goog

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v6 4/5] kunit: test: add test plan to KUnit TAP format
  2020-08-04 20:47 ` [PATCH v6 4/5] kunit: test: add test plan to KUnit TAP format Brendan Higgins
@ 2020-08-04 20:47   ` Brendan Higgins
  0 siblings, 0 replies; 11+ messages in thread
From: Brendan Higgins @ 2020-08-04 20:47 UTC (permalink / raw)
  To: jdike, richard, anton.ivanov, arnd, keescook, skhan,
	alan.maguire, yzaikin, davidgow, akpm, rppt, frowand.list
  Cc: gregkh, sboyd, logang, mcgrof, linux-arch, linux-kselftest,
	kunit-dev, linux-kernel, linux-doc, Brendan Higgins

TAP 14 allows an optional test plan to be emitted before the start of
the start of testing[1]; this is valuable because it makes it possible
for a test harness to detect whether the number of tests run matches the
number of tests expected to be run, ensuring that no tests silently
failed.

Link[1]: https://github.com/isaacs/testanything.github.io/blob/tap14/tap-version-14-specification.md#the-plan
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
---
 lib/kunit/executor.c                          |  17 ++++
 lib/kunit/test.c                              |  11 ---
 tools/testing/kunit/kunit_parser.py           |  76 ++++++++++++++----
 .../test_is_test_passed-all_passed.log        | Bin 1562 -> 1567 bytes
 .../test_data/test_is_test_passed-crash.log   | Bin 3016 -> 3021 bytes
 .../test_data/test_is_test_passed-failure.log | Bin 1700 -> 1705 bytes
 6 files changed, 79 insertions(+), 25 deletions(-)

diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index 4aab7f70a88c3..a95742a4ece73 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c
@@ -11,10 +11,27 @@ extern struct kunit_suite * const * const __kunit_suites_end[];
 
 #if IS_BUILTIN(CONFIG_KUNIT)
 
+static void kunit_print_tap_header(void)
+{
+	struct kunit_suite * const * const *suites, * const *subsuite;
+	int num_of_suites = 0;
+
+	for (suites = __kunit_suites_start;
+	     suites < __kunit_suites_end;
+	     suites++)
+		for (subsuite = *suites; *subsuite != NULL; subsuite++)
+			num_of_suites++;
+
+	pr_info("TAP version 14\n");
+	pr_info("1..%d\n", num_of_suites);
+}
+
 int kunit_run_all_tests(void)
 {
 	struct kunit_suite * const * const *suites;
 
+	kunit_print_tap_header();
+
 	for (suites = __kunit_suites_start;
 	     suites < __kunit_suites_end;
 	     suites++)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 918dff400a9d7..b1835ccb3fce2 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -19,16 +19,6 @@ static void kunit_set_failure(struct kunit *test)
 	WRITE_ONCE(test->success, false);
 }
 
-static void kunit_print_tap_version(void)
-{
-	static bool kunit_has_printed_tap_version;
-
-	if (!kunit_has_printed_tap_version) {
-		pr_info("TAP version 14\n");
-		kunit_has_printed_tap_version = true;
-	}
-}
-
 /*
  * Append formatted message to log, size of which is limited to
  * KUNIT_LOG_SIZE bytes (including null terminating byte).
@@ -68,7 +58,6 @@ EXPORT_SYMBOL_GPL(kunit_suite_num_test_cases);
 
 static void kunit_print_subtest_start(struct kunit_suite *suite)
 {
-	kunit_print_tap_version();
 	kunit_log(KERN_INFO, suite, KUNIT_SUBTEST_INDENT "# Subtest: %s",
 		  suite->name);
 	kunit_log(KERN_INFO, suite, KUNIT_SUBTEST_INDENT "1..%zd",
diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
index f13e0c0d66639..8019e3dd4c32f 100644
--- a/tools/testing/kunit/kunit_parser.py
+++ b/tools/testing/kunit/kunit_parser.py
@@ -45,10 +45,11 @@ class TestStatus(Enum):
 	FAILURE = auto()
 	TEST_CRASHED = auto()
 	NO_TESTS = auto()
+	FAILURE_TO_PARSE_TESTS = auto()
 
 kunit_start_re = re.compile(r'TAP version [0-9]+$')
 kunit_end_re = re.compile('(List of all partitions:|'
-			  'Kernel panic - not syncing: VFS:|reboot: System halted)')
+			  'Kernel panic - not syncing: VFS:)')
 
 def isolate_kunit_output(kernel_output):
 	started = False
@@ -109,7 +110,7 @@ OkNotOkResult = namedtuple('OkNotOkResult', ['is_ok','description', 'text'])
 
 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]+ - (.*)$')
+OK_NOT_OK_MODULE = re.compile(r'^(ok|not ok) ([0-9]+) - (.*)$')
 
 def parse_ok_not_ok_test_case(lines: List[str], test_case: TestCase) -> bool:
 	save_non_diagnositic(lines, test_case)
@@ -197,7 +198,9 @@ def max_status(left: TestStatus, right: TestStatus) -> TestStatus:
 	else:
 		return TestStatus.SUCCESS
 
-def parse_ok_not_ok_test_suite(lines: List[str], test_suite: TestSuite) -> bool:
+def parse_ok_not_ok_test_suite(lines: List[str],
+			       test_suite: TestSuite,
+			       expected_suite_index: int) -> bool:
 	consume_non_diagnositic(lines)
 	if not lines:
 		test_suite.status = TestStatus.TEST_CRASHED
@@ -210,6 +213,12 @@ def parse_ok_not_ok_test_suite(lines: List[str], test_suite: TestSuite) -> bool:
 			test_suite.status = TestStatus.SUCCESS
 		else:
 			test_suite.status = TestStatus.FAILURE
+		suite_index = int(match.group(2))
+		if suite_index != expected_suite_index:
+			print_with_timestamp(
+				red('[ERROR] ') + 'expected_suite_index ' +
+				str(expected_suite_index) + ', but got ' +
+				str(suite_index))
 		return True
 	else:
 		return False
@@ -222,7 +231,7 @@ def bubble_up_test_case_errors(test_suite: TestSuite) -> TestStatus:
 	max_test_case_status = bubble_up_errors(lambda x: x.status, test_suite.cases)
 	return max_status(max_test_case_status, test_suite.status)
 
-def parse_test_suite(lines: List[str]) -> TestSuite:
+def parse_test_suite(lines: List[str], expected_suite_index: int) -> TestSuite:
 	if not lines:
 		return None
 	consume_non_diagnositic(lines)
@@ -241,7 +250,7 @@ def parse_test_suite(lines: List[str]) -> TestSuite:
 			break
 		test_suite.cases.append(test_case)
 		expected_test_case_num -= 1
-	if parse_ok_not_ok_test_suite(lines, test_suite):
+	if parse_ok_not_ok_test_suite(lines, test_suite, expected_suite_index):
 		test_suite.status = bubble_up_test_case_errors(test_suite)
 		return test_suite
 	elif not lines:
@@ -261,6 +270,17 @@ def parse_tap_header(lines: List[str]) -> bool:
 	else:
 		return False
 
+TEST_PLAN = re.compile(r'[0-9]+\.\.([0-9]+)')
+
+def parse_test_plan(lines: List[str]) -> int:
+	consume_non_diagnositic(lines)
+	match = TEST_PLAN.match(lines[0])
+	if match:
+		lines.pop(0)
+		return int(match.group(1))
+	else:
+		return None
+
 def bubble_up_suite_errors(test_suite_list: List[TestSuite]) -> TestStatus:
 	return bubble_up_errors(lambda x: x.status, test_suite_list)
 
@@ -268,20 +288,33 @@ def parse_test_result(lines: List[str]) -> TestResult:
 	consume_non_diagnositic(lines)
 	if not lines or not parse_tap_header(lines):
 		return TestResult(TestStatus.NO_TESTS, [], lines)
+	expected_test_suite_num = parse_test_plan(lines)
+	if not expected_test_suite_num:
+		return TestResult(TestStatus.FAILURE_TO_PARSE_TESTS, [], lines)
 	test_suites = []
-	test_suite = parse_test_suite(lines)
-	while test_suite:
-		test_suites.append(test_suite)
-		test_suite = parse_test_suite(lines)
-	return TestResult(bubble_up_suite_errors(test_suites), test_suites, lines)
+	for i in range(1, expected_test_suite_num + 1):
+		test_suite = parse_test_suite(lines, i)
+		if test_suite:
+			test_suites.append(test_suite)
+		else:
+			print_with_timestamp(
+				red('[ERROR] ') + ' expected ' +
+				str(expected_test_suite_num) +
+				' test suites, but got ' + str(i - 2))
+			break
+	test_suite = parse_test_suite(lines, -1)
+	if test_suite:
+		print_with_timestamp(red('[ERROR] ') +
+			'got unexpected test suite: ' + test_suite.name)
+	if test_suites:
+		return TestResult(bubble_up_suite_errors(test_suites), test_suites, lines)
+	else:
+		return TestResult(TestStatus.NO_TESTS, [], lines)
 
-def parse_run_tests(kernel_output) -> TestResult:
+def print_and_count_results(test_result: TestResult) -> None:
 	total_tests = 0
 	failed_tests = 0
 	crashed_tests = 0
-	test_result = parse_test_result(list(isolate_kunit_output(kernel_output)))
-	if test_result.status == TestStatus.NO_TESTS:
-		print_with_timestamp(red('[ERROR] ') + 'no kunit output detected')
 	for test_suite in test_result.suites:
 		if test_suite.status == TestStatus.SUCCESS:
 			print_suite_divider(green('[PASSED] ') + test_suite.name)
@@ -303,6 +336,21 @@ def parse_run_tests(kernel_output) -> TestResult:
 				print_with_timestamp(red('[FAILED] ') + test_case.name)
 				print_log(map(yellow, test_case.log))
 				print_with_timestamp('')
+	return total_tests, failed_tests, crashed_tests
+
+def parse_run_tests(kernel_output) -> TestResult:
+	total_tests = 0
+	failed_tests = 0
+	crashed_tests = 0
+	test_result = parse_test_result(list(isolate_kunit_output(kernel_output)))
+	if test_result.status == TestStatus.NO_TESTS:
+		print(red('[ERROR] ') + yellow('no tests run!'))
+	elif test_result.status == TestStatus.FAILURE_TO_PARSE_TESTS:
+		print(red('[ERROR] ') + yellow('could not parse test results!'))
+	else:
+		(total_tests,
+		 failed_tests,
+		 crashed_tests) = print_and_count_results(test_result)
 	print_with_timestamp(DIVIDER)
 	fmt = green if test_result.status == TestStatus.SUCCESS else red
 	print_with_timestamp(
diff --git a/tools/testing/kunit/test_data/test_is_test_passed-all_passed.log b/tools/testing/kunit/test_data/test_is_test_passed-all_passed.log
index 62ebc0288355c4b122ccc18ae2505f971efa57bc..bc0dc8fe35b760b1feb74ec419818dbfae1adb5c 100644
GIT binary patch
delta 28
jcmbQmGoME|#4$jjEVZaOGe1wk(1goSPtRy09}gP<dC~`u

delta 23
ecmbQwGmD2W#4$jjEVZaOGe1wk&}5@94;uhhkp{*9

diff --git a/tools/testing/kunit/test_data/test_is_test_passed-crash.log b/tools/testing/kunit/test_data/test_is_test_passed-crash.log
index 0b249870c8be417a5865bd40a24c8597bb7f5ab1..4d97f6708c4a5ad5bb2ac879e12afca6e816d83d 100644
GIT binary patch
delta 15
WcmX>hepY;fFN>j`p3z318g2k9Uj*m?

delta 10
RcmX>renNbL@5Z2NZU7lr1S$Xk

diff --git a/tools/testing/kunit/test_data/test_is_test_passed-failure.log b/tools/testing/kunit/test_data/test_is_test_passed-failure.log
index 9e89d32d5667a59d137f8adacf3a88fdb7f88baf..7a416497e3bec044eefc1535f7d84ee85703ba97 100644
GIT binary patch
delta 28
jcmZ3&yOLKp#4$jjEVZaOGe1wk(1goSPtRy0-!wJ=eKrU$

delta 23
ecmZ3<yM&i7#4$jjEVZaOGe1wk&}5_VG&TTPhX-Z=

-- 
2.28.0.163.g6104cc2f0b6-goog

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v6 5/5] Documentation: kunit: add a brief blurb about kunit_test_suite
  2020-08-04 20:47 [PATCH v6 0/5] kunit: create a centralized executor to dispatch all KUnit tests Brendan Higgins
                   ` (4 preceding siblings ...)
  2020-08-04 20:47 ` [PATCH v6 4/5] kunit: test: add test plan to KUnit TAP format Brendan Higgins
@ 2020-08-04 20:47 ` Brendan Higgins
  2020-08-05  6:01   ` Stephen Boyd
  5 siblings, 1 reply; 11+ messages in thread
From: Brendan Higgins @ 2020-08-04 20:47 UTC (permalink / raw)
  To: jdike, richard, anton.ivanov, arnd, keescook, skhan,
	alan.maguire, yzaikin, davidgow, akpm, rppt, frowand.list
  Cc: gregkh, sboyd, logang, mcgrof, linux-arch, linux-kselftest,
	kunit-dev, linux-kernel, linux-doc, Brendan Higgins

Add a brief blurb saying how and when the kunit_test_suite() macro
works to the usage documentation.

Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
---
 Documentation/dev-tools/kunit/usage.rst | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
index 3c3fe8b5feccf..961d3ea3ca19a 100644
--- a/Documentation/dev-tools/kunit/usage.rst
+++ b/Documentation/dev-tools/kunit/usage.rst
@@ -211,6 +211,11 @@ KUnit test framework.
 .. note::
    A test case will only be run if it is associated with a test suite.
 
+``kunit_test_suite(...)`` is a macro which tells the linker to put the specified
+test suite in a special linker section so that it can be run by KUnit either
+after late_init, or when the test module is loaded (depending on whether the
+test was built in or not).
+
 For more information on these types of things see the :doc:`api/test`.
 
 Isolating Behavior
-- 
2.28.0.163.g6104cc2f0b6-goog

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v6 5/5] Documentation: kunit: add a brief blurb about kunit_test_suite
  2020-08-04 20:47 ` [PATCH v6 5/5] Documentation: kunit: add a brief blurb about kunit_test_suite Brendan Higgins
@ 2020-08-05  6:01   ` Stephen Boyd
  2020-08-05  6:01     ` Stephen Boyd
  0 siblings, 1 reply; 11+ messages in thread
From: Stephen Boyd @ 2020-08-05  6:01 UTC (permalink / raw)
  To: akpm, alan.maguire, anton.ivanov, arnd, davidgow, frowand.list,
	jdike, keescook, richard, rppt, skhan, yzaikin
  Cc: gregkh, logang, mcgrof, linux-arch, linux-kselftest, kunit-dev,
	linux-kernel, linux-doc, Brendan Higgins

Quoting Brendan Higgins (2020-08-04 13:47:45)
> Add a brief blurb saying how and when the kunit_test_suite() macro
> works to the usage documentation.
> 
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---
>  Documentation/dev-tools/kunit/usage.rst | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
> index 3c3fe8b5feccf..961d3ea3ca19a 100644
> --- a/Documentation/dev-tools/kunit/usage.rst
> +++ b/Documentation/dev-tools/kunit/usage.rst
> @@ -211,6 +211,11 @@ KUnit test framework.
>  .. note::
>     A test case will only be run if it is associated with a test suite.
>  
> +``kunit_test_suite(...)`` is a macro which tells the linker to put the specified
> +test suite in a special linker section so that it can be run by KUnit either
> +after late_init, or when the test module is loaded (depending on whether the

"late_init" probably needs some italics or something, but otherwise.

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v6 5/5] Documentation: kunit: add a brief blurb about kunit_test_suite
  2020-08-05  6:01   ` Stephen Boyd
@ 2020-08-05  6:01     ` Stephen Boyd
  0 siblings, 0 replies; 11+ messages in thread
From: Stephen Boyd @ 2020-08-05  6:01 UTC (permalink / raw)
  To: akpm, alan.maguire, anton.ivanov, arnd, davidgow, frowand.list,
	jdike, keescook, richard, rppt, skhan, yzaikin
  Cc: gregkh, logang, mcgrof, linux-arch, linux-kselftest, kunit-dev,
	linux-kernel, linux-doc, Brendan Higgins

Quoting Brendan Higgins (2020-08-04 13:47:45)
> Add a brief blurb saying how and when the kunit_test_suite() macro
> works to the usage documentation.
> 
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> ---
>  Documentation/dev-tools/kunit/usage.rst | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
> index 3c3fe8b5feccf..961d3ea3ca19a 100644
> --- a/Documentation/dev-tools/kunit/usage.rst
> +++ b/Documentation/dev-tools/kunit/usage.rst
> @@ -211,6 +211,11 @@ KUnit test framework.
>  .. note::
>     A test case will only be run if it is associated with a test suite.
>  
> +``kunit_test_suite(...)`` is a macro which tells the linker to put the specified
> +test suite in a special linker section so that it can be run by KUnit either
> +after late_init, or when the test module is loaded (depending on whether the

"late_init" probably needs some italics or something, but otherwise.

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v6 2/5] kunit: test: create a single centralized executor for all tests
  2020-08-04 20:47 ` [PATCH v6 2/5] kunit: test: create a single centralized executor for all tests Brendan Higgins
@ 2020-10-13 11:09   ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 11+ messages in thread
From: Mauro Carvalho Chehab @ 2020-10-13 11:09 UTC (permalink / raw)
  To: Brendan Higgins, skhan
  Cc: jdike, richard, anton.ivanov, arnd, keescook, alan.maguire,
	yzaikin, davidgow, akpm, rppt, frowand.list, gregkh, sboyd,
	logang, mcgrof, linux-arch, linux-kselftest, kunit-dev,
	linux-kernel, linux-doc

Em Tue,  4 Aug 2020 13:47:42 -0700
Brendan Higgins <brendanhiggins@google.com> escreveu:

> From: Alan Maguire <alan.maguire@oracle.com>
> 
> Add a centralized executor to dispatch tests rather than relying on
> late_initcall to schedule each test suite separately. Centralized
> execution is for built-in tests only; modules will execute tests when
> loaded.

This patch adds 3 new warnings when doing "make htmldocs".
Those two are trivial to fix:

	../include/kunit/test.h:282: warning: Function parameter or member '__suites' not described in 'kunit_test_suites_for_module'
	../include/kunit/test.h:282: warning: Excess function parameter 'suites_list' description in 'kunit_test_suites_for_module'

<patch>
diff --git a/include/kunit/test.h b/include/kunit/test.h
index a423fffefea0..f2c387c60fac 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -256,9 +256,9 @@ static inline int kunit_run_all_tests(void)
  * kunit_test_suites() - used to register one or more &struct kunit_suite
  *			 with KUnit.
  *
- * @suites_list...: a statically allocated list of &struct kunit_suite.
+ * @__suites: a statically allocated list of &struct kunit_suite.
  *
- * Registers @suites_list with the test framework. See &struct kunit_suite for
+ * Registers @__suites with the test framework. See &struct kunit_suite for
  * more information.
  *
  * If a test suite is built-in, module_init() gets translated into
</patch>

> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> Co-developed-by: Iurii Zaikin <yzaikin@google.com>
> Signed-off-by: Iurii Zaikin <yzaikin@google.com>
> Co-developed-by: Brendan Higgins <brendanhiggins@google.com>
> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> ---
>  include/kunit/test.h | 67 +++++++++++++++++++++++++++++---------------
>  lib/kunit/Makefile   |  3 +-
>  lib/kunit/executor.c | 28 ++++++++++++++++++
>  lib/kunit/test.c     |  2 +-
>  4 files changed, 76 insertions(+), 24 deletions(-)
>  create mode 100644 lib/kunit/executor.c
> 
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 47e61e1d53370..f3e86c3953a2b 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -224,7 +224,7 @@ size_t kunit_suite_num_test_cases(struct kunit_suite *suite);
>  unsigned int kunit_test_case_num(struct kunit_suite *suite,
>  				 struct kunit_case *test_case);
>  
> -int __kunit_test_suites_init(struct kunit_suite **suites);
> +int __kunit_test_suites_init(struct kunit_suite * const * const suites);
>  
>  void __kunit_test_suites_exit(struct kunit_suite **suites);
>  
> @@ -237,34 +237,57 @@ void __kunit_test_suites_exit(struct kunit_suite **suites);
>   * Registers @suites_list with the test framework. See &struct kunit_suite for
>   * more information.
>   *
> - * 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@google.com): 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.
> + * If a test suite is built-in, module_init() gets translated into
> + * an initcall which we don't want as the idea is that for builtins
> + * the executor will manage execution.  So ensure we do not define
> + * module_{init|exit} functions for the builtin case when registering
> + * suites via kunit_test_suites() below.
>   */
> -#define kunit_test_suites(suites_list...)				\
> -	static struct kunit_suite *suites[] = {suites_list, NULL};	\
> -	static int kunit_test_suites_init(void)				\
> +#ifdef MODULE
> +#define kunit_test_suites_for_module(__suites)				\
> +	static int __init kunit_test_suites_init(void)			\
>  	{								\
> -		return __kunit_test_suites_init(suites);		\
> +		return __kunit_test_suites_init(__suites);		\
>  	}								\
> -	late_initcall(kunit_test_suites_init);				\
> +	module_init(kunit_test_suites_init);				\
> +									\
>  	static void __exit kunit_test_suites_exit(void)			\
>  	{								\
> -		return __kunit_test_suites_exit(suites);		\
> +		return __kunit_test_suites_exit(__suites);		\
>  	}								\
>  	module_exit(kunit_test_suites_exit)
> +#else
> +#define kunit_test_suites_for_module(__suites)
> +#endif /* MODULE */
> +
> +#define __kunit_test_suites(unique_array, unique_suites, ...)		       \
> +	static struct kunit_suite *unique_array[] = { __VA_ARGS__, NULL };     \
> +	kunit_test_suites_for_module(unique_array);			       \
> +	static struct kunit_suite **unique_suites			       \
> +	__used __section(.kunit_test_suites) = unique_array
> +
> +/**
> + * kunit_test_suites() - used to register one or more &struct kunit_suite
> + *			 with KUnit.
> + *
> + * @suites: a statically allocated list of &struct kunit_suite.
> + *
> + * Registers @suites with the test framework. See &struct kunit_suite for
> + * more information.
> + *
> + * When builtin,  KUnit tests are all run via executor; this is done
> + * by placing the array of struct kunit_suite * in the .kunit_test_suites
> + * ELF section.
> + *
> + * An alternative is to build the tests as a module.  Because modules do not
> + * support multiple initcall()s, we need to initialize an array of suites for a
> + * module.
> + *
> + */
> +#define kunit_test_suites(...)						\
> +	__kunit_test_suites(__UNIQUE_ID(array),				\
> +			    __UNIQUE_ID(suites),			\
> +			    __VA_ARGS__)

But the third warning:

	../include/kunit/test.h:314: warning: Excess function parameter 'suites' description in 'kunit_test_suites'

is more problematic.

At least for me, it sounds a **very bad** idea to not pass "array"
and "suites" to this macro, but, instead, to rely that some
previous code would have added such vars with those specific
names.

Also, you can't document a parameter "suites" that it is not a
parameter.

IMO, the right fix would be to define the macro as:

	#define kunit_test_suites(array, suites, arg...)			\
		__kunit_test_suites(__UNIQUE_ID(array),			\
				    __UNIQUE_ID(suites),		\
				    ## arg)

and then document "array", "suites" and "arg" using kernel-doc
markups.

Thanks,
Mauro

^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-10-13 11:09 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-04 20:47 [PATCH v6 0/5] kunit: create a centralized executor to dispatch all KUnit tests Brendan Higgins
2020-08-04 20:47 ` Brendan Higgins
2020-08-04 20:47 ` [PATCH v6 1/5] vmlinux.lds.h: add linker section for KUnit test suites Brendan Higgins
2020-08-04 20:47 ` [PATCH v6 2/5] kunit: test: create a single centralized executor for all tests Brendan Higgins
2020-10-13 11:09   ` Mauro Carvalho Chehab
2020-08-04 20:47 ` [PATCH v6 3/5] init: main: add KUnit to kernel init Brendan Higgins
2020-08-04 20:47 ` [PATCH v6 4/5] kunit: test: add test plan to KUnit TAP format Brendan Higgins
2020-08-04 20:47   ` Brendan Higgins
2020-08-04 20:47 ` [PATCH v6 5/5] Documentation: kunit: add a brief blurb about kunit_test_suite Brendan Higgins
2020-08-05  6:01   ` Stephen Boyd
2020-08-05  6:01     ` Stephen Boyd

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).