On 07/11/20 3:36 pm, Marco Elver wrote:
On Sat, 7 Nov 2020 at 05:58, David Gow davidgow@google.com wrote:
On Sat, Nov 7, 2020 at 3:22 AM Arpitha Raghunandan 98.arpi@gmail.com wrote:
Implementation of support for parameterized testing in KUnit. This approach requires the creation of a test case using the KUNIT_CASE_PARAM macro that accepts a generator function as input. This generator function should return the next parameter given the previous parameter in parameterized tests. It also provides a macro to generate common-case generators.
Signed-off-by: Arpitha Raghunandan 98.arpi@gmail.com Co-developed-by: Marco Elver elver@google.com Signed-off-by: Marco Elver elver@google.com
This looks good to me! A couple of minor thoughts about the output format below, but I'm quite happy to have this as-is regardless.
Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
Changes v5->v6:
- Fix alignment to maintain consistency
Changes v4->v5:
- Update kernel-doc comments.
- Use const void* for generator return and prev value types.
- Add kernel-doc comment for KUNIT_ARRAY_PARAM.
- Rework parameterized test case execution strategy: each parameter is executed as if it was its own test case, with its own test initialization and cleanup (init and exit are called, etc.). However, we cannot add new test cases per TAP protocol once we have already started execution. Instead, log the result of each parameter run as a diagnostic comment.
Changes v3->v4:
- Rename kunit variables
- Rename generator function helper macro
- Add documentation for generator approach
- Display test case name in case of failure along with param index
Changes v2->v3:
- Modifictaion of generator macro and method
Changes v1->v2:
- Use of a generator method to access test case parameters
include/kunit/test.h | 36 ++++++++++++++++++++++++++++++++++ lib/kunit/test.c | 46 +++++++++++++++++++++++++++++++------------- 2 files changed, 69 insertions(+), 13 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h index db1b0ae666c4..16616d3974f9 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -107,6 +107,7 @@ struct kunit;
[...]
kunit_suite_for_each_test_case(suite, test_case)kunit_run_case_catch_errors(suite, test_case);
kunit_suite_for_each_test_case(suite, test_case) {struct kunit test = { .param_value = NULL, .param_index = 0 };bool test_success = true;if (test_case->generate_params)test.param_value = test_case->generate_params(NULL);do {kunit_run_case_catch_errors(suite, test_case, &test);test_success &= test_case->success;if (test_case->generate_params) {kunit_log(KERN_INFO, &test,KUNIT_SUBTEST_INDENT"# %s: param-%d %s",Would it make sense to have this imitate the TAP format a bit more? So, have "# [ok|not ok] - [name]" as the format? [name] could be something like "[test_case->name]:param-[index]" or similar. If we keep it commented out and don't indent it further, it won't formally be a nested test (though if we wanted to support those later, it'd be easy to add), but I think it would be nicer to be consistent here.
The previous attempt [1] at something similar failed because it seems we'd need to teach kunit-tool new tricks [2], too. [1] https://lkml.kernel.org/r/20201105195503.GA2399621@elver.google.com [2] https://lkml.kernel.org/r/20201106123433.GA3563235@elver.google.com
So if we go with a different format, we might need a patch before this one to make kunit-tool compatible with that type of diagnostic.
Currently I think we have the following proposals for a format:
- The current "# [test_case->name]: param-[index] [ok|not ok]" --
this works well, because no changes to kunit-tool are required, and it also picks up the diagnostic context for the case and displays that on test failure.
- Your proposed "# [ok|not ok] - [test_case->name]:param-[index]".
As-is, this needs a patch for kunit-tool as well. I just checked, and if we change it to "# [ok|not ok] - [test_case->name]: param-[index]" (note the space after ':') it works without changing kunit-tool. ;-)
- Something like "# [ok|not ok] param-[index] - [test_case->name]",
which I had played with earlier but kunit-tool is definitely not yet happy with.
So my current preference is (2) with the extra space (no change to kunit-tool required). WDYT?
Which format do we finally go with?
My other suggestion -- albeit one outside the scope of this initial version -- would be to allow the "param-%d" name to be overridden somehow by a test. For example, the ext4 inode test has names for all its test cases: it'd be nice to be able to display those instead (even if they're not formatted as identifiers as-is).
Right, I was thinking about this, but it'd need a way to optionally pass another function that converts const void* params to readable strings. But as you say, we should do that as a follow-up patch later because it might require a few more iterations.
[...]
Thanks, -- Marco