[PATCH v8 kunit-next 0/4] kunit: add debugfs representation to show results

[PATCH v8 kunit-next 0/4] kunit: add debugfs representation to show results

[Alan Maguire]

When kunit tests are run on native (i.e. non-UML) environments, the results
of test execution are often intermixed with dmesg output.  This patch
series attempts to solve this by providing a debugfs representation
of the results of the last test run, available as

/sys/kernel/debug/kunit/<testsuite>/results

Changes since v7:

- renamed KUNIT_INDENT[2] to KUNIT_SUBTEST_INDENT, KUNIT_SUBSUBTEST_INDENT
  and added more description to their definitions to clarify why they
  are defined as they are (Shuah)
- defined KUNIT_SUBSUBTEST_INDENT directly as 8 spaces to avoid
  checkpatch error (Shuah)

Changes since v6:

- fixed regexp parsing in kunit_parser.py to ensure test results are read
  successfully with 4-space indentation (Brendan, patch 3)

Changes since v5:

- replaced undefined behaviour use of snprintf(buf, ..., buf) in
  kunit_log() with a function to append string to existing log
  (Frank, patch 1)
- added clarification on log size limitations to documentation
  (Frank, patch 4)

Changes since v4:

- added suite-level log expectations to kunit log test (Brendan, patch 2)
- added log expectations (of it being NULL) for case where
  CONFIG_KUNIT_DEBUGFS=n to kunit log test (patch 2)
- added patch 3 which replaces subtest tab indentation with 4 space
  indentation as per TAP 14 spec (Frank, patch 3)

Changes since v3:

- added CONFIG_KUNIT_DEBUGFS to support conditional compilation of debugfs
  representation, including string logging (Frank, patch 1)
- removed unneeded NULL check for test_case in
  kunit_suite_for_each_test_case() (Frank, patch 1)
- added kunit log test to verify logging multiple strings works
  (Frank, patch 2)
- rephrased description of results file (Frank, patch 3)

Changes since v2:

- updated kunit_status2str() to kunit_status_to_string() and made it
  static inline in include/kunit/test.h (Brendan)
- added log string to struct kunit_suite and kunit_case, with log
  pointer in struct kunit pointing at the case log.  This allows us
  to collect kunit_[err|info|warning]() messages at the same time
  as we printk() them.  This solves for the most part the sharing
  of log messages between test execution and debugfs since we
  just print the suite log (which contains the test suite preamble)
  and the individual test logs.  The only exception is the suite-level
  status, which we cannot store in the suite log as it would mean
  we'd print the suite and its status prior to the suite's results.
  (Brendan, patch 1)
- dropped debugfs-based kunit run patch for now so as not to cause
  problems with tests currently under development (Brendan)
- fixed doc issues with code block (Brendan, patch 3)

Changes since v1:
 - trimmed unneeded include files in lib/kunit/debugfs.c (Greg)
 - renamed global debugfs functions to be prefixed with kunit_ (Greg)
 - removed error checking for debugfs operations (Greg)

Alan Maguire (4):
  kunit: add debugfs /sys/kernel/debug/kunit/<suite>/results display
  kunit: add log test
  kunit: subtests should be indented 4 spaces according to TAP
  kunit: update documentation to describe debugfs representation

 Documentation/dev-tools/kunit/usage.rst |  14 +++
 include/kunit/test.h                    |  63 ++++++++++++--
 lib/kunit/Kconfig                       |   8 ++
 lib/kunit/Makefile                      |   4 +
 lib/kunit/assert.c                      |  79 ++++++++---------
 lib/kunit/debugfs.c                     | 116 +++++++++++++++++++++++++
 lib/kunit/debugfs.h                     |  30 +++++++
 lib/kunit/kunit-test.c                  |  44 +++++++++-
 lib/kunit/test.c                        | 148 +++++++++++++++++++++++++-------
 tools/testing/kunit/kunit_parser.py     |  10 +--
 10 files changed, 430 insertions(+), 86 deletions(-)
 create mode 100644 lib/kunit/debugfs.c
 create mode 100644 lib/kunit/debugfs.h

-- 
1.8.3.1

[PATCH v8 kunit-next 1/4] kunit: add debugfs /sys/kernel/debug/kunit/<suite>/results display

[Alan Maguire]

add debugfs support for displaying kunit test suite results; this is
especially useful for module-loaded tests to allow disentangling of
test result display from other dmesg events.  debugfs support is
provided if CONFIG_KUNIT_DEBUGFS=y.

As well as printk()ing messages, we append them to a per-test log.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Frank Rowand <frank.rowand@sony.com>
---
 include/kunit/test.h   |  54 +++++++++++++++---
 lib/kunit/Kconfig      |   8 +++
 lib/kunit/Makefile     |   4 ++
 lib/kunit/debugfs.c    | 116 ++++++++++++++++++++++++++++++++++++++
 lib/kunit/debugfs.h    |  30 ++++++++++
 lib/kunit/kunit-test.c |   4 +-
 lib/kunit/test.c       | 147 ++++++++++++++++++++++++++++++++++++++-----------
 7 files changed, 322 insertions(+), 41 deletions(-)
 create mode 100644 lib/kunit/debugfs.c
 create mode 100644 lib/kunit/debugfs.h

diff --git a/include/kunit/test.h b/include/kunit/test.h
index 2dfb550..f7b2ed4c 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -81,6 +81,9 @@ struct kunit_resource {
 
 struct kunit;
 
+/* Size of log associated with test. */
+#define KUNIT_LOG_SIZE	512
+
 /**
  * struct kunit_case - represents an individual test case.
  *
@@ -123,8 +126,14 @@ struct kunit_case {
 
 	/* private: internal use only. */
 	bool success;
+	char *log;
 };
 
+static inline char *kunit_status_to_string(bool status)
+{
+	return status ? "ok" : "not ok";
+}
+
 /**
  * KUNIT_CASE - A helper for creating a &struct kunit_case
  *
@@ -157,6 +166,10 @@ struct kunit_suite {
 	int (*init)(struct kunit *test);
 	void (*exit)(struct kunit *test);
 	struct kunit_case *test_cases;
+
+	/* private - internal use only */
+	struct dentry *debugfs;
+	char *log;
 };
 
 /**
@@ -175,6 +188,7 @@ struct kunit {
 
 	/* private: internal use only. */
 	const char *name; /* Read only after initialization! */
+	char *log; /* Points at case log after initialization */
 	struct kunit_try_catch try_catch;
 	/*
 	 * success starts as true, and may only be set to false during a
@@ -193,10 +207,19 @@ struct kunit {
 	struct list_head resources; /* Protected by lock. */
 };
 
-void kunit_init_test(struct kunit *test, const char *name);
+void kunit_init_test(struct kunit *test, const char *name, char *log);
 
 int kunit_run_tests(struct kunit_suite *suite);
 
+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);
+
+void __kunit_test_suites_exit(struct kunit_suite **suites);
+
 /**
  * kunit_test_suites() - used to register one or more &struct kunit_suite
  *			 with KUnit.
@@ -226,20 +249,22 @@ struct kunit {
 	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;						\
+		return __kunit_test_suites_init(suites);		\
 	}								\
 	late_initcall(kunit_test_suites_init);				\
 	static void __exit kunit_test_suites_exit(void)			\
 	{								\
-		return;							\
+		return __kunit_test_suites_exit(suites);		\
 	}								\
 	module_exit(kunit_test_suites_exit)
 
 #define kunit_test_suite(suite)	kunit_test_suites(&suite)
 
+#define kunit_suite_for_each_test_case(suite, test_case)		\
+	for (test_case = suite->test_cases; test_case->run_case; test_case++)
+
+bool kunit_suite_has_succeeded(struct kunit_suite *suite);
+
 /*
  * Like kunit_alloc_resource() below, but returns the struct kunit_resource
  * object that contains the allocation. This is mostly for testing purposes.
@@ -356,8 +381,21 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
 
 void kunit_cleanup(struct kunit *test);
 
-#define kunit_printk(lvl, test, fmt, ...) \
-	printk(lvl "\t# %s: " fmt, (test)->name, ##__VA_ARGS__)
+void kunit_log_append(char *log, const char *fmt, ...);
+
+/*
+ * printk and log to per-test or per-suite log buffer.  Logging only done
+ * if CONFIG_KUNIT_DEBUGFS is 'y'; if it is 'n', no log is allocated/used.
+ */
+#define kunit_log(lvl, test_or_suite, fmt, ...)				\
+	do {								\
+		printk(lvl fmt, ##__VA_ARGS__);				\
+		kunit_log_append((test_or_suite)->log,	fmt "\n",	\
+				 ##__VA_ARGS__);			\
+	} while (0)
+
+#define kunit_printk(lvl, test, fmt, ...)				\
+	kunit_log(lvl, test, "\t# %s: " fmt, (test)->name, ##__VA_ARGS__)
 
 /**
  * kunit_info() - Prints an INFO level message associated with @test.
diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig
index 065aa16..95d12e3 100644
--- a/lib/kunit/Kconfig
+++ b/lib/kunit/Kconfig
@@ -14,6 +14,14 @@ menuconfig KUNIT
 
 if KUNIT
 
+config KUNIT_DEBUGFS
+	bool "KUnit - Enable /sys/kernel/debug/kunit debugfs representation"
+	help
+	  Enable debugfs representation for kunit.  Currently this consists
+	  of /sys/kernel/debug/kunit/<test_suite>/results files for each
+	  test suite, which allow users to see results of the last test suite
+	  run that occurred.
+
 config KUNIT_TEST
 	tristate "KUnit test for KUnit"
 	help
diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
index fab5564..724b943 100644
--- a/lib/kunit/Makefile
+++ b/lib/kunit/Makefile
@@ -5,6 +5,10 @@ kunit-objs +=				test.o \
 					assert.o \
 					try-catch.o
 
+ifeq ($(CONFIG_KUNIT_DEBUGFS),y)
+kunit-objs +=				debugfs.o
+endif
+
 obj-$(CONFIG_KUNIT_TEST) +=		kunit-test.o
 
 # string-stream-test compiles built-in only.
diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c
new file mode 100644
index 0000000..9214c49
--- /dev/null
+++ b/lib/kunit/debugfs.c
@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2020, Oracle and/or its affiliates.
+ *    Author: Alan Maguire <alan.maguire@oracle.com>
+ */
+
+#include <linux/debugfs.h>
+#include <linux/module.h>
+
+#include <kunit/test.h>
+
+#include "string-stream.h"
+
+#define KUNIT_DEBUGFS_ROOT             "kunit"
+#define KUNIT_DEBUGFS_RESULTS          "results"
+
+/*
+ * Create a debugfs representation of test suites:
+ *
+ * Path						Semantics
+ * /sys/kernel/debug/kunit/<testsuite>/results	Show results of last run for
+ *						testsuite
+ *
+ */
+
+static struct dentry *debugfs_rootdir;
+
+void kunit_debugfs_cleanup(void)
+{
+	debugfs_remove_recursive(debugfs_rootdir);
+}
+
+void kunit_debugfs_init(void)
+{
+	if (!debugfs_rootdir)
+		debugfs_rootdir = debugfs_create_dir(KUNIT_DEBUGFS_ROOT, NULL);
+}
+
+static void debugfs_print_result(struct seq_file *seq,
+				 struct kunit_suite *suite,
+				 struct kunit_case *test_case)
+{
+	if (!test_case || !test_case->log)
+		return;
+
+	seq_printf(seq, "%s", test_case->log);
+}
+
+/*
+ * /sys/kernel/debug/kunit/<testsuite>/results shows all results for testsuite.
+ */
+static int debugfs_print_results(struct seq_file *seq, void *v)
+{
+	struct kunit_suite *suite = (struct kunit_suite *)seq->private;
+	bool success = kunit_suite_has_succeeded(suite);
+	struct kunit_case *test_case;
+
+	if (!suite || !suite->log)
+		return 0;
+
+	seq_printf(seq, "%s", suite->log);
+
+	kunit_suite_for_each_test_case(suite, test_case)
+		debugfs_print_result(seq, suite, test_case);
+
+	seq_printf(seq, "%s %d - %s\n",
+		   kunit_status_to_string(success), 1, suite->name);
+	return 0;
+}
+
+static int debugfs_release(struct inode *inode, struct file *file)
+{
+	return single_release(inode, file);
+}
+
+static int debugfs_results_open(struct inode *inode, struct file *file)
+{
+	struct kunit_suite *suite;
+
+	suite = (struct kunit_suite *)inode->i_private;
+
+	return single_open(file, debugfs_print_results, suite);
+}
+
+static const struct file_operations debugfs_results_fops = {
+	.open = debugfs_results_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = debugfs_release,
+};
+
+void kunit_debugfs_create_suite(struct kunit_suite *suite)
+{
+	struct kunit_case *test_case;
+
+	/* Allocate logs before creating debugfs representation. */
+	suite->log = kzalloc(KUNIT_LOG_SIZE, GFP_KERNEL);
+	kunit_suite_for_each_test_case(suite, test_case)
+		test_case->log = kzalloc(KUNIT_LOG_SIZE, GFP_KERNEL);
+
+	suite->debugfs = debugfs_create_dir(suite->name, debugfs_rootdir);
+
+	debugfs_create_file(KUNIT_DEBUGFS_RESULTS, S_IFREG | 0444,
+			    suite->debugfs,
+			    suite, &debugfs_results_fops);
+}
+
+void kunit_debugfs_destroy_suite(struct kunit_suite *suite)
+{
+	struct kunit_case *test_case;
+
+	debugfs_remove_recursive(suite->debugfs);
+	kfree(suite->log);
+	kunit_suite_for_each_test_case(suite, test_case)
+		kfree(test_case->log);
+}
diff --git a/lib/kunit/debugfs.h b/lib/kunit/debugfs.h
new file mode 100644
index 0000000..dcc7d75
--- /dev/null
+++ b/lib/kunit/debugfs.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2020, Oracle and/or its affiliates.
+ */
+
+#ifndef _KUNIT_DEBUGFS_H
+#define _KUNIT_DEBUGFS_H
+
+#include <kunit/test.h>
+
+#ifdef CONFIG_KUNIT_DEBUGFS
+
+void kunit_debugfs_create_suite(struct kunit_suite *suite);
+void kunit_debugfs_destroy_suite(struct kunit_suite *suite);
+void kunit_debugfs_init(void);
+void kunit_debugfs_cleanup(void);
+
+#else
+
+static inline void kunit_debugfs_create_suite(struct kunit_suite *suite) { }
+
+static inline void kunit_debugfs_destroy_suite(struct kunit_suite *suite) { }
+
+static inline void kunit_debugfs_init(void) { }
+
+static inline void kunit_debugfs_cleanup(void) { }
+
+#endif /* CONFIG_KUNIT_DEBUGFS */
+
+#endif /* _KUNIT_DEBUGFS_H */
diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
index ccb8d2e..aceb5bf 100644
--- a/lib/kunit/kunit-test.c
+++ b/lib/kunit/kunit-test.c
@@ -134,7 +134,7 @@ static void kunit_resource_test_init_resources(struct kunit *test)
 {
 	struct kunit_test_resource_context *ctx = test->priv;
 
-	kunit_init_test(&ctx->test, "testing_test_init_test");
+	kunit_init_test(&ctx->test, "testing_test_init_test", NULL);
 
 	KUNIT_EXPECT_TRUE(test, list_empty(&ctx->test.resources));
 }
@@ -301,7 +301,7 @@ static int kunit_resource_test_init(struct kunit *test)
 
 	test->priv = ctx;
 
-	kunit_init_test(&ctx->test, "test_test_context");
+	kunit_init_test(&ctx->test, "test_test_context", NULL);
 
 	return 0;
 }
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 9242f93..a3fa21f 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -10,6 +10,7 @@
 #include <linux/kernel.h>
 #include <linux/sched/debug.h>
 
+#include "debugfs.h"
 #include "string-stream.h"
 #include "try-catch-impl.h"
 
@@ -28,73 +29,116 @@ static void kunit_print_tap_version(void)
 	}
 }
 
-static size_t kunit_test_cases_len(struct kunit_case *test_cases)
+/*
+ * Append formatted message to log, size of which is limited to
+ * KUNIT_LOG_SIZE bytes (including null terminating byte).
+ */
+void kunit_log_append(char *log, const char *fmt, ...)
+{
+	char line[KUNIT_LOG_SIZE];
+	va_list args;
+	int len_left;
+
+	if (!log)
+		return;
+
+	len_left = KUNIT_LOG_SIZE - strlen(log) - 1;
+	if (len_left <= 0)
+		return;
+
+	va_start(args, fmt);
+	vsnprintf(line, sizeof(line), fmt, args);
+	va_end(args);
+
+	strncat(log, line, len_left);
+}
+EXPORT_SYMBOL_GPL(kunit_log_append);
+
+size_t kunit_suite_num_test_cases(struct kunit_suite *suite)
 {
 	struct kunit_case *test_case;
 	size_t len = 0;
 
-	for (test_case = test_cases; test_case->run_case; test_case++)
+	kunit_suite_for_each_test_case(suite, test_case)
 		len++;
 
 	return len;
 }
+EXPORT_SYMBOL_GPL(kunit_suite_num_test_cases);
 
 static void kunit_print_subtest_start(struct kunit_suite *suite)
 {
 	kunit_print_tap_version();
-	pr_info("\t# Subtest: %s\n", suite->name);
-	pr_info("\t1..%zd\n", kunit_test_cases_len(suite->test_cases));
+	kunit_log(KERN_INFO, suite, "\t# Subtest: %s", suite->name);
+	kunit_log(KERN_INFO, suite, "\t1..%zd",
+		  kunit_suite_num_test_cases(suite));
 }
 
-static void kunit_print_ok_not_ok(bool should_indent,
+static void kunit_print_ok_not_ok(void *test_or_suite,
+				  bool is_test,
 				  bool is_ok,
 				  size_t test_number,
 				  const char *description)
 {
-	const char *indent, *ok_not_ok;
-
-	if (should_indent)
-		indent = "\t";
-	else
-		indent = "";
+	struct kunit_suite *suite = is_test ? NULL : test_or_suite;
+	struct kunit *test = is_test ? test_or_suite : NULL;
 
-	if (is_ok)
-		ok_not_ok = "ok";
+	/*
+	 * We do not log the test suite results as doing so would
+	 * mean debugfs display would consist of the test suite
+	 * description and status prior to individual test results.
+	 * Hence directly printk the suite status, and we will
+	 * separately seq_printf() the suite status for the debugfs
+	 * representation.
+	 */
+	if (suite)
+		pr_info("%s %zd - %s",
+			kunit_status_to_string(is_ok),
+			test_number, description);
 	else
-		ok_not_ok = "not ok";
-
-	pr_info("%s%s %zd - %s\n", indent, ok_not_ok, test_number, description);
+		kunit_log(KERN_INFO, test, "\t%s %zd - %s",
+			  kunit_status_to_string(is_ok),
+			  test_number, description);
 }
 
-static bool kunit_suite_has_succeeded(struct kunit_suite *suite)
+bool kunit_suite_has_succeeded(struct kunit_suite *suite)
 {
 	const struct kunit_case *test_case;
 
-	for (test_case = suite->test_cases; test_case->run_case; test_case++)
+	kunit_suite_for_each_test_case(suite, test_case) {
 		if (!test_case->success)
 			return false;
+	}
 
 	return true;
 }
+EXPORT_SYMBOL_GPL(kunit_suite_has_succeeded);
 
 static void kunit_print_subtest_end(struct kunit_suite *suite)
 {
 	static size_t kunit_suite_counter = 1;
 
-	kunit_print_ok_not_ok(false,
+	kunit_print_ok_not_ok((void *)suite, false,
 			      kunit_suite_has_succeeded(suite),
 			      kunit_suite_counter++,
 			      suite->name);
 }
 
-static void kunit_print_test_case_ok_not_ok(struct kunit_case *test_case,
-					    size_t test_number)
+unsigned int kunit_test_case_num(struct kunit_suite *suite,
+				 struct kunit_case *test_case)
 {
-	kunit_print_ok_not_ok(true,
-			      test_case->success,
-			      test_number,
-			      test_case->name);
+	struct kunit_case *tc;
+	unsigned int i = 1;
+
+	kunit_suite_for_each_test_case(suite, tc) {
+		if (tc == test_case)
+			return i;
+		i++;
+	}
+
+	return 0;
 }
+EXPORT_SYMBOL_GPL(kunit_test_case_num);
 
 static void kunit_print_string_stream(struct kunit *test,
 				      struct string_stream *stream)
@@ -102,6 +146,9 @@ static void kunit_print_string_stream(struct kunit *test,
 	struct string_stream_fragment *fragment;
 	char *buf;
 
+	if (string_stream_is_empty(stream))
+		return;
+
 	buf = string_stream_get_string(stream);
 	if (!buf) {
 		kunit_err(test,
@@ -175,11 +222,14 @@ void kunit_do_assertion(struct kunit *test,
 }
 EXPORT_SYMBOL_GPL(kunit_do_assertion);
 
-void kunit_init_test(struct kunit *test, const char *name)
+void kunit_init_test(struct kunit *test, const char *name, char *log)
 {
 	spin_lock_init(&test->lock);
 	INIT_LIST_HEAD(&test->resources);
 	test->name = name;
+	test->log = log;
+	if (test->log)
+		test->log[0] = '\0';
 	test->success = true;
 }
 EXPORT_SYMBOL_GPL(kunit_init_test);
@@ -290,7 +340,7 @@ static void kunit_run_case_catch_errors(struct kunit_suite *suite,
 	struct kunit_try_catch *try_catch;
 	struct kunit test;
 
-	kunit_init_test(&test, test_case->name);
+	kunit_init_test(&test, test_case->name, test_case->log);
 	try_catch = &test.try_catch;
 
 	kunit_try_catch_init(try_catch,
@@ -303,19 +353,20 @@ static void kunit_run_case_catch_errors(struct kunit_suite *suite,
 	kunit_try_catch_run(try_catch, &context);
 
 	test_case->success = test.success;
+
+	kunit_print_ok_not_ok(&test, true, test_case->success,
+			      kunit_test_case_num(suite, test_case),
+			      test_case->name);
 }
 
 int kunit_run_tests(struct kunit_suite *suite)
 {
 	struct kunit_case *test_case;
-	size_t test_case_count = 1;
 
 	kunit_print_subtest_start(suite);
 
-	for (test_case = suite->test_cases; test_case->run_case; test_case++) {
+	kunit_suite_for_each_test_case(suite, test_case)
 		kunit_run_case_catch_errors(suite, test_case);
-		kunit_print_test_case_ok_not_ok(test_case, test_case_count++);
-	}
 
 	kunit_print_subtest_end(suite);
 
@@ -323,6 +374,37 @@ int kunit_run_tests(struct kunit_suite *suite)
 }
 EXPORT_SYMBOL_GPL(kunit_run_tests);
 
+static void kunit_init_suite(struct kunit_suite *suite)
+{
+	kunit_debugfs_create_suite(suite);
+}
+
+int __kunit_test_suites_init(struct kunit_suite **suites)
+{
+	unsigned int i;
+
+	for (i = 0; suites[i] != NULL; i++) {
+		kunit_init_suite(suites[i]);
+		kunit_run_tests(suites[i]);
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(__kunit_test_suites_init);
+
+static void kunit_exit_suite(struct kunit_suite *suite)
+{
+	kunit_debugfs_destroy_suite(suite);
+}
+
+void __kunit_test_suites_exit(struct kunit_suite **suites)
+{
+	unsigned int i;
+
+	for (i = 0; suites[i] != NULL; i++)
+		kunit_exit_suite(suites[i]);
+}
+EXPORT_SYMBOL_GPL(__kunit_test_suites_exit);
+
 struct kunit_resource *kunit_alloc_and_get_resource(struct kunit *test,
 						    kunit_resource_init_t init,
 						    kunit_resource_free_t free,
@@ -489,12 +571,15 @@ void kunit_cleanup(struct kunit *test)
 
 static int __init kunit_init(void)
 {
+	kunit_debugfs_init();
+
 	return 0;
 }
 late_initcall(kunit_init);
 
 static void __exit kunit_exit(void)
 {
+	kunit_debugfs_cleanup();
 }
 module_exit(kunit_exit);
 
-- 
1.8.3.1

[PATCH v8 kunit-next 2/4] kunit: add log test

[Alan Maguire]

the logging test ensures multiple strings logged appear in the
log string associated with the test when CONFIG_KUNIT_DEBUGFS is
enabled.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Frank Rowand <frank.rowand@sony.com>
---
 lib/kunit/kunit-test.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
index aceb5bf..4f3d36a 100644
--- a/lib/kunit/kunit-test.c
+++ b/lib/kunit/kunit-test.c
@@ -329,6 +329,44 @@ static void kunit_resource_test_exit(struct kunit *test)
 	.exit = kunit_resource_test_exit,
 	.test_cases = kunit_resource_test_cases,
 };
-kunit_test_suites(&kunit_try_catch_test_suite, &kunit_resource_test_suite);
+
+static void kunit_log_test(struct kunit *test);
+
+static struct kunit_case kunit_log_test_cases[] = {
+	KUNIT_CASE(kunit_log_test),
+	{}
+};
+
+static struct kunit_suite kunit_log_test_suite = {
+	.name = "kunit-log-test",
+	.test_cases = kunit_log_test_cases,
+};
+
+static void kunit_log_test(struct kunit *test)
+{
+	struct kunit_suite *suite = &kunit_log_test_suite;
+
+	kunit_log(KERN_INFO, test, "put this in log.");
+	kunit_log(KERN_INFO, test, "this too.");
+	kunit_log(KERN_INFO, suite, "add to suite log.");
+	kunit_log(KERN_INFO, suite, "along with this.");
+
+#ifdef CONFIG_KUNIT_DEBUGFS
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
+				     strstr(test->log, "put this in log."));
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
+				     strstr(test->log, "this too."));
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
+				     strstr(suite->log, "add to suite log."));
+	KUNIT_EXPECT_NOT_ERR_OR_NULL(test,
+				     strstr(suite->log, "along with this."));
+#else
+	KUNIT_EXPECT_PTR_EQ(test, test->log, (char *)NULL);
+	KUNIT_EXPECT_PTR_EQ(test, suite->log, (char *)NULL);
+#endif
+}
+
+kunit_test_suites(&kunit_try_catch_test_suite, &kunit_resource_test_suite,
+		  &kunit_log_test_suite);
 
 MODULE_LICENSE("GPL v2");
-- 
1.8.3.1

[PATCH v8 kunit-next 3/4] kunit: subtests should be indented 4 spaces according to TAP

[Alan Maguire]

Introduce KUNIT_SUBTEST_INDENT macro which corresponds to 4-space
indentation and KUNIT_SUBSUBTEST_INDENT macro which corresponds to
8-space indentation in line with TAP spec (e.g. see "Subtests"
section of https://node-tap.org/tap-protocol/).

Use these macros in place of one or two tabs in strings to clarify
why we are indenting.

Suggested-by: Frank Rowand <frowand.list@gmail.com>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Reviewed-by: Frank Rowand <frank.rowand@sony.com>
---
 include/kunit/test.h                | 11 +++++-
 lib/kunit/assert.c                  | 79 +++++++++++++++++++------------------
 lib/kunit/test.c                    |  7 ++--
 tools/testing/kunit/kunit_parser.py | 10 ++---
 4 files changed, 59 insertions(+), 48 deletions(-)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index f7b2ed4c..9b0c46a 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -84,6 +84,14 @@ struct kunit_resource {
 /* Size of log associated with test. */
 #define KUNIT_LOG_SIZE	512
 
+/*
+ * TAP specifies subtest stream indentation of 4 spaces, 8 spaces for a
+ * sub-subtest.  See the "Subtests" section in
+ * https://node-tap.org/tap-protocol/
+ */
+#define KUNIT_SUBTEST_INDENT		"    "
+#define KUNIT_SUBSUBTEST_INDENT		"        "
+
 /**
  * struct kunit_case - represents an individual test case.
  *
@@ -395,7 +403,8 @@ static inline void *kunit_kzalloc(struct kunit *test, size_t size, gfp_t gfp)
 	} while (0)
 
 #define kunit_printk(lvl, test, fmt, ...)				\
-	kunit_log(lvl, test, "\t# %s: " fmt, (test)->name, ##__VA_ARGS__)
+	kunit_log(lvl, test, KUNIT_SUBTEST_INDENT "# %s: " fmt,		\
+		  (test)->name,	##__VA_ARGS__)
 
 /**
  * kunit_info() - Prints an INFO level message associated with @test.
diff --git a/lib/kunit/assert.c b/lib/kunit/assert.c
index 02ecd0d..33acdaa 100644
--- a/lib/kunit/assert.c
+++ b/lib/kunit/assert.c
@@ -6,6 +6,7 @@
  * Author: Brendan Higgins <brendanhiggins@google.com>
  */
 #include <kunit/assert.h>
+#include <kunit/test.h>
 
 #include "string-stream.h"
 
@@ -53,12 +54,12 @@ void kunit_unary_assert_format(const struct kunit_assert *assert,
 	kunit_base_assert_format(assert, stream);
 	if (unary_assert->expected_true)
 		string_stream_add(stream,
-				 "\tExpected %s to be true, but is false\n",
-				 unary_assert->condition);
+				  KUNIT_SUBTEST_INDENT "Expected %s to be true, but is false\n",
+				  unary_assert->condition);
 	else
 		string_stream_add(stream,
-				 "\tExpected %s to be false, but is true\n",
-				 unary_assert->condition);
+				  KUNIT_SUBTEST_INDENT "Expected %s to be false, but is true\n",
+				  unary_assert->condition);
 	kunit_assert_print_msg(assert, stream);
 }
 EXPORT_SYMBOL_GPL(kunit_unary_assert_format);
@@ -72,13 +73,13 @@ void kunit_ptr_not_err_assert_format(const struct kunit_assert *assert,
 	kunit_base_assert_format(assert, stream);
 	if (!ptr_assert->value) {
 		string_stream_add(stream,
-				 "\tExpected %s is not null, but is\n",
-				 ptr_assert->text);
+				  KUNIT_SUBTEST_INDENT "Expected %s is not null, but is\n",
+				  ptr_assert->text);
 	} else if (IS_ERR(ptr_assert->value)) {
 		string_stream_add(stream,
-				 "\tExpected %s is not error, but is: %ld\n",
-				 ptr_assert->text,
-				 PTR_ERR(ptr_assert->value));
+				  KUNIT_SUBTEST_INDENT "Expected %s is not error, but is: %ld\n",
+				  ptr_assert->text,
+				  PTR_ERR(ptr_assert->value));
 	}
 	kunit_assert_print_msg(assert, stream);
 }
@@ -92,16 +93,16 @@ void kunit_binary_assert_format(const struct kunit_assert *assert,
 
 	kunit_base_assert_format(assert, stream);
 	string_stream_add(stream,
-			 "\tExpected %s %s %s, but\n",
-			 binary_assert->left_text,
-			 binary_assert->operation,
-			 binary_assert->right_text);
-	string_stream_add(stream, "\t\t%s == %lld\n",
-			 binary_assert->left_text,
-			 binary_assert->left_value);
-	string_stream_add(stream, "\t\t%s == %lld",
-			 binary_assert->right_text,
-			 binary_assert->right_value);
+			  KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
+			  binary_assert->left_text,
+			  binary_assert->operation,
+			  binary_assert->right_text);
+	string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld\n",
+			  binary_assert->left_text,
+			  binary_assert->left_value);
+	string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %lld",
+			  binary_assert->right_text,
+			  binary_assert->right_value);
 	kunit_assert_print_msg(assert, stream);
 }
 EXPORT_SYMBOL_GPL(kunit_binary_assert_format);
@@ -114,16 +115,16 @@ void kunit_binary_ptr_assert_format(const struct kunit_assert *assert,
 
 	kunit_base_assert_format(assert, stream);
 	string_stream_add(stream,
-			 "\tExpected %s %s %s, but\n",
-			 binary_assert->left_text,
-			 binary_assert->operation,
-			 binary_assert->right_text);
-	string_stream_add(stream, "\t\t%s == %px\n",
-			 binary_assert->left_text,
-			 binary_assert->left_value);
-	string_stream_add(stream, "\t\t%s == %px",
-			 binary_assert->right_text,
-			 binary_assert->right_value);
+			  KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
+			  binary_assert->left_text,
+			  binary_assert->operation,
+			  binary_assert->right_text);
+	string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %px\n",
+			  binary_assert->left_text,
+			  binary_assert->left_value);
+	string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %px",
+			  binary_assert->right_text,
+			  binary_assert->right_value);
 	kunit_assert_print_msg(assert, stream);
 }
 EXPORT_SYMBOL_GPL(kunit_binary_ptr_assert_format);
@@ -136,16 +137,16 @@ void kunit_binary_str_assert_format(const struct kunit_assert *assert,
 
 	kunit_base_assert_format(assert, stream);
 	string_stream_add(stream,
-			 "\tExpected %s %s %s, but\n",
-			 binary_assert->left_text,
-			 binary_assert->operation,
-			 binary_assert->right_text);
-	string_stream_add(stream, "\t\t%s == %s\n",
-			 binary_assert->left_text,
-			 binary_assert->left_value);
-	string_stream_add(stream, "\t\t%s == %s",
-			 binary_assert->right_text,
-			 binary_assert->right_value);
+			  KUNIT_SUBTEST_INDENT "Expected %s %s %s, but\n",
+			  binary_assert->left_text,
+			  binary_assert->operation,
+			  binary_assert->right_text);
+	string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %s\n",
+			  binary_assert->left_text,
+			  binary_assert->left_value);
+	string_stream_add(stream, KUNIT_SUBSUBTEST_INDENT "%s == %s",
+			  binary_assert->right_text,
+			  binary_assert->right_value);
 	kunit_assert_print_msg(assert, stream);
 }
 EXPORT_SYMBOL_GPL(kunit_binary_str_assert_format);
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index a3fa21f..7a6430a 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -69,8 +69,9 @@ size_t kunit_suite_num_test_cases(struct kunit_suite *suite)
 static void kunit_print_subtest_start(struct kunit_suite *suite)
 {
 	kunit_print_tap_version();
-	kunit_log(KERN_INFO, suite, "\t# Subtest: %s", suite->name);
-	kunit_log(KERN_INFO, suite, "\t1..%zd",
+	kunit_log(KERN_INFO, suite, KUNIT_SUBTEST_INDENT "# Subtest: %s",
+		  suite->name);
+	kunit_log(KERN_INFO, suite, KUNIT_SUBTEST_INDENT "1..%zd",
 		  kunit_suite_num_test_cases(suite));
 }
 
@@ -96,7 +97,7 @@ static void kunit_print_ok_not_ok(void *test_or_suite,
 			kunit_status_to_string(is_ok),
 			test_number, description);
 	else
-		kunit_log(KERN_INFO, test, "\t%s %zd - %s",
+		kunit_log(KERN_INFO, test, KUNIT_SUBTEST_INDENT "%s %zd - %s",
 			  kunit_status_to_string(is_ok),
 			  test_number, description);
 }
diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
index 0281506..64aac9d 100644
--- a/tools/testing/kunit/kunit_parser.py
+++ b/tools/testing/kunit/kunit_parser.py
@@ -94,7 +94,7 @@ def print_log(log):
 	for m in log:
 		print_with_timestamp(m)
 
-TAP_ENTRIES = re.compile(r'^(TAP|\t?ok|\t?not ok|\t?[0-9]+\.\.[0-9]+|\t?#).*$')
+TAP_ENTRIES = re.compile(r'^(TAP|[\s]*ok|[\s]*not ok|[\s]*[0-9]+\.\.[0-9]+|[\s]*#).*$')
 
 def consume_non_diagnositic(lines: List[str]) -> None:
 	while lines and not TAP_ENTRIES.match(lines[0]):
@@ -107,7 +107,7 @@ def save_non_diagnositic(lines: List[str], test_case: TestCase) -> None:
 
 OkNotOkResult = namedtuple('OkNotOkResult', ['is_ok','description', 'text'])
 
-OK_NOT_OK_SUBTEST = re.compile(r'^\t(ok|not ok) [0-9]+ - (.*)$')
+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]+ - (.*)$')
 
@@ -134,7 +134,7 @@ def parse_ok_not_ok_test_case(lines: List[str], test_case: TestCase) -> bool:
 	else:
 		return False
 
-SUBTEST_DIAGNOSTIC = re.compile(r'^\t# .*?: (.*)$')
+SUBTEST_DIAGNOSTIC = re.compile(r'^[\s]+# .*?: (.*)$')
 DIAGNOSTIC_CRASH_MESSAGE = 'kunit test case crashed!'
 
 def parse_diagnostic(lines: List[str], test_case: TestCase) -> bool:
@@ -161,7 +161,7 @@ def parse_test_case(lines: List[str]) -> TestCase:
 	else:
 		return None
 
-SUBTEST_HEADER = re.compile(r'^\t# Subtest: (.*)$')
+SUBTEST_HEADER = re.compile(r'^[\s]+# Subtest: (.*)$')
 
 def parse_subtest_header(lines: List[str]) -> str:
 	consume_non_diagnositic(lines)
@@ -174,7 +174,7 @@ def parse_subtest_header(lines: List[str]) -> str:
 	else:
 		return None
 
-SUBTEST_PLAN = re.compile(r'\t[0-9]+\.\.([0-9]+)')
+SUBTEST_PLAN = re.compile(r'[\s]+[0-9]+\.\.([0-9]+)')
 
 def parse_subtest_plan(lines: List[str]) -> int:
 	consume_non_diagnositic(lines)
-- 
1.8.3.1

[PATCH v8 kunit-next 4/4] kunit: update documentation to describe debugfs representation

[Alan Maguire]

Documentation should describe debugfs layout and semantics.

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Frank Rowand <frank.rowand@sony.com>
---
 Documentation/dev-tools/kunit/usage.rst | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
index 607758a..473a236 100644
--- a/Documentation/dev-tools/kunit/usage.rst
+++ b/Documentation/dev-tools/kunit/usage.rst
@@ -591,3 +591,17 @@ able to run one test case per invocation.
 
 .. TODO(brendanhiggins@google.com): Add an actual example of an architecture
    dependent KUnit test.
+
+KUnit debugfs representation
+============================
+When kunit test suites are initialized, they create an associated directory
+in /sys/kernel/debug/kunit/<test-suite>.  The directory contains one file
+
+- results: "cat results" displays results of each test case and the results
+  of the entire suite for the last test run.
+
+The debugfs representation is primarily of use when kunit test suites are
+run in a native environment, either as modules or builtin.  Having a way
+to display results like this is valuable as otherwise results can be
+intermixed with other events in dmesg output.  The maximum size of each
+results file is KUNIT_LOG_SIZE bytes (defined in include/kunit/test.h).
-- 
1.8.3.1

Re: [PATCH v8 kunit-next 0/4] kunit: add debugfs representation to show results

[shuah]

On 3/26/20 8:25 AM, Alan Maguire wrote:
> When kunit tests are run on native (i.e. non-UML) environments, the results
> of test execution are often intermixed with dmesg output.  This patch
> series attempts to solve this by providing a debugfs representation
> of the results of the last test run, available as
> 
> /sys/kernel/debug/kunit/<testsuite>/results
> 
> Changes since v7:
> 
> - renamed KUNIT_INDENT[2] to KUNIT_SUBTEST_INDENT, KUNIT_SUBSUBTEST_INDENT
>    and added more description to their definitions to clarify why they
>    are defined as they are (Shuah)
> - defined KUNIT_SUBSUBTEST_INDENT directly as 8 spaces to avoid
>    checkpatch error (Shuah)
> 
> Changes since v6:
> 
> - fixed regexp parsing in kunit_parser.py to ensure test results are read
>    successfully with 4-space indentation (Brendan, patch 3)
> 
> Changes since v5:
> 
> - replaced undefined behaviour use of snprintf(buf, ..., buf) in
>    kunit_log() with a function to append string to existing log
>    (Frank, patch 1)
> - added clarification on log size limitations to documentation
>    (Frank, patch 4)
> 
> Changes since v4:
> 
> - added suite-level log expectations to kunit log test (Brendan, patch 2)
> - added log expectations (of it being NULL) for case where
>    CONFIG_KUNIT_DEBUGFS=n to kunit log test (patch 2)
> - added patch 3 which replaces subtest tab indentation with 4 space
>    indentation as per TAP 14 spec (Frank, patch 3)
> 
> Changes since v3:
> 
> - added CONFIG_KUNIT_DEBUGFS to support conditional compilation of debugfs
>    representation, including string logging (Frank, patch 1)
> - removed unneeded NULL check for test_case in
>    kunit_suite_for_each_test_case() (Frank, patch 1)
> - added kunit log test to verify logging multiple strings works
>    (Frank, patch 2)
> - rephrased description of results file (Frank, patch 3)
> 
> Changes since v2:
> 
> - updated kunit_status2str() to kunit_status_to_string() and made it
>    static inline in include/kunit/test.h (Brendan)
> - added log string to struct kunit_suite and kunit_case, with log
>    pointer in struct kunit pointing at the case log.  This allows us
>    to collect kunit_[err|info|warning]() messages at the same time
>    as we printk() them.  This solves for the most part the sharing
>    of log messages between test execution and debugfs since we
>    just print the suite log (which contains the test suite preamble)
>    and the individual test logs.  The only exception is the suite-level
>    status, which we cannot store in the suite log as it would mean
>    we'd print the suite and its status prior to the suite's results.
>    (Brendan, patch 1)
> - dropped debugfs-based kunit run patch for now so as not to cause
>    problems with tests currently under development (Brendan)
> - fixed doc issues with code block (Brendan, patch 3)
> 
> Changes since v1:
>   - trimmed unneeded include files in lib/kunit/debugfs.c (Greg)
>   - renamed global debugfs functions to be prefixed with kunit_ (Greg)
>   - removed error checking for debugfs operations (Greg)
> 
> Alan Maguire (4):
>    kunit: add debugfs /sys/kernel/debug/kunit/<suite>/results display
>    kunit: add log test
>    kunit: subtests should be indented 4 spaces according to TAP
>    kunit: update documentation to describe debugfs representation
> 
>   Documentation/dev-tools/kunit/usage.rst |  14 +++
>   include/kunit/test.h                    |  63 ++++++++++++--
>   lib/kunit/Kconfig                       |   8 ++
>   lib/kunit/Makefile                      |   4 +
>   lib/kunit/assert.c                      |  79 ++++++++---------
>   lib/kunit/debugfs.c                     | 116 +++++++++++++++++++++++++
>   lib/kunit/debugfs.h                     |  30 +++++++
>   lib/kunit/kunit-test.c                  |  44 +++++++++-
>   lib/kunit/test.c                        | 148 +++++++++++++++++++++++++-------
>   tools/testing/kunit/kunit_parser.py     |  10 +--
>   10 files changed, 430 insertions(+), 86 deletions(-)
>   create mode 100644 lib/kunit/debugfs.c
>   create mode 100644 lib/kunit/debugfs.h
> 

Thanks. Applied to linux-kselftest kunit for 5.7-rc1.

thanks,
-- Shuah

Re: [PATCH v8 kunit-next 1/4] kunit: add debugfs /sys/kernel/debug/kunit/<suite>/results display

[shuah]

Hi Alan,

On 3/26/20 8:25 AM, Alan Maguire wrote:
> add debugfs support for displaying kunit test suite results; this is
> especially useful for module-loaded tests to allow disentangling of
> test result display from other dmesg events.  debugfs support is
> provided if CONFIG_KUNIT_DEBUGFS=y.
> 
> As well as printk()ing messages, we append them to a per-test log.
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
> Reviewed-by: Frank Rowand <frank.rowand@sony.com>
> ---
>   include/kunit/test.h   |  54 +++++++++++++++---
>   lib/kunit/Kconfig      |   8 +++

Missing defaults for config options?

>   lib/kunit/Makefile     |   4 ++
>   lib/kunit/debugfs.c    | 116 ++++++++++++++++++++++++++++++++++++++
>   lib/kunit/debugfs.h    |  30 ++++++++++
>   lib/kunit/kunit-test.c |   4 +-
>   lib/kunit/test.c       | 147 ++++++++++++++++++++++++++++++++++++++-----------
>   7 files changed, 322 insertions(+), 41 deletions(-)
>   create mode 100644 lib/kunit/debugfs.c
>   create mode 100644 lib/kunit/debugfs.h
> 

snip

> diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig
> index 065aa16..95d12e3 100644
> --- a/lib/kunit/Kconfig
> +++ b/lib/kunit/Kconfig
> @@ -14,6 +14,14 @@ menuconfig KUNIT
>   
>   if KUNIT
>   
> +config KUNIT_DEBUGFS
> +	bool "KUnit - Enable /sys/kernel/debug/kunit debugfs representation"
> +	help
> +	  Enable debugfs representation for kunit.  Currently this consists
> +	  of /sys/kernel/debug/kunit/<test_suite>/results files for each
> +	  test suite, which allow users to see results of the last test suite
> +	  run that occurred.
> +

Any reason why there is default for this option?

Same for all other options. I am sending pull request for now without
defaults. Would like to see this fixed for rc2.

thanks,
-- Shuah

Re: [PATCH v8 kunit-next 1/4] kunit: add debugfs /sys/kernel/debug/kunit/<suite>/results display

[Alan Maguire]

On Wed, 1 Apr 2020, shuah wrote:

> Hi Alan,
> 
> On 3/26/20 8:25 AM, Alan Maguire wrote:
> > add debugfs support for displaying kunit test suite results; this is
> > especially useful for module-loaded tests to allow disentangling of
> > test result display from other dmesg events.  debugfs support is
> > provided if CONFIG_KUNIT_DEBUGFS=y.
> > 
> > As well as printk()ing messages, we append them to a per-test log.
> > 
> > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
> > Reviewed-by: Frank Rowand <frank.rowand@sony.com>
> > ---
> >   include/kunit/test.h   |  54 +++++++++++++++---
> >   lib/kunit/Kconfig      |   8 +++
> 
> Missing defaults for config options?
> 
> >   lib/kunit/Makefile     |   4 ++
> >   lib/kunit/debugfs.c    | 116 ++++++++++++++++++++++++++++++++++++++
> >   lib/kunit/debugfs.h    |  30 ++++++++++
> >   lib/kunit/kunit-test.c |   4 +-
> >   lib/kunit/test.c       | 147
> >   ++++++++++++++++++++++++++++++++++++++-----------
> >   7 files changed, 322 insertions(+), 41 deletions(-)
> >   create mode 100644 lib/kunit/debugfs.c
> >   create mode 100644 lib/kunit/debugfs.h
> > 
> 
> snip
> 
> > diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig
> > index 065aa16..95d12e3 100644
> > --- a/lib/kunit/Kconfig
> > +++ b/lib/kunit/Kconfig
> > @@ -14,6 +14,14 @@ menuconfig KUNIT
> >   
> >   if KUNIT
> >   
> > +config KUNIT_DEBUGFS
> > +	bool "KUnit - Enable /sys/kernel/debug/kunit debugfs representation"
> > +	help
> > +	  Enable debugfs representation for kunit.  Currently this consists
> > +	  of /sys/kernel/debug/kunit/<test_suite>/results files for each
> > +	  test suite, which allow users to see results of the last test suite
> > +	  run that occurred.
> > +
> 
> Any reason why there is default for this option?
> 
> Same for all other options. I am sending pull request for now without
> defaults. Would like to see this fixed for rc2.
>

Sure, I'll send a patch shortly. Just wanted to get opinions
on what those defaults should be; my working assumption is

- CONFIG_KUNIT should be default n; 
- CONFIG_KUNIT_DEBUGFS should be default y (enabled by default
  if CONFIG_KUNIT is set);
- CONFIG_KUNIT_TEST, CONFIG_KUNIT_EXAMPLE_TEST should be default n

Does this sound right? Thanks!

Alan

> thanks,
> -- Shuah
> 

Re: [PATCH v8 kunit-next 1/4] kunit: add debugfs /sys/kernel/debug/kunit/<suite>/results display

[Greg KH]

On Thu, Apr 02, 2020 at 04:27:35PM +0100, Alan Maguire wrote:
> On Wed, 1 Apr 2020, shuah wrote:
> 
> > Hi Alan,
> > 
> > On 3/26/20 8:25 AM, Alan Maguire wrote:
> > > add debugfs support for displaying kunit test suite results; this is
> > > especially useful for module-loaded tests to allow disentangling of
> > > test result display from other dmesg events.  debugfs support is
> > > provided if CONFIG_KUNIT_DEBUGFS=y.
> > > 
> > > As well as printk()ing messages, we append them to a per-test log.
> > > 
> > > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > > Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
> > > Reviewed-by: Frank Rowand <frank.rowand@sony.com>
> > > ---
> > >   include/kunit/test.h   |  54 +++++++++++++++---
> > >   lib/kunit/Kconfig      |   8 +++
> > 
> > Missing defaults for config options?
> > 
> > >   lib/kunit/Makefile     |   4 ++
> > >   lib/kunit/debugfs.c    | 116 ++++++++++++++++++++++++++++++++++++++
> > >   lib/kunit/debugfs.h    |  30 ++++++++++
> > >   lib/kunit/kunit-test.c |   4 +-
> > >   lib/kunit/test.c       | 147
> > >   ++++++++++++++++++++++++++++++++++++++-----------
> > >   7 files changed, 322 insertions(+), 41 deletions(-)
> > >   create mode 100644 lib/kunit/debugfs.c
> > >   create mode 100644 lib/kunit/debugfs.h
> > > 
> > 
> > snip
> > 
> > > diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig
> > > index 065aa16..95d12e3 100644
> > > --- a/lib/kunit/Kconfig
> > > +++ b/lib/kunit/Kconfig
> > > @@ -14,6 +14,14 @@ menuconfig KUNIT
> > >   
> > >   if KUNIT
> > >   
> > > +config KUNIT_DEBUGFS
> > > +	bool "KUnit - Enable /sys/kernel/debug/kunit debugfs representation"
> > > +	help
> > > +	  Enable debugfs representation for kunit.  Currently this consists
> > > +	  of /sys/kernel/debug/kunit/<test_suite>/results files for each
> > > +	  test suite, which allow users to see results of the last test suite
> > > +	  run that occurred.
> > > +
> > 
> > Any reason why there is default for this option?
> > 
> > Same for all other options. I am sending pull request for now without
> > defaults. Would like to see this fixed for rc2.
> >
> 
> Sure, I'll send a patch shortly. Just wanted to get opinions
> on what those defaults should be; my working assumption is
> 
> - CONFIG_KUNIT should be default n; 

No default means 'n', so there's no need to say that at all.

> - CONFIG_KUNIT_DEBUGFS should be default y (enabled by default
>   if CONFIG_KUNIT is set);

Why?  If it's is 'y', then don't even make it an option at all, just
always have it :)

'y' is almost always reserved for "your machine will not function
properly without this enabled".

> - CONFIG_KUNIT_TEST, CONFIG_KUNIT_EXAMPLE_TEST should be default n

So no need to specify anything, 'n' is the default.

thanks,

greg k-h

Re: [PATCH v8 kunit-next 1/4] kunit: add debugfs /sys/kernel/debug/kunit/<suite>/results display

[Alan Maguire]

On Thu, 2 Apr 2020, Greg KH wrote:

> On Thu, Apr 02, 2020 at 04:27:35PM +0100, Alan Maguire wrote:
> > On Wed, 1 Apr 2020, shuah wrote:
> > 
> > > Hi Alan,
> > > 
> > > On 3/26/20 8:25 AM, Alan Maguire wrote:
> > > > add debugfs support for displaying kunit test suite results; this is
> > > > especially useful for module-loaded tests to allow disentangling of
> > > > test result display from other dmesg events.  debugfs support is
> > > > provided if CONFIG_KUNIT_DEBUGFS=y.
> > > > 
> > > > As well as printk()ing messages, we append them to a per-test log.
> > > > 
> > > > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > > > Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
> > > > Reviewed-by: Frank Rowand <frank.rowand@sony.com>
> > > > ---
> > > >   include/kunit/test.h   |  54 +++++++++++++++---
> > > >   lib/kunit/Kconfig      |   8 +++
> > > 
> > > Missing defaults for config options?
> > > 
> > > >   lib/kunit/Makefile     |   4 ++
> > > >   lib/kunit/debugfs.c    | 116 ++++++++++++++++++++++++++++++++++++++
> > > >   lib/kunit/debugfs.h    |  30 ++++++++++
> > > >   lib/kunit/kunit-test.c |   4 +-
> > > >   lib/kunit/test.c       | 147
> > > >   ++++++++++++++++++++++++++++++++++++++-----------
> > > >   7 files changed, 322 insertions(+), 41 deletions(-)
> > > >   create mode 100644 lib/kunit/debugfs.c
> > > >   create mode 100644 lib/kunit/debugfs.h
> > > > 
> > > 
> > > snip
> > > 
> > > > diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig
> > > > index 065aa16..95d12e3 100644
> > > > --- a/lib/kunit/Kconfig
> > > > +++ b/lib/kunit/Kconfig
> > > > @@ -14,6 +14,14 @@ menuconfig KUNIT
> > > >   
> > > >   if KUNIT
> > > >   
> > > > +config KUNIT_DEBUGFS
> > > > +	bool "KUnit - Enable /sys/kernel/debug/kunit debugfs representation"
> > > > +	help
> > > > +	  Enable debugfs representation for kunit.  Currently this consists
> > > > +	  of /sys/kernel/debug/kunit/<test_suite>/results files for each
> > > > +	  test suite, which allow users to see results of the last test suite
> > > > +	  run that occurred.
> > > > +
> > > 
> > > Any reason why there is default for this option?
> > > 
> > > Same for all other options. I am sending pull request for now without
> > > defaults. Would like to see this fixed for rc2.
> > >
> > 
> > Sure, I'll send a patch shortly. Just wanted to get opinions
> > on what those defaults should be; my working assumption is
> > 
> > - CONFIG_KUNIT should be default n; 
> 
> No default means 'n', so there's no need to say that at all.
> 

Great!

> > - CONFIG_KUNIT_DEBUGFS should be default y (enabled by default
> >   if CONFIG_KUNIT is set);
> 
> Why?  If it's is 'y', then don't even make it an option at all, just
> always have it :)
> 
> 'y' is almost always reserved for "your machine will not function
> properly without this enabled".
>

Okay, so in this case not having a default makes sense I think
(we added the option as saying 'n' avoids allocating per-test
loggging memory which might cause problems on low-memory systems).

> > - CONFIG_KUNIT_TEST, CONFIG_KUNIT_EXAMPLE_TEST should be default n
> 
> So no need to specify anything, 'n' is the default.
>

Excellent! So based on the above seems like we don't need
to add any defaults here I think.  Shuah/Brendan, let me know
if I'm missing anything here. Thanks!

Alan

> thanks,
> 
> greg k-h
> 

Re: [PATCH v8 kunit-next 1/4] kunit: add debugfs /sys/kernel/debug/kunit/<suite>/results display

[Marco Elver]

Hello,

On Thu, 26 Mar 2020, Alan Maguire wrote:

> add debugfs support for displaying kunit test suite results; this is
> especially useful for module-loaded tests to allow disentangling of
> test result display from other dmesg events.  debugfs support is
> provided if CONFIG_KUNIT_DEBUGFS=y.
> 
> As well as printk()ing messages, we append them to a per-test log.
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
> Reviewed-by: Frank Rowand <frank.rowand@sony.com>
> ---
>  include/kunit/test.h   |  54 +++++++++++++++---
>  lib/kunit/Kconfig      |   8 +++
>  lib/kunit/Makefile     |   4 ++
>  lib/kunit/debugfs.c    | 116 ++++++++++++++++++++++++++++++++++++++
>  lib/kunit/debugfs.h    |  30 ++++++++++
>  lib/kunit/kunit-test.c |   4 +-
>  lib/kunit/test.c       | 147 ++++++++++++++++++++++++++++++++++++++-----------
>  7 files changed, 322 insertions(+), 41 deletions(-)
>  create mode 100644 lib/kunit/debugfs.c
>  create mode 100644 lib/kunit/debugfs.h
> 
[...]
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 9242f93..a3fa21f 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
[...]
> -static void kunit_print_ok_not_ok(bool should_indent,
> +static void kunit_print_ok_not_ok(void *test_or_suite,
> +				  bool is_test,
>  				  bool is_ok,
>  				  size_t test_number,
>  				  const char *description)
>  {
> -	const char *indent, *ok_not_ok;
> -
> -	if (should_indent)
> -		indent = "\t";
> -	else
> -		indent = "";
> +	struct kunit_suite *suite = is_test ? NULL : test_or_suite;
> +	struct kunit *test = is_test ? test_or_suite : NULL;
>  
> -	if (is_ok)
> -		ok_not_ok = "ok";
> +	/*
> +	 * We do not log the test suite results as doing so would
> +	 * mean debugfs display would consist of the test suite
> +	 * description and status prior to individual test results.
> +	 * Hence directly printk the suite status, and we will
> +	 * separately seq_printf() the suite status for the debugfs
> +	 * representation.
> +	 */
> +	if (suite)
> +		pr_info("%s %zd - %s",

I think this is missing '\n' -- is this intentional?

With v5.7-rc1, when I run a test via module, the final "ok" is only
printed once another message is printed to the kernel log (which can
take a while).

Thanks,
-- Marco

> +			kunit_status_to_string(is_ok),
> +			test_number, description);
>  	else
> -		ok_not_ok = "not ok";
> -
> -	pr_info("%s%s %zd - %s\n", indent, ok_not_ok, test_number, description);
> +		kunit_log(KERN_INFO, test, "\t%s %zd - %s",
> +			  kunit_status_to_string(is_ok),
> +			  test_number, description);
>  }
[...]