The indentation of parameterized tests messages is currently broken in kunit. Try to fix that by introducing a test level attribute, that will be increased during nested parameterized tests execution, and use it to generate correct indent at the runtime when printing message or writing them to the log.
Also improve kunit by providing test plan for the parameterized tests.
Cc: David Gow davidgow@google.com Cc: Rae Moar rmoar@google.com
Michal Wajdeczko (4): kunit: Drop redundant text from suite init failure message kunit: Fix indentation level of suite messages kunit: Fix indentation of parameterized tests messages kunit: Prepare test plan for parameterized subtests
include/kunit/test.h | 25 ++++++++++++-- lib/kunit/test.c | 79 +++++++++++++++++++++++--------------------- 2 files changed, 65 insertions(+), 39 deletions(-)
If a suite initialization fails, then our diagnostic message will include redundant indent and hash sign as all this was already added by the kunit_printk() used by kunit_err() macro.
This could be easily seen if we force some error in our example test by modyfing example_test_init_suite() and running:
$ ./tools/testing/kunit/kunit.py run --raw_output \ --kunitconfig ./lib/kunit/.kunitconfig "example.*"
KTAP version 1 1..1 # example: initializing suite # example: # failed to initialize (-19) not ok 1 example
Fix that and while around improve error code reporting by using error pointer format %pe that gives more user friendly output:
KTAP version 1 1..1 # example: initializing suite # example: failed to initialize (-ENODEV) not ok 1 example
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Cc: David Gow davidgow@google.com Cc: Rae Moar rmoar@google.com --- lib/kunit/test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index f2eb71f1a66c..fb5981ce578d 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -568,8 +568,8 @@ int kunit_run_tests(struct kunit_suite *suite) if (suite->suite_init) { suite->suite_init_err = suite->suite_init(suite); if (suite->suite_init_err) { - kunit_err(suite, KUNIT_SUBTEST_INDENT - "# failed to initialize (%d)", suite->suite_init_err); + kunit_err(suite, "failed to initialize (%pe)", + ERR_PTR(suite->suite_init_err)); goto suite_end; } }
On Mon, Sep 25, 2023 at 1:58 PM Michal Wajdeczko michal.wajdeczko@intel.com wrote:
If a suite initialization fails, then our diagnostic message will include redundant indent and hash sign as all this was already added by the kunit_printk() used by kunit_err() macro.
This could be easily seen if we force some error in our example test by modyfing example_test_init_suite() and running:
$ ./tools/testing/kunit/kunit.py run --raw_output \ --kunitconfig ./lib/kunit/.kunitconfig "example.*"
KTAP version 1 1..1 # example: initializing suite # example: # failed to initialize (-19) not ok 1 exampleFix that and while around improve error code reporting by using error pointer format %pe that gives more user friendly output:
KTAP version 1 1..1 # example: initializing suite # example: failed to initialize (-ENODEV) not ok 1 exampleSigned-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Cc: David Gow davidgow@google.com Cc: Rae Moar rmoar@google.com
Hello!
This change looks good to me.
Reviewed-by: Rae Moar rmoar@google.com
Thanks! -Rae
lib/kunit/test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index f2eb71f1a66c..fb5981ce578d 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -568,8 +568,8 @@ int kunit_run_tests(struct kunit_suite *suite) if (suite->suite_init) { suite->suite_init_err = suite->suite_init(suite); if (suite->suite_init_err) {
kunit_err(suite, KUNIT_SUBTEST_INDENT"# failed to initialize (%d)", suite->suite_init_err);
kunit_err(suite, "failed to initialize (%pe)",ERR_PTR(suite->suite_init_err)); goto suite_end; } }-- 2.25.1
On Tue, 26 Sept 2023 at 01:58, Michal Wajdeczko michal.wajdeczko@intel.com wrote:
If a suite initialization fails, then our diagnostic message will include redundant indent and hash sign as all this was already added by the kunit_printk() used by kunit_err() macro.
This could be easily seen if we force some error in our example test by modyfing example_test_init_suite() and running:
$ ./tools/testing/kunit/kunit.py run --raw_output \ --kunitconfig ./lib/kunit/.kunitconfig "example.*"
KTAP version 1 1..1 # example: initializing suite # example: # failed to initialize (-19) not ok 1 exampleFix that and while around improve error code reporting by using error pointer format %pe that gives more user friendly output:
KTAP version 1 1..1 # example: initializing suite # example: failed to initialize (-ENODEV) not ok 1 exampleSigned-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Cc: David Gow davidgow@google.com Cc: Rae Moar rmoar@google.com
Nice catch!
Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
lib/kunit/test.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index f2eb71f1a66c..fb5981ce578d 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -568,8 +568,8 @@ int kunit_run_tests(struct kunit_suite *suite) if (suite->suite_init) { suite->suite_init_err = suite->suite_init(suite); if (suite->suite_init_err) {
kunit_err(suite, KUNIT_SUBTEST_INDENT"# failed to initialize (%d)", suite->suite_init_err);
kunit_err(suite, "failed to initialize (%pe)",ERR_PTR(suite->suite_init_err)); goto suite_end; } }-- 2.25.1
A kunit suite is a top level test from the KTAP point of view but all suite diagnostic messages are printed at the subtest level:
$ ./tools/testing/kunit/kunit.py run --raw_output \ --kunitconfig ./lib/kunit/.kunitconfig "example.*simple*"
KTAP version 1 1..1 # example: initializing suite # example: failed to initialize (-ENODEV) not ok 1 example
KTAP version 1 1..1 # example: initializing suite KTAP version 1 # Subtest: example # module: kunit_example_test 1..1 # example_simple_test: initializing # example_simple_test: cleaning up ok 1 example_simple_test # example: exiting suite ok 1 example
Replace hardcoded indent in kunit_printk() with flexible indentation based on the argument type (test or suite):
KTAP version 1 1..1 # example: initializing suite # example: failed to initialize (-ENODEV) not ok 1 example
KTAP version 1 1..1 # example: initializing suite KTAP version 1 # Subtest: example # module: kunit_example_test 1..1 # example_simple_test: initializing # example_simple_test: cleaning up ok 1 example_simple_test # example: exiting suite ok 1 example
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Cc: David Gow davidgow@google.com Cc: Rae Moar rmoar@google.com --- include/kunit/test.h | 24 ++++++++++++++++++++++-- lib/kunit/test.c | 7 ------- 2 files changed, 22 insertions(+), 9 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index 20ed9f9275c9..158876c4cb43 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -509,6 +509,21 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt, kunit_try_catch_throw(&((test_or_suite)->try_catch)); \ } while (0)
+/* Currently supported test levels */ +enum { + KUNIT_LEVEL_SUITE = 0, + KUNIT_LEVEL_CASE, + KUNIT_LEVEL_CASE_PARAM, +}; + +#define kunit_level(test_or_suite) \ + _Generic((test_or_suite), \ + struct kunit_suite * : KUNIT_LEVEL_SUITE, \ + struct kunit * : KUNIT_LEVEL_CASE) + +#define kunit_indent_level(test_or_suite) \ + (KUNIT_INDENT_LEN * kunit_level(test_or_suite)) + /* * 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. @@ -520,9 +535,14 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt, ##__VA_ARGS__); \ } while (0)
+#define kunit_log_indent(lvl, test_or_suite, fmt, ...) \ + kunit_log(lvl, test_or_suite, "%*s" fmt, \ + kunit_indent_level(test_or_suite), "", \ + ##__VA_ARGS__) + #define kunit_printk(lvl, test, fmt, ...) \ - kunit_log(lvl, test, KUNIT_SUBTEST_INDENT "# %s: " fmt, \ - (test)->name, ##__VA_ARGS__) + kunit_log_indent(lvl, test, "# %s: " fmt, \ + (test)->name, ##__VA_ARGS__)
/** * kunit_info() - Prints an INFO level message associated with @test. diff --git a/lib/kunit/test.c b/lib/kunit/test.c index fb5981ce578d..d10e6d610e20 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -135,13 +135,6 @@ size_t kunit_suite_num_test_cases(struct kunit_suite *suite) } EXPORT_SYMBOL_GPL(kunit_suite_num_test_cases);
-/* Currently supported test levels */ -enum { - KUNIT_LEVEL_SUITE = 0, - KUNIT_LEVEL_CASE, - KUNIT_LEVEL_CASE_PARAM, -}; - static void kunit_print_suite_start(struct kunit_suite *suite) { /*
On Mon, Sep 25, 2023 at 1:58 PM Michal Wajdeczko michal.wajdeczko@intel.com wrote:
A kunit suite is a top level test from the KTAP point of view but all suite diagnostic messages are printed at the subtest level:
$ ./tools/testing/kunit/kunit.py run --raw_output \ --kunitconfig ./lib/kunit/.kunitconfig "example.*simple*" KTAP version 1 1..1 # example: initializing suite # example: failed to initialize (-ENODEV) not ok 1 example KTAP version 1 1..1 # example: initializing suite KTAP version 1 # Subtest: example # module: kunit_example_test 1..1 # example_simple_test: initializing # example_simple_test: cleaning up ok 1 example_simple_test # example: exiting suite ok 1 exampleReplace hardcoded indent in kunit_printk() with flexible indentation based on the argument type (test or suite):
KTAP version 1 1..1 # example: initializing suite # example: failed to initialize (-ENODEV) not ok 1 example KTAP version 1 1..1 # example: initializing suite KTAP version 1 # Subtest: example # module: kunit_example_test 1..1 # example_simple_test: initializing # example_simple_test: cleaning up ok 1 example_simple_test # example: exiting suite ok 1 example
Hi!
I am happy to see this change to improve the indentation of parameterized tests. It has been bugging me for a bit. This seems to be working well but I just had a few comments to potentially discuss.
Thanks!
-Rae
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Cc: David Gow davidgow@google.com Cc: Rae Moar rmoar@google.com
include/kunit/test.h | 24 ++++++++++++++++++++++-- lib/kunit/test.c | 7 ------- 2 files changed, 22 insertions(+), 9 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index 20ed9f9275c9..158876c4cb43 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -509,6 +509,21 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt, kunit_try_catch_throw(&((test_or_suite)->try_catch)); \ } while (0)
+/* Currently supported test levels */ +enum {
KUNIT_LEVEL_SUITE = 0,KUNIT_LEVEL_CASE,KUNIT_LEVEL_CASE_PARAM,+};
I do find this slightly confusing to have a KUNIT_LEVEL_SUITE as the suite output occurs on multiple levels. Plus, I also don't see any uses of the SUITE level or the PARAM level anymore. Do you have any ideas on this?
We could consider just using the test->level field introduced in the next patch to manage the levels. Or I wouldn't mind keeping this just for clarity?
+#define kunit_level(test_or_suite) \
_Generic((test_or_suite), \struct kunit_suite * : KUNIT_LEVEL_SUITE, \struct kunit * : KUNIT_LEVEL_CASE)
I am not super familiar with using _Generic so I would want David's opinion.
Also I can think of cases where it would be helpful to add an option for struct kunit_case *, however, that may be an addition for the future.
And then additionally, this macro seems to be only used for the struct kunit * case. Will we plan to use this in the future for suites?
+#define kunit_indent_level(test_or_suite) \
(KUNIT_INDENT_LEN * kunit_level(test_or_suite))/*
- 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.
@@ -520,9 +535,14 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt, ##__VA_ARGS__); \ } while (0)
+#define kunit_log_indent(lvl, test_or_suite, fmt, ...) \
kunit_log(lvl, test_or_suite, "%*s" fmt, \kunit_indent_level(test_or_suite), "", \##__VA_ARGS__)#define kunit_printk(lvl, test, fmt, ...) \
kunit_log(lvl, test, KUNIT_SUBTEST_INDENT "# %s: " fmt, \(test)->name, ##__VA_ARGS__)
kunit_log_indent(lvl, test, "# %s: " fmt, \(test)->name, ##__VA_ARGS__)
I wonder if we could consider removing the need to use KUNIT_SUBTEST_INDENT in all locations. I am primarily thinking about its uses in debugfs.c and test.c.
For example in debugfs.c: pr_info(KUNIT_SUBTEST_INDENT "KTAP version 1\n");
Although, as this is a suite object that is printing at the test case level, I am not quite sure how to use the existing macros.
/**
- kunit_info() - Prints an INFO level message associated with @test.
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index fb5981ce578d..d10e6d610e20 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -135,13 +135,6 @@ size_t kunit_suite_num_test_cases(struct kunit_suite *suite) } EXPORT_SYMBOL_GPL(kunit_suite_num_test_cases);
-/* Currently supported test levels */ -enum {
KUNIT_LEVEL_SUITE = 0,KUNIT_LEVEL_CASE,KUNIT_LEVEL_CASE_PARAM,-};
static void kunit_print_suite_start(struct kunit_suite *suite) { /* -- 2.25.1
On 28.09.2023 22:52, Rae Moar wrote:
On Mon, Sep 25, 2023 at 1:58 PM Michal Wajdeczko michal.wajdeczko@intel.com wrote:
A kunit suite is a top level test from the KTAP point of view but all suite diagnostic messages are printed at the subtest level:
$ ./tools/testing/kunit/kunit.py run --raw_output \ --kunitconfig ./lib/kunit/.kunitconfig "example.*simple*" KTAP version 1 1..1 # example: initializing suite # example: failed to initialize (-ENODEV) not ok 1 example KTAP version 1 1..1 # example: initializing suite KTAP version 1 # Subtest: example # module: kunit_example_test 1..1 # example_simple_test: initializing # example_simple_test: cleaning up ok 1 example_simple_test # example: exiting suite ok 1 exampleReplace hardcoded indent in kunit_printk() with flexible indentation based on the argument type (test or suite):
KTAP version 1 1..1 # example: initializing suite # example: failed to initialize (-ENODEV) not ok 1 example KTAP version 1 1..1 # example: initializing suite KTAP version 1 # Subtest: example # module: kunit_example_test 1..1 # example_simple_test: initializing # example_simple_test: cleaning up ok 1 example_simple_test # example: exiting suite ok 1 exampleHi!
I am happy to see this change to improve the indentation of parameterized tests. It has been bugging me for a bit. This seems to be working well but I just had a few comments to potentially discuss.
Thanks!
-Rae
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Cc: David Gow davidgow@google.com Cc: Rae Moar rmoar@google.com
include/kunit/test.h | 24 ++++++++++++++++++++++-- lib/kunit/test.c | 7 ------- 2 files changed, 22 insertions(+), 9 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index 20ed9f9275c9..158876c4cb43 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -509,6 +509,21 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt, kunit_try_catch_throw(&((test_or_suite)->try_catch)); \ } while (0)
+/* Currently supported test levels */ +enum {
KUNIT_LEVEL_SUITE = 0,KUNIT_LEVEL_CASE,KUNIT_LEVEL_CASE_PARAM,+};
I do find this slightly confusing to have a KUNIT_LEVEL_SUITE as the suite output occurs on multiple levels. Plus, I also don't see any uses of the SUITE level or the PARAM level anymore. Do you have any ideas on this?
This enum was just promoted as-is from the test.c as I didn't want to use magic 0 or 1 in below kunit_level() macro.
Note that the KUNIT_LEVEL_SUITE is now used indirectly whenever you call any of kunit_printk() with suite param like here:
./kunit-example-test.c:60: kunit_info(suite, "initializing suite\n"); ./kunit-example-test.c:71: kunit_info(suite, "exiting suite\n");
as this will result in calls to kunit_indent_level(suite) and kunit_level(suite) macro.
And KUNIT_LEVEL_CASE_PARAM is maybe a leftover, as now we change test levels using direct increase/decrease statements, but still IMO this enum nicely reflects what kind of levels we currently do support at this point. But could be dropped if needed.
Regarding _"suite output occurs on multiple levels"_ I found that also confusing, but IMO this is due to the kunit implementation details and/or KTAP design decisions as it looks that "suite" is treated sometimes as "subtest" and sometimes as a top level "test".
That makes a little trouble while trying to implement printing in a consistent manner. Maybe we should consider introducing concept of the "executor" with its own level attribute? Then mapping will be like this:
KTAP version 1 <- kunit executor (level=0) 1..1 <- kunit executor (level=0) # Test: example <- kunit executor (level=0) ?? # module: kunit_example_test <- kunit executor (level=0) ?? # example: initializing suite <- suite (test level=0) KTAP version 1 <- suite executor (level=1) # Subtest: example <- suite executor (level=1) # module: kunit_example_test <- suite executor (level=1) 1..2 <- suite executor (level=1) # test_1: initializing <- test_case (test level=1) # test_1: cleaning up <- test_case (test level=1) # test_1: pass:1 fail:0 skip:0 tota <- suite executor (level=1) ok 1 test_1 <- suite executor (level=1) KTAP version 1 <- params executor (level=2) # Subtest: test_2 <- params executor (level=2) 1..2 <- params executor (level=2) # test_2: initializing <- test_case (test level=2) # test_2: cleaning up <- test_case (test level=2) ok 1 example value 1 <- params executor (level=2) # test_2: initializing <- test_case (test level=2) # test_2: cleaning up <- test_case (test level=2) ok 2 example value 0 <- params executor (level=2) # test_2: pass:2 fail:0 skip:0 tota <- suite executor (level=1) ok 2 test_2 <- suite executor (level=1) # example: exiting suite <- suite (test level=0) # example: pass:2 fail:0 skip:0 total:2 <- kunit executor (level=0) # Totals: pass:3 fail:0 skip:0 total:3 <- kunit executor (level=0) ok 1 example <- kunit executor (level=0)
Then any suite and/or test level will be just based on executor.level This could be done as follow-up improvement.
We could consider just using the test->level field introduced in the next patch to manage the levels. Or I wouldn't mind keeping this just for clarity?
We still need some definition for initial level, no? And at this point in series, we still need at least two defs.
+#define kunit_level(test_or_suite) \
_Generic((test_or_suite), \struct kunit_suite * : KUNIT_LEVEL_SUITE, \struct kunit * : KUNIT_LEVEL_CASE)I am not super familiar with using _Generic so I would want David's opinion.
Also I can think of cases where it would be helpful to add an option for struct kunit_case *, however, that may be an addition for the future.
I had entry for struct kunit_test_case* but removed that as it was never used in current code (no calls to kunit_log() with test_case)
And then additionally, this macro seems to be only used for the struct kunit * case. Will we plan to use this in the future for suites?
As pointed above we already use it for test and suite:
./kunit-example-test.c:60: kunit_info(suite, "initializing suite\n"); ./kunit-example-test.c:71: kunit_info(suite, "exiting suite\n");
+#define kunit_indent_level(test_or_suite) \
(KUNIT_INDENT_LEN * kunit_level(test_or_suite))/*
- 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.
@@ -520,9 +535,14 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt, ##__VA_ARGS__); \ } while (0)
+#define kunit_log_indent(lvl, test_or_suite, fmt, ...) \
kunit_log(lvl, test_or_suite, "%*s" fmt, \kunit_indent_level(test_or_suite), "", \##__VA_ARGS__)#define kunit_printk(lvl, test, fmt, ...) \
kunit_log(lvl, test, KUNIT_SUBTEST_INDENT "# %s: " fmt, \(test)->name, ##__VA_ARGS__)
kunit_log_indent(lvl, test, "# %s: " fmt, \(test)->name, ##__VA_ARGS__)I wonder if we could consider removing the need to use KUNIT_SUBTEST_INDENT in all locations. I am primarily thinking about its uses in debugfs.c and test.c.
For example in debugfs.c: pr_info(KUNIT_SUBTEST_INDENT "KTAP version 1\n");
Although, as this is a suite object that is printing at the test case level, I am not quite sure how to use the existing macros.
We could add some wrapper like kunit_pr() that takes suite/test/executor from which we can derive right indent level, but that would be another follow up task once we agree on location and use of .level attribute.
Michal
/**
- kunit_info() - Prints an INFO level message associated with @test.
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index fb5981ce578d..d10e6d610e20 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -135,13 +135,6 @@ size_t kunit_suite_num_test_cases(struct kunit_suite *suite) } EXPORT_SYMBOL_GPL(kunit_suite_num_test_cases);
-/* Currently supported test levels */ -enum {
KUNIT_LEVEL_SUITE = 0,KUNIT_LEVEL_CASE,KUNIT_LEVEL_CASE_PARAM,-};
static void kunit_print_suite_start(struct kunit_suite *suite) { /* -- 2.25.1
On Mon, Oct 2, 2023 at 9:42 AM Michal Wajdeczko michal.wajdeczko@intel.com wrote:
On 28.09.2023 22:52, Rae Moar wrote:
On Mon, Sep 25, 2023 at 1:58 PM Michal Wajdeczko michal.wajdeczko@intel.com wrote:
A kunit suite is a top level test from the KTAP point of view but all suite diagnostic messages are printed at the subtest level:
$ ./tools/testing/kunit/kunit.py run --raw_output \ --kunitconfig ./lib/kunit/.kunitconfig "example.*simple*" KTAP version 1 1..1 # example: initializing suite # example: failed to initialize (-ENODEV) not ok 1 example KTAP version 1 1..1 # example: initializing suite KTAP version 1 # Subtest: example # module: kunit_example_test 1..1 # example_simple_test: initializing # example_simple_test: cleaning up ok 1 example_simple_test # example: exiting suite ok 1 exampleReplace hardcoded indent in kunit_printk() with flexible indentation based on the argument type (test or suite):
KTAP version 1 1..1 # example: initializing suite # example: failed to initialize (-ENODEV) not ok 1 example KTAP version 1 1..1 # example: initializing suite KTAP version 1 # Subtest: example # module: kunit_example_test 1..1 # example_simple_test: initializing # example_simple_test: cleaning up ok 1 example_simple_test # example: exiting suite ok 1 exampleHi!
I am happy to see this change to improve the indentation of parameterized tests. It has been bugging me for a bit. This seems to be working well but I just had a few comments to potentially discuss.
Thanks!
-Rae
Hello!
Thanks for getting back to me.
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Cc: David Gow davidgow@google.com Cc: Rae Moar rmoar@google.com
include/kunit/test.h | 24 ++++++++++++++++++++++-- lib/kunit/test.c | 7 ------- 2 files changed, 22 insertions(+), 9 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index 20ed9f9275c9..158876c4cb43 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -509,6 +509,21 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt, kunit_try_catch_throw(&((test_or_suite)->try_catch)); \ } while (0)
+/* Currently supported test levels */ +enum {
KUNIT_LEVEL_SUITE = 0,KUNIT_LEVEL_CASE,KUNIT_LEVEL_CASE_PARAM,+};
I do find this slightly confusing to have a KUNIT_LEVEL_SUITE as the suite output occurs on multiple levels. Plus, I also don't see any uses of the SUITE level or the PARAM level anymore. Do you have any ideas on this?
This enum was just promoted as-is from the test.c as I didn't want to use magic 0 or 1 in below kunit_level() macro.
Note that the KUNIT_LEVEL_SUITE is now used indirectly whenever you call any of kunit_printk() with suite param like here:
./kunit-example-test.c:60: kunit_info(suite, "initializing suite\n"); ./kunit-example-test.c:71: kunit_info(suite, "exiting suite\n");
Oops sorry for missing this instance.
as this will result in calls to kunit_indent_level(suite) and kunit_level(suite) macro.
And KUNIT_LEVEL_CASE_PARAM is maybe a leftover, as now we change test levels using direct increase/decrease statements, but still IMO this enum nicely reflects what kind of levels we currently do support at this point. But could be dropped if needed.
Given the suite level is being used, I am on board with keeping the enum as is. Plus, although the param level is not explicitly being used, it does correspond with the value of the test->level field when running parameterized tests.
Regarding _"suite output occurs on multiple levels"_ I found that also confusing, but IMO this is due to the kunit implementation details and/or KTAP design decisions as it looks that "suite" is treated sometimes as "subtest" and sometimes as a top level "test".
That makes a little trouble while trying to implement printing in a consistent manner. Maybe we should consider introducing concept of the "executor" with its own level attribute? Then mapping will be like this:
KTAP version 1 <- kunit executor (level=0) 1..1 <- kunit executor (level=0) # Test: example <- kunit executor (level=0) ?? # module: kunit_example_test <- kunit executor (level=0) ?? # example: initializing suite <- suite (test level=0) KTAP version 1 <- suite executor (level=1) # Subtest: example <- suite executor (level=1) # module: kunit_example_test <- suite executor (level=1) 1..2 <- suite executor (level=1) # test_1: initializing <- test_case (test level=1) # test_1: cleaning up <- test_case (test level=1) # test_1: pass:1 fail:0 skip:0 tota <- suite executor (level=1) ok 1 test_1 <- suite executor (level=1) KTAP version 1 <- params executor (level=2) # Subtest: test_2 <- params executor (level=2) 1..2 <- params executor (level=2) # test_2: initializing <- test_case (test level=2) # test_2: cleaning up <- test_case (test level=2) ok 1 example value 1 <- params executor (level=2) # test_2: initializing <- test_case (test level=2) # test_2: cleaning up <- test_case (test level=2) ok 2 example value 0 <- params executor (level=2) # test_2: pass:2 fail:0 skip:0 tota <- suite executor (level=1) ok 2 test_2 <- suite executor (level=1) # example: exiting suite <- suite (test level=0) # example: pass:2 fail:0 skip:0 total:2 <- kunit executor (level=0) # Totals: pass:3 fail:0 skip:0 total:3 <- kunit executor (level=0) ok 1 example <- kunit executor (level=0)
Then any suite and/or test level will be just based on executor.level This could be done as follow-up improvement.
I like this concept of a general executor or test object. I would be interested in David's opinion on this.
However, this may be a future task for when we change the overall KUnit test implementation to be more general (remove the strict concept of suite vs test case).
Perhaps for now we should keep your current implementation.
We could consider just using the test->level field introduced in the next patch to manage the levels. Or I wouldn't mind keeping this just for clarity?
We still need some definition for initial level, no? And at this point in series, we still need at least two defs.
To use the test->level field we would need to alter the overall KUnit implementation to use a general test structure. Since that is not our current structure, let's keep the original design.
+#define kunit_level(test_or_suite) \
_Generic((test_or_suite), \struct kunit_suite * : KUNIT_LEVEL_SUITE, \struct kunit * : KUNIT_LEVEL_CASE)I am not super familiar with using _Generic so I would want David's opinion.
Also I can think of cases where it would be helpful to add an option for struct kunit_case *, however, that may be an addition for the future.
I had entry for struct kunit_test_case* but removed that as it was never used in current code (no calls to kunit_log() with test_case)
That seems fine to me. We can always add this in later.
And then additionally, this macro seems to be only used for the struct kunit * case. Will we plan to use this in the future for suites?
As pointed above we already use it for test and suite:
./kunit-example-test.c:60: kunit_info(suite, "initializing suite\n"); ./kunit-example-test.c:71: kunit_info(suite, "exiting suite\n");
+#define kunit_indent_level(test_or_suite) \
(KUNIT_INDENT_LEN * kunit_level(test_or_suite))/*
- 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.
@@ -520,9 +535,14 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt, ##__VA_ARGS__); \ } while (0)
+#define kunit_log_indent(lvl, test_or_suite, fmt, ...) \
kunit_log(lvl, test_or_suite, "%*s" fmt, \kunit_indent_level(test_or_suite), "", \##__VA_ARGS__)#define kunit_printk(lvl, test, fmt, ...) \
kunit_log(lvl, test, KUNIT_SUBTEST_INDENT "# %s: " fmt, \(test)->name, ##__VA_ARGS__)
kunit_log_indent(lvl, test, "# %s: " fmt, \(test)->name, ##__VA_ARGS__)I wonder if we could consider removing the need to use KUNIT_SUBTEST_INDENT in all locations. I am primarily thinking about its uses in debugfs.c and test.c.
For example in debugfs.c: pr_info(KUNIT_SUBTEST_INDENT "KTAP version 1\n");
Although, as this is a suite object that is printing at the test case level, I am not quite sure how to use the existing macros.
We could add some wrapper like kunit_pr() that takes suite/test/executor from which we can derive right indent level, but that would be another follow up task once we agree on location and use of .level attribute.
I think it would be great to offer the kunit_pr option or some general macro for formatting given the level.
Thanks! -Rae
Michal
/**
- kunit_info() - Prints an INFO level message associated with @test.
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index fb5981ce578d..d10e6d610e20 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -135,13 +135,6 @@ size_t kunit_suite_num_test_cases(struct kunit_suite *suite) } EXPORT_SYMBOL_GPL(kunit_suite_num_test_cases);
-/* Currently supported test levels */ -enum {
KUNIT_LEVEL_SUITE = 0,KUNIT_LEVEL_CASE,KUNIT_LEVEL_CASE_PARAM,-};
static void kunit_print_suite_start(struct kunit_suite *suite) { /* -- 2.25.1
On Tue, 26 Sept 2023 at 01:58, Michal Wajdeczko michal.wajdeczko@intel.com wrote:
A kunit suite is a top level test from the KTAP point of view but all suite diagnostic messages are printed at the subtest level:
$ ./tools/testing/kunit/kunit.py run --raw_output \ --kunitconfig ./lib/kunit/.kunitconfig "example.*simple*" KTAP version 1 1..1 # example: initializing suite # example: failed to initialize (-ENODEV) not ok 1 example KTAP version 1 1..1 # example: initializing suite KTAP version 1 # Subtest: example # module: kunit_example_test 1..1 # example_simple_test: initializing # example_simple_test: cleaning up ok 1 example_simple_test # example: exiting suite ok 1 exampleReplace hardcoded indent in kunit_printk() with flexible indentation based on the argument type (test or suite):
KTAP version 1 1..1 # example: initializing suite # example: failed to initialize (-ENODEV) not ok 1 example KTAP version 1 1..1 # example: initializing suite KTAP version 1 # Subtest: example # module: kunit_example_test 1..1 # example_simple_test: initializing # example_simple_test: cleaning up ok 1 example_simple_test # example: exiting suite ok 1 exampleSigned-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Cc: David Gow davidgow@google.com Cc: Rae Moar rmoar@google.com
I think this is looking pretty good overall. It'll need rebasing on the current kselftest/kunit branch, though.
As Rae points out, some of this will probably need reworking if we start to support more arbitrary nesting. Equally, we probably need a name for the 'top level' container (of which suites are effectively subtests). To add to the name bikeshedding, while 'executor' works well enough, I think the (K)TAP spec refers to this as the 'test document'. Is that right, Rae?
include/kunit/test.h | 24 ++++++++++++++++++++++-- lib/kunit/test.c | 7 ------- 2 files changed, 22 insertions(+), 9 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index 20ed9f9275c9..158876c4cb43 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -509,6 +509,21 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt, kunit_try_catch_throw(&((test_or_suite)->try_catch)); \ } while (0)
+/* Currently supported test levels */ +enum {
KUNIT_LEVEL_SUITE = 0,KUNIT_LEVEL_CASE,KUNIT_LEVEL_CASE_PARAM,+};
+#define kunit_level(test_or_suite) \
_Generic((test_or_suite), \struct kunit_suite * : KUNIT_LEVEL_SUITE, \struct kunit * : KUNIT_LEVEL_CASE)+#define kunit_indent_level(test_or_suite) \
(KUNIT_INDENT_LEN * kunit_level(test_or_suite))
This looks good to me. I like the use of _Generic() -- I suspect there might be one or two other places in the KUnit code it could be useful in the future.
I don't think we need to handle kunit_case here: once a test is actually being run (and therefore able to log anything), it's already got a struct kunit*.
/*
- 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.
@@ -520,9 +535,14 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt, ##__VA_ARGS__); \ } while (0)
+#define kunit_log_indent(lvl, test_or_suite, fmt, ...) \
kunit_log(lvl, test_or_suite, "%*s" fmt, \kunit_indent_level(test_or_suite), "", \##__VA_ARGS__)#define kunit_printk(lvl, test, fmt, ...) \
kunit_log(lvl, test, KUNIT_SUBTEST_INDENT "# %s: " fmt, \(test)->name, ##__VA_ARGS__)
kunit_log_indent(lvl, test, "# %s: " fmt, \(test)->name, ##__VA_ARGS__)/**
- kunit_info() - Prints an INFO level message associated with @test.
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index fb5981ce578d..d10e6d610e20 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -135,13 +135,6 @@ size_t kunit_suite_num_test_cases(struct kunit_suite *suite) } EXPORT_SYMBOL_GPL(kunit_suite_num_test_cases);
-/* Currently supported test levels */ -enum {
KUNIT_LEVEL_SUITE = 0,KUNIT_LEVEL_CASE,KUNIT_LEVEL_CASE_PARAM,-};
static void kunit_print_suite_start(struct kunit_suite *suite) { /* -- 2.25.1
When running parametrized test cases, diagnostic messages are not properly aligned with the test result lines:
$ ./tools/testing/kunit/kunit.py run --raw_output \ --kunitconfig ./lib/kunit/.kunitconfig *.example_params*
KTAP version 1 1..1 # example: initializing suite KTAP version 1 # Subtest: example # module: kunit_example_test 1..1 KTAP version 1 # Subtest: example_params_test # example_params_test: initializing # example_params_test: cleaning up ok 1 example value 3 # SKIP unsupported param value 3 # example_params_test: initializing # example_params_test: cleaning up ok 2 example value 2 # example_params_test: initializing # example_params_test: cleaning up ok 3 example value 1 # example_params_test: initializing # example_params_test: cleaning up ok 4 example value 0 # SKIP unsupported param value 0 # example_params_test: pass:2 fail:0 skip:2 total:4 ok 1 example_params_test # example: exiting suite # Totals: pass:2 fail:0 skip:2 total:4 ok 1 example
Add test level attribute and use it to generate proper indent at the runtime:
KTAP version 1 1..1 # example: initializing suite KTAP version 1 # Subtest: example # module: kunit_example_test 1..1 KTAP version 1 # Subtest: example_params_test # example_params_test: initializing # example_params_test: cleaning up ok 1 example value 3 # SKIP unsupported param value 3 # example_params_test: initializing # example_params_test: cleaning up ok 2 example value 2 # example_params_test: initializing # example_params_test: cleaning up ok 3 example value 1 # example_params_test: initializing # example_params_test: cleaning up ok 4 example value 0 # SKIP unsupported param value 0 # example_params_test: pass:2 fail:0 skip:2 total:4 ok 1 example_params_test # example: exiting suite # Totals: pass:2 fail:0 skip:2 total:4 ok 1 example
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Cc: David Gow davidgow@google.com Cc: Rae Moar rmoar@google.com --- include/kunit/test.h | 3 ++- lib/kunit/test.c | 52 ++++++++++++++++++++------------------------ 2 files changed, 26 insertions(+), 29 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index 158876c4cb43..4804d539e10f 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -276,6 +276,7 @@ struct kunit { void *priv;
/* private: internal use only. */ + unsigned int level; /* Helps in proper log indent */ const char *name; /* Read only after initialization! */ struct string_stream *log; /* Points at case log after initialization */ struct kunit_try_catch try_catch; @@ -519,7 +520,7 @@ enum { #define kunit_level(test_or_suite) \ _Generic((test_or_suite), \ struct kunit_suite * : KUNIT_LEVEL_SUITE, \ - struct kunit * : KUNIT_LEVEL_CASE) + struct kunit * : ((struct kunit *)(test_or_suite))->level)
#define kunit_indent_level(test_or_suite) \ (KUNIT_INDENT_LEN * kunit_level(test_or_suite)) diff --git a/lib/kunit/test.c b/lib/kunit/test.c index d10e6d610e20..43c3efc286e4 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -99,14 +99,13 @@ static void kunit_print_test_stats(struct kunit *test, if (!kunit_should_print_stats(stats)) return;
- kunit_log(KERN_INFO, test, - KUNIT_SUBTEST_INDENT - "# %s: pass:%lu fail:%lu skip:%lu total:%lu", - test->name, - stats.passed, - stats.failed, - stats.skipped, - stats.total); + kunit_log_indent(KERN_INFO, test, + "# %s: pass:%lu fail:%lu skip:%lu total:%lu", + test->name, + stats.passed, + stats.failed, + stats.skipped, + stats.total); }
/* Append formatted message to log. */ @@ -154,7 +153,6 @@ static void kunit_print_suite_start(struct kunit_suite *suite) }
static void kunit_print_ok_not_ok(struct kunit *test, - unsigned int test_level, enum kunit_status status, size_t test_number, const char *description, @@ -163,12 +161,6 @@ static void kunit_print_ok_not_ok(struct kunit *test, const char *directive_header = (status == KUNIT_SKIPPED) ? " # SKIP " : ""; const char *directive_body = (status == KUNIT_SKIPPED) ? directive : "";
- /* - * When test is NULL assume that results are from the suite - * and today suite results are expected at level 0 only. - */ - WARN(!test && test_level, "suite test level can't be %u!\n", test_level); - /* * We do not log the test suite results as doing so would * mean debugfs display would consist of an incorrect test @@ -182,12 +174,11 @@ static void kunit_print_ok_not_ok(struct kunit *test, test_number, description, directive_header, directive_body); else - kunit_log(KERN_INFO, test, - "%*s%s %zd %s%s%s", - KUNIT_INDENT_LEN * test_level, "", - kunit_status_to_ok_not_ok(status), - test_number, description, directive_header, - directive_body); + kunit_log_indent(KERN_INFO, test, + "%s %zd %s%s%s", + kunit_status_to_ok_not_ok(status), + test_number, description, directive_header, + directive_body); }
enum kunit_status kunit_suite_has_succeeded(struct kunit_suite *suite) @@ -213,7 +204,7 @@ static size_t kunit_suite_counter = 1;
static void kunit_print_suite_end(struct kunit_suite *suite) { - kunit_print_ok_not_ok(NULL, KUNIT_LEVEL_SUITE, + kunit_print_ok_not_ok(NULL, kunit_suite_has_succeeded(suite), kunit_suite_counter++, suite->name, @@ -322,6 +313,7 @@ void kunit_init_test(struct kunit *test, const char *name, struct string_stream { spin_lock_init(&test->lock); INIT_LIST_HEAD(&test->resources); + test->level = KUNIT_LEVEL_CASE; test->name = name; test->log = log; if (test->log) @@ -584,14 +576,15 @@ int kunit_run_tests(struct kunit_suite *suite) kunit_run_case_catch_errors(suite, test_case, &test); kunit_update_stats(¶m_stats, test.status); } else { + /* Parameterized test is one level up from simple test-case. */ + test.level++; + /* Get initial param. */ param_desc[0] = '\0'; test.param_value = test_case->generate_params(NULL, param_desc); test_case->status = KUNIT_SKIPPED; - kunit_log(KERN_INFO, &test, KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT - "KTAP version 1\n"); - kunit_log(KERN_INFO, &test, KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT - "# Subtest: %s", test_case->name); + kunit_log_indent(KERN_INFO, &test, "KTAP version 1\n"); + kunit_log_indent(KERN_INFO, &test, "# Subtest: %s", test_case->name);
while (test.param_value) { kunit_run_case_catch_errors(suite, test_case, &test); @@ -601,7 +594,7 @@ int kunit_run_tests(struct kunit_suite *suite) "param-%d", test.param_index); }
- kunit_print_ok_not_ok(&test, KUNIT_LEVEL_CASE_PARAM, + kunit_print_ok_not_ok(&test, test.status, test.param_index + 1, param_desc, @@ -616,13 +609,16 @@ int kunit_run_tests(struct kunit_suite *suite) test.status = KUNIT_SUCCESS; test.status_comment[0] = '\0'; } + + /* Return to parent (test-case) level. */ + test.level--; }
kunit_print_attr((void *)test_case, true, KUNIT_LEVEL_CASE);
kunit_print_test_stats(&test, param_stats);
- kunit_print_ok_not_ok(&test, KUNIT_LEVEL_CASE, test_case->status, + kunit_print_ok_not_ok(&test, test_case->status, kunit_test_case_num(suite, test_case), test_case->name, test.status_comment);
On Mon, Sep 25, 2023 at 1:58 PM Michal Wajdeczko michal.wajdeczko@intel.com wrote:
When running parametrized test cases, diagnostic messages are not properly aligned with the test result lines:
$ ./tools/testing/kunit/kunit.py run --raw_output \ --kunitconfig ./lib/kunit/.kunitconfig *.example_params* KTAP version 1 1..1 # example: initializing suite KTAP version 1 # Subtest: example # module: kunit_example_test 1..1 KTAP version 1 # Subtest: example_params_test # example_params_test: initializing # example_params_test: cleaning up ok 1 example value 3 # SKIP unsupported param value 3 # example_params_test: initializing # example_params_test: cleaning up ok 2 example value 2 # example_params_test: initializing # example_params_test: cleaning up ok 3 example value 1 # example_params_test: initializing # example_params_test: cleaning up ok 4 example value 0 # SKIP unsupported param value 0 # example_params_test: pass:2 fail:0 skip:2 total:4 ok 1 example_params_test # example: exiting suite # Totals: pass:2 fail:0 skip:2 total:4 ok 1 exampleAdd test level attribute and use it to generate proper indent at the runtime:
KTAP version 1 1..1 # example: initializing suite KTAP version 1 # Subtest: example # module: kunit_example_test 1..1 KTAP version 1 # Subtest: example_params_test # example_params_test: initializing # example_params_test: cleaning up ok 1 example value 3 # SKIP unsupported param value 3 # example_params_test: initializing # example_params_test: cleaning up ok 2 example value 2 # example_params_test: initializing # example_params_test: cleaning up ok 3 example value 1 # example_params_test: initializing # example_params_test: cleaning up ok 4 example value 0 # SKIP unsupported param value 0 # example_params_test: pass:2 fail:0 skip:2 total:4 ok 1 example_params_test # example: exiting suite # Totals: pass:2 fail:0 skip:2 total:4 ok 1 exampleSigned-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Cc: David Gow davidgow@google.com Cc: Rae Moar rmoar@google.com
Hello!
Great to see these changes! Just a few comments below.
Thanks! -Rae
include/kunit/test.h | 3 ++- lib/kunit/test.c | 52 ++++++++++++++++++++------------------------ 2 files changed, 26 insertions(+), 29 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index 158876c4cb43..4804d539e10f 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -276,6 +276,7 @@ struct kunit { void *priv;
/* private: internal use only. */
unsigned int level; /* Helps in proper log indent */ const char *name; /* Read only after initialization! */ struct string_stream *log; /* Points at case log after initialization */ struct kunit_try_catch try_catch;@@ -519,7 +520,7 @@ enum { #define kunit_level(test_or_suite) \ _Generic((test_or_suite), \ struct kunit_suite * : KUNIT_LEVEL_SUITE, \
struct kunit * : KUNIT_LEVEL_CASE)
struct kunit * : ((struct kunit *)(test_or_suite))->level)#define kunit_indent_level(test_or_suite) \ (KUNIT_INDENT_LEN * kunit_level(test_or_suite)) diff --git a/lib/kunit/test.c b/lib/kunit/test.c index d10e6d610e20..43c3efc286e4 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -99,14 +99,13 @@ static void kunit_print_test_stats(struct kunit *test, if (!kunit_should_print_stats(stats)) return;
kunit_log(KERN_INFO, test,KUNIT_SUBTEST_INDENT"# %s: pass:%lu fail:%lu skip:%lu total:%lu",test->name,stats.passed,stats.failed,stats.skipped,stats.total);
kunit_log_indent(KERN_INFO, test,"# %s: pass:%lu fail:%lu skip:%lu total:%lu",test->name,stats.passed,stats.failed,stats.skipped,stats.total);
I would prefer if we keep the same indentation level as before. Otherwise looks good.
}
/* Append formatted message to log. */ @@ -154,7 +153,6 @@ static void kunit_print_suite_start(struct kunit_suite *suite) }
static void kunit_print_ok_not_ok(struct kunit *test,
unsigned int test_level, enum kunit_status status, size_t test_number, const char *description,@@ -163,12 +161,6 @@ static void kunit_print_ok_not_ok(struct kunit *test, const char *directive_header = (status == KUNIT_SKIPPED) ? " # SKIP " : ""; const char *directive_body = (status == KUNIT_SKIPPED) ? directive : "";
/** When test is NULL assume that results are from the suite* and today suite results are expected at level 0 only.*/WARN(!test && test_level, "suite test level can't be %u!\n", test_level);/* * We do not log the test suite results as doing so would * mean debugfs display would consist of an incorrect test@@ -182,12 +174,11 @@ static void kunit_print_ok_not_ok(struct kunit *test, test_number, description, directive_header, directive_body); else
kunit_log(KERN_INFO, test,"%*s%s %zd %s%s%s",KUNIT_INDENT_LEN * test_level, "",kunit_status_to_ok_not_ok(status),test_number, description, directive_header,directive_body);
kunit_log_indent(KERN_INFO, test,"%s %zd %s%s%s",kunit_status_to_ok_not_ok(status),test_number, description, directive_header,directive_body);
Again, I would prefer we keep the same indentation as before as it matches the rest of the file.
}
enum kunit_status kunit_suite_has_succeeded(struct kunit_suite *suite) @@ -213,7 +204,7 @@ static size_t kunit_suite_counter = 1;
static void kunit_print_suite_end(struct kunit_suite *suite) {
kunit_print_ok_not_ok(NULL, KUNIT_LEVEL_SUITE,
kunit_print_ok_not_ok(NULL, kunit_suite_has_succeeded(suite), kunit_suite_counter++, suite->name,@@ -322,6 +313,7 @@ void kunit_init_test(struct kunit *test, const char *name, struct string_stream { spin_lock_init(&test->lock); INIT_LIST_HEAD(&test->resources);
test->level = KUNIT_LEVEL_CASE; test->name = name; test->log = log; if (test->log)@@ -584,14 +576,15 @@ int kunit_run_tests(struct kunit_suite *suite) kunit_run_case_catch_errors(suite, test_case, &test); kunit_update_stats(¶m_stats, test.status); } else {
/* Parameterized test is one level up from simple test-case. */test.level++;/* Get initial param. */ param_desc[0] = '\0'; test.param_value = test_case->generate_params(NULL, param_desc); test_case->status = KUNIT_SKIPPED;
kunit_log(KERN_INFO, &test, KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT"KTAP version 1\n");kunit_log(KERN_INFO, &test, KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT"# Subtest: %s", test_case->name);
kunit_log_indent(KERN_INFO, &test, "KTAP version 1\n");kunit_log_indent(KERN_INFO, &test, "# Subtest: %s", test_case->name); while (test.param_value) { kunit_run_case_catch_errors(suite, test_case, &test);@@ -601,7 +594,7 @@ int kunit_run_tests(struct kunit_suite *suite) "param-%d", test.param_index); }
kunit_print_ok_not_ok(&test, KUNIT_LEVEL_CASE_PARAM,
kunit_print_ok_not_ok(&test, test.status, test.param_index + 1, param_desc,@@ -616,13 +609,16 @@ int kunit_run_tests(struct kunit_suite *suite) test.status = KUNIT_SUCCESS; test.status_comment[0] = '\0'; }
/* Return to parent (test-case) level. */test.level--; } kunit_print_attr((void *)test_case, true, KUNIT_LEVEL_CASE); kunit_print_test_stats(&test, param_stats);
kunit_print_ok_not_ok(&test, KUNIT_LEVEL_CASE, test_case->status,
kunit_print_ok_not_ok(&test, test_case->status, kunit_test_case_num(suite, test_case), test_case->name, test.status_comment);-- 2.25.1
On 28.09.2023 22:53, Rae Moar wrote:
On Mon, Sep 25, 2023 at 1:58 PM Michal Wajdeczko michal.wajdeczko@intel.com wrote:
When running parametrized test cases, diagnostic messages are not properly aligned with the test result lines:
$ ./tools/testing/kunit/kunit.py run --raw_output \ --kunitconfig ./lib/kunit/.kunitconfig *.example_params* KTAP version 1 1..1 # example: initializing suite KTAP version 1 # Subtest: example # module: kunit_example_test 1..1 KTAP version 1 # Subtest: example_params_test # example_params_test: initializing # example_params_test: cleaning up ok 1 example value 3 # SKIP unsupported param value 3 # example_params_test: initializing # example_params_test: cleaning up ok 2 example value 2 # example_params_test: initializing # example_params_test: cleaning up ok 3 example value 1 # example_params_test: initializing # example_params_test: cleaning up ok 4 example value 0 # SKIP unsupported param value 0 # example_params_test: pass:2 fail:0 skip:2 total:4 ok 1 example_params_test # example: exiting suite # Totals: pass:2 fail:0 skip:2 total:4 ok 1 exampleAdd test level attribute and use it to generate proper indent at the runtime:
KTAP version 1 1..1 # example: initializing suite KTAP version 1 # Subtest: example # module: kunit_example_test 1..1 KTAP version 1 # Subtest: example_params_test # example_params_test: initializing # example_params_test: cleaning up ok 1 example value 3 # SKIP unsupported param value 3 # example_params_test: initializing # example_params_test: cleaning up ok 2 example value 2 # example_params_test: initializing # example_params_test: cleaning up ok 3 example value 1 # example_params_test: initializing # example_params_test: cleaning up ok 4 example value 0 # SKIP unsupported param value 0 # example_params_test: pass:2 fail:0 skip:2 total:4 ok 1 example_params_test # example: exiting suite # Totals: pass:2 fail:0 skip:2 total:4 ok 1 exampleSigned-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Cc: David Gow davidgow@google.com Cc: Rae Moar rmoar@google.com
Hello!
Great to see these changes! Just a few comments below.
Thanks! -Rae
include/kunit/test.h | 3 ++- lib/kunit/test.c | 52 ++++++++++++++++++++------------------------ 2 files changed, 26 insertions(+), 29 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index 158876c4cb43..4804d539e10f 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -276,6 +276,7 @@ struct kunit { void *priv;
/* private: internal use only. */
unsigned int level; /* Helps in proper log indent */ const char *name; /* Read only after initialization! */ struct string_stream *log; /* Points at case log after initialization */ struct kunit_try_catch try_catch;@@ -519,7 +520,7 @@ enum { #define kunit_level(test_or_suite) \ _Generic((test_or_suite), \ struct kunit_suite * : KUNIT_LEVEL_SUITE, \
struct kunit * : KUNIT_LEVEL_CASE)
struct kunit * : ((struct kunit *)(test_or_suite))->level)#define kunit_indent_level(test_or_suite) \ (KUNIT_INDENT_LEN * kunit_level(test_or_suite)) diff --git a/lib/kunit/test.c b/lib/kunit/test.c index d10e6d610e20..43c3efc286e4 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -99,14 +99,13 @@ static void kunit_print_test_stats(struct kunit *test, if (!kunit_should_print_stats(stats)) return;
kunit_log(KERN_INFO, test,KUNIT_SUBTEST_INDENT"# %s: pass:%lu fail:%lu skip:%lu total:%lu",test->name,stats.passed,stats.failed,stats.skipped,stats.total);
kunit_log_indent(KERN_INFO, test,"# %s: pass:%lu fail:%lu skip:%lu total:%lu",test->name,stats.passed,stats.failed,stats.skipped,stats.total);I would prefer if we keep the same indentation level as before.
note that then scripts/checkpatch.pl will complain with:
CHECK: Alignment should match open parenthesis #109: FILE: lib/kunit/test.c:103: + kunit_log_indent(KERN_INFO, test, "# %s: pass:%lu fail:%lu skip:%lu total:%lu",
CHECK: Alignment should match open parenthesis #141: FILE: lib/kunit/test.c:178: + kunit_log_indent(KERN_INFO, test, + "%s %zd %s%s%s",
are you ok with that?
Michal
Otherwise looks good.
}
/* Append formatted message to log. */ @@ -154,7 +153,6 @@ static void kunit_print_suite_start(struct kunit_suite *suite) }
static void kunit_print_ok_not_ok(struct kunit *test,
unsigned int test_level, enum kunit_status status, size_t test_number, const char *description,@@ -163,12 +161,6 @@ static void kunit_print_ok_not_ok(struct kunit *test, const char *directive_header = (status == KUNIT_SKIPPED) ? " # SKIP " : ""; const char *directive_body = (status == KUNIT_SKIPPED) ? directive : "";
/** When test is NULL assume that results are from the suite* and today suite results are expected at level 0 only.*/WARN(!test && test_level, "suite test level can't be %u!\n", test_level);/* * We do not log the test suite results as doing so would * mean debugfs display would consist of an incorrect test@@ -182,12 +174,11 @@ static void kunit_print_ok_not_ok(struct kunit *test, test_number, description, directive_header, directive_body); else
kunit_log(KERN_INFO, test,"%*s%s %zd %s%s%s",KUNIT_INDENT_LEN * test_level, "",kunit_status_to_ok_not_ok(status),test_number, description, directive_header,directive_body);
kunit_log_indent(KERN_INFO, test,"%s %zd %s%s%s",kunit_status_to_ok_not_ok(status),test_number, description, directive_header,directive_body);Again, I would prefer we keep the same indentation as before as it matches the rest of the file.
}
enum kunit_status kunit_suite_has_succeeded(struct kunit_suite *suite) @@ -213,7 +204,7 @@ static size_t kunit_suite_counter = 1;
static void kunit_print_suite_end(struct kunit_suite *suite) {
kunit_print_ok_not_ok(NULL, KUNIT_LEVEL_SUITE,
kunit_print_ok_not_ok(NULL, kunit_suite_has_succeeded(suite), kunit_suite_counter++, suite->name,@@ -322,6 +313,7 @@ void kunit_init_test(struct kunit *test, const char *name, struct string_stream { spin_lock_init(&test->lock); INIT_LIST_HEAD(&test->resources);
test->level = KUNIT_LEVEL_CASE; test->name = name; test->log = log; if (test->log)@@ -584,14 +576,15 @@ int kunit_run_tests(struct kunit_suite *suite) kunit_run_case_catch_errors(suite, test_case, &test); kunit_update_stats(¶m_stats, test.status); } else {
/* Parameterized test is one level up from simple test-case. */test.level++;/* Get initial param. */ param_desc[0] = '\0'; test.param_value = test_case->generate_params(NULL, param_desc); test_case->status = KUNIT_SKIPPED;
kunit_log(KERN_INFO, &test, KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT"KTAP version 1\n");kunit_log(KERN_INFO, &test, KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT"# Subtest: %s", test_case->name);
kunit_log_indent(KERN_INFO, &test, "KTAP version 1\n");kunit_log_indent(KERN_INFO, &test, "# Subtest: %s", test_case->name); while (test.param_value) { kunit_run_case_catch_errors(suite, test_case, &test);@@ -601,7 +594,7 @@ int kunit_run_tests(struct kunit_suite *suite) "param-%d", test.param_index); }
kunit_print_ok_not_ok(&test, KUNIT_LEVEL_CASE_PARAM,
kunit_print_ok_not_ok(&test, test.status, test.param_index + 1, param_desc,@@ -616,13 +609,16 @@ int kunit_run_tests(struct kunit_suite *suite) test.status = KUNIT_SUCCESS; test.status_comment[0] = '\0'; }
/* Return to parent (test-case) level. */test.level--; } kunit_print_attr((void *)test_case, true, KUNIT_LEVEL_CASE); kunit_print_test_stats(&test, param_stats);
kunit_print_ok_not_ok(&test, KUNIT_LEVEL_CASE, test_case->status,
kunit_print_ok_not_ok(&test, test_case->status, kunit_test_case_num(suite, test_case), test_case->name, test.status_comment);-- 2.25.1
On Mon, Oct 2, 2023 at 9:43 AM Michal Wajdeczko michal.wajdeczko@intel.com wrote:
On 28.09.2023 22:53, Rae Moar wrote:
On Mon, Sep 25, 2023 at 1:58 PM Michal Wajdeczko michal.wajdeczko@intel.com wrote:
When running parametrized test cases, diagnostic messages are not properly aligned with the test result lines:
$ ./tools/testing/kunit/kunit.py run --raw_output \ --kunitconfig ./lib/kunit/.kunitconfig *.example_params* KTAP version 1 1..1 # example: initializing suite KTAP version 1 # Subtest: example # module: kunit_example_test 1..1 KTAP version 1 # Subtest: example_params_test # example_params_test: initializing # example_params_test: cleaning up ok 1 example value 3 # SKIP unsupported param value 3 # example_params_test: initializing # example_params_test: cleaning up ok 2 example value 2 # example_params_test: initializing # example_params_test: cleaning up ok 3 example value 1 # example_params_test: initializing # example_params_test: cleaning up ok 4 example value 0 # SKIP unsupported param value 0 # example_params_test: pass:2 fail:0 skip:2 total:4 ok 1 example_params_test # example: exiting suite # Totals: pass:2 fail:0 skip:2 total:4 ok 1 exampleAdd test level attribute and use it to generate proper indent at the runtime:
KTAP version 1 1..1 # example: initializing suite KTAP version 1 # Subtest: example # module: kunit_example_test 1..1 KTAP version 1 # Subtest: example_params_test # example_params_test: initializing # example_params_test: cleaning up ok 1 example value 3 # SKIP unsupported param value 3 # example_params_test: initializing # example_params_test: cleaning up ok 2 example value 2 # example_params_test: initializing # example_params_test: cleaning up ok 3 example value 1 # example_params_test: initializing # example_params_test: cleaning up ok 4 example value 0 # SKIP unsupported param value 0 # example_params_test: pass:2 fail:0 skip:2 total:4 ok 1 example_params_test # example: exiting suite # Totals: pass:2 fail:0 skip:2 total:4 ok 1 exampleSigned-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Cc: David Gow davidgow@google.com Cc: Rae Moar rmoar@google.com
Hello!
Great to see these changes! Just a few comments below.
Thanks! -Rae
include/kunit/test.h | 3 ++- lib/kunit/test.c | 52 ++++++++++++++++++++------------------------ 2 files changed, 26 insertions(+), 29 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index 158876c4cb43..4804d539e10f 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -276,6 +276,7 @@ struct kunit { void *priv;
/* private: internal use only. */
unsigned int level; /* Helps in proper log indent */ const char *name; /* Read only after initialization! */ struct string_stream *log; /* Points at case log after initialization */ struct kunit_try_catch try_catch;@@ -519,7 +520,7 @@ enum { #define kunit_level(test_or_suite) \ _Generic((test_or_suite), \ struct kunit_suite * : KUNIT_LEVEL_SUITE, \
struct kunit * : KUNIT_LEVEL_CASE)
struct kunit * : ((struct kunit *)(test_or_suite))->level)#define kunit_indent_level(test_or_suite) \ (KUNIT_INDENT_LEN * kunit_level(test_or_suite)) diff --git a/lib/kunit/test.c b/lib/kunit/test.c index d10e6d610e20..43c3efc286e4 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -99,14 +99,13 @@ static void kunit_print_test_stats(struct kunit *test, if (!kunit_should_print_stats(stats)) return;
kunit_log(KERN_INFO, test,KUNIT_SUBTEST_INDENT"# %s: pass:%lu fail:%lu skip:%lu total:%lu",test->name,stats.passed,stats.failed,stats.skipped,stats.total);
kunit_log_indent(KERN_INFO, test,"# %s: pass:%lu fail:%lu skip:%lu total:%lu",test->name,stats.passed,stats.failed,stats.skipped,stats.total);I would prefer if we keep the same indentation level as before.
note that then scripts/checkpatch.pl will complain with:
CHECK: Alignment should match open parenthesis #109: FILE: lib/kunit/test.c:103:
kunit_log_indent(KERN_INFO, test, "# %s: pass:%lu fail:%lu skip:%lu total:%lu",CHECK: Alignment should match open parenthesis #141: FILE: lib/kunit/test.c:178:
kunit_log_indent(KERN_INFO, test,"%s %zd %s%s%s",are you ok with that?
Michal
Hello!
I understand now. It is unfortunate that the previous indentation causes a checkpatch warning. I suppose KUnit should fix the indentation of the file as a whole in a separate patch then.
Since the issue of the indentation is resolved, I am happy with this current patch.
Thanks! -Rae
Otherwise looks good.
}
/* Append formatted message to log. */ @@ -154,7 +153,6 @@ static void kunit_print_suite_start(struct kunit_suite *suite) }
static void kunit_print_ok_not_ok(struct kunit *test,
unsigned int test_level, enum kunit_status status, size_t test_number, const char *description,@@ -163,12 +161,6 @@ static void kunit_print_ok_not_ok(struct kunit *test, const char *directive_header = (status == KUNIT_SKIPPED) ? " # SKIP " : ""; const char *directive_body = (status == KUNIT_SKIPPED) ? directive : "";
/** When test is NULL assume that results are from the suite* and today suite results are expected at level 0 only.*/WARN(!test && test_level, "suite test level can't be %u!\n", test_level);/* * We do not log the test suite results as doing so would * mean debugfs display would consist of an incorrect test@@ -182,12 +174,11 @@ static void kunit_print_ok_not_ok(struct kunit *test, test_number, description, directive_header, directive_body); else
kunit_log(KERN_INFO, test,"%*s%s %zd %s%s%s",KUNIT_INDENT_LEN * test_level, "",kunit_status_to_ok_not_ok(status),test_number, description, directive_header,directive_body);
kunit_log_indent(KERN_INFO, test,"%s %zd %s%s%s",kunit_status_to_ok_not_ok(status),test_number, description, directive_header,directive_body);Again, I would prefer we keep the same indentation as before as it matches the rest of the file.
}
enum kunit_status kunit_suite_has_succeeded(struct kunit_suite *suite) @@ -213,7 +204,7 @@ static size_t kunit_suite_counter = 1;
static void kunit_print_suite_end(struct kunit_suite *suite) {
kunit_print_ok_not_ok(NULL, KUNIT_LEVEL_SUITE,
kunit_print_ok_not_ok(NULL, kunit_suite_has_succeeded(suite), kunit_suite_counter++, suite->name,@@ -322,6 +313,7 @@ void kunit_init_test(struct kunit *test, const char *name, struct string_stream { spin_lock_init(&test->lock); INIT_LIST_HEAD(&test->resources);
test->level = KUNIT_LEVEL_CASE; test->name = name; test->log = log; if (test->log)@@ -584,14 +576,15 @@ int kunit_run_tests(struct kunit_suite *suite) kunit_run_case_catch_errors(suite, test_case, &test); kunit_update_stats(¶m_stats, test.status); } else {
/* Parameterized test is one level up from simple test-case. */test.level++;/* Get initial param. */ param_desc[0] = '\0'; test.param_value = test_case->generate_params(NULL, param_desc); test_case->status = KUNIT_SKIPPED;
kunit_log(KERN_INFO, &test, KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT"KTAP version 1\n");kunit_log(KERN_INFO, &test, KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT"# Subtest: %s", test_case->name);
kunit_log_indent(KERN_INFO, &test, "KTAP version 1\n");kunit_log_indent(KERN_INFO, &test, "# Subtest: %s", test_case->name); while (test.param_value) { kunit_run_case_catch_errors(suite, test_case, &test);@@ -601,7 +594,7 @@ int kunit_run_tests(struct kunit_suite *suite) "param-%d", test.param_index); }
kunit_print_ok_not_ok(&test, KUNIT_LEVEL_CASE_PARAM,
kunit_print_ok_not_ok(&test, test.status, test.param_index + 1, param_desc,@@ -616,13 +609,16 @@ int kunit_run_tests(struct kunit_suite *suite) test.status = KUNIT_SUCCESS; test.status_comment[0] = '\0'; }
/* Return to parent (test-case) level. */test.level--; } kunit_print_attr((void *)test_case, true, KUNIT_LEVEL_CASE); kunit_print_test_stats(&test, param_stats);
kunit_print_ok_not_ok(&test, KUNIT_LEVEL_CASE, test_case->status,
kunit_print_ok_not_ok(&test, test_case->status, kunit_test_case_num(suite, test_case), test_case->name, test.status_comment);-- 2.25.1
In case of parameterized tests we are not providing a test plan so we can't detect if any result is missing.
Count available params using the same generator as during a test execution
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Cc: David Gow davidgow@google.com Cc: Rae Moar rmoar@google.com --- lib/kunit/test.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index 43c3efc286e4..55eabb324f39 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -540,6 +540,20 @@ static void kunit_accumulate_stats(struct kunit_result_stats *total, total->total += add.total; }
+static size_t count_test_case_params(struct kunit_case *test_case) +{ + char param_desc[KUNIT_PARAM_DESC_SIZE]; + const void *param_value = NULL; + size_t num = 0; + + if (test_case->generate_params) + while ((param_value = test_case->generate_params(param_value, + param_desc))) + num++; + + return num; +} + int kunit_run_tests(struct kunit_suite *suite) { char param_desc[KUNIT_PARAM_DESC_SIZE]; @@ -585,6 +599,8 @@ int kunit_run_tests(struct kunit_suite *suite) test_case->status = KUNIT_SKIPPED; kunit_log_indent(KERN_INFO, &test, "KTAP version 1\n"); kunit_log_indent(KERN_INFO, &test, "# Subtest: %s", test_case->name); + kunit_log_indent(KERN_INFO, &test, "1..%zd\n", + count_test_case_params(test_case));
while (test.param_value) { kunit_run_case_catch_errors(suite, test_case, &test);
On Mon, Sep 25, 2023 at 1:58 PM Michal Wajdeczko michal.wajdeczko@intel.com wrote:
In case of parameterized tests we are not providing a test plan so we can't detect if any result is missing.
Count available params using the same generator as during a test execution
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Cc: David Gow davidgow@google.com Cc: Rae Moar rmoar@google.com
lib/kunit/test.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index 43c3efc286e4..55eabb324f39 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -540,6 +540,20 @@ static void kunit_accumulate_stats(struct kunit_result_stats *total, total->total += add.total; }
+static size_t count_test_case_params(struct kunit_case *test_case) +{
char param_desc[KUNIT_PARAM_DESC_SIZE];const void *param_value = NULL;size_t num = 0;if (test_case->generate_params)while ((param_value = test_case->generate_params(param_value,param_desc)))num++;return num;+}
Hello!
This change largely looks good to me. However, I am not 100 percent confident that the function to generate parameters always produces the same output (or same number of test cases). I would be interested in David's opinion on this.
Otherwise it seems to work well!
Thanks! -Rae
int kunit_run_tests(struct kunit_suite *suite) { char param_desc[KUNIT_PARAM_DESC_SIZE]; @@ -585,6 +599,8 @@ int kunit_run_tests(struct kunit_suite *suite) test_case->status = KUNIT_SKIPPED; kunit_log_indent(KERN_INFO, &test, "KTAP version 1\n"); kunit_log_indent(KERN_INFO, &test, "# Subtest: %s", test_case->name);
kunit_log_indent(KERN_INFO, &test, "1..%zd\n",count_test_case_params(test_case)); while (test.param_value) { kunit_run_case_catch_errors(suite, test_case, &test);-- 2.25.1
On 28.09.2023 22:54, Rae Moar wrote:
On Mon, Sep 25, 2023 at 1:58 PM Michal Wajdeczko michal.wajdeczko@intel.com wrote:
In case of parameterized tests we are not providing a test plan so we can't detect if any result is missing.
Count available params using the same generator as during a test execution
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Cc: David Gow davidgow@google.com Cc: Rae Moar rmoar@google.com
lib/kunit/test.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/lib/kunit/test.c b/lib/kunit/test.c index 43c3efc286e4..55eabb324f39 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -540,6 +540,20 @@ static void kunit_accumulate_stats(struct kunit_result_stats *total, total->total += add.total; }
+static size_t count_test_case_params(struct kunit_case *test_case) +{
char param_desc[KUNIT_PARAM_DESC_SIZE];const void *param_value = NULL;size_t num = 0;if (test_case->generate_params)while ((param_value = test_case->generate_params(param_value,param_desc)))num++;return num;+}
Hello!
This change largely looks good to me. However, I am not 100 percent confident that the function to generate parameters always produces the same output (or same number of test cases). I would be interested in David's opinion on this.
Right, it's not explicitly specified in KUNIT_CASE_PARAM nor test_case.generate_params documentation, but I would assume that while generating different output could be fine (and harmless to this patch), like based on a random seed, but IMO at the same time it should be prohibited to generate different number of params, as this would make harder to compare each execution for regression.
Alternatively we can introduce some flag to indicate whether provided param generator is stable or not and then provide test plan only for the former.
Michal
Otherwise it seems to work well!
Thanks! -Rae
int kunit_run_tests(struct kunit_suite *suite) { char param_desc[KUNIT_PARAM_DESC_SIZE]; @@ -585,6 +599,8 @@ int kunit_run_tests(struct kunit_suite *suite) test_case->status = KUNIT_SKIPPED; kunit_log_indent(KERN_INFO, &test, "KTAP version 1\n"); kunit_log_indent(KERN_INFO, &test, "# Subtest: %s", test_case->name);
kunit_log_indent(KERN_INFO, &test, "1..%zd\n",count_test_case_params(test_case)); while (test.param_value) { kunit_run_case_catch_errors(suite, test_case, &test);-- 2.25.1
On Mon, 2 Oct 2023 at 21:55, Michal Wajdeczko michal.wajdeczko@intel.com wrote:
On 28.09.2023 22:54, Rae Moar wrote:
On Mon, Sep 25, 2023 at 1:58 PM Michal Wajdeczko michal.wajdeczko@intel.com wrote:
In case of parameterized tests we are not providing a test plan so we can't detect if any result is missing.
Count available params using the same generator as during a test execution
Signed-off-by: Michal Wajdeczko michal.wajdeczko@intel.com Cc: David Gow davidgow@google.com Cc: Rae Moar rmoar@google.com
<...snip...>
Hello!
This change largely looks good to me. However, I am not 100 percent confident that the function to generate parameters always produces the same output (or same number of test cases). I would be interested in David's opinion on this.
Right, it's not explicitly specified in KUNIT_CASE_PARAM nor test_case.generate_params documentation, but I would assume that while generating different output could be fine (and harmless to this patch), like based on a random seed, but IMO at the same time it should be prohibited to generate different number of params, as this would make harder to compare each execution for regression.
There are definitely some valid cases for parameterised tests to generate different numbers of tests in different configs / environments (e.g., there are some where the number of parameters depends on the number of CPUs). That being said, it shouldn't be a problem in a relatively standard test environment with any of the tests we currently have.
Some of these issues can be got around by having tests be generated regardless, but having those tests be skipped if required.
Alternatively we can introduce some flag to indicate whether provided param generator is stable or not and then provide test plan only for the former.
I think this sounds like a good idea, and a use for the KUnit attributes system. I'd thought that a 'deterministic' attribute would make sense, but it may make sense to split it into a 'deterministic' and 'fixed structure' flag (the latter only requiring that the number, order, names, etc of tests and subtests be the same).
There have been a couple of people asking for a feature to deliberately randomise test ordering, too. We'd want to clear these flags if that's in use. Of course, ideally anyone doing regression testing would be able to use the test/parameter name/description instead of test number, so ordering of tests shouldn't matter unless tests were buggy.
linux-kselftest-mirror@lists.linaro.org
-
David Gow -
Michal Wajdeczko -
Rae Moar