On 27/10/20 2:33 pm, Marco Elver wrote:
I just tried to give this a spin on some of my tests and noticed some more things (apologies for the multiple rounds of comments):
On Mon, 26 Oct 2020 at 19:36, Arpitha Raghunandan 98.arpi@gmail.com wrote: [...]
/**
- struct kunit_suite - describes a related collection of &struct kunit_case
@@ -208,6 +217,15 @@ struct kunit { const char *name; /* Read only after initialization! */ char *log; /* Points at case log after initialization */ struct kunit_try_catch try_catch;
/* param_values points to test case parameters in parameterized tests */void *param_values;This should be singular, i.e. "param_value", since the generator only generates 1 value for each test. Whether or not that value is a pointer that points to more than 1 value or is an integer etc. is entirely test-dependent.
/** current_param stores the index of the parameter in* the array of parameters in parameterized tests.* current_param + 1 is printed to indicate the parameter* that causes the test to fail in case of test failure.*/int current_param;I think, per your comment above, this should be named "param_index". Also, I would suggest removing the mention of "array" in the comment, because the parameters aren't dependent on use of an array.
/* * success starts as true, and may only be set to false during a * test case; thus, it is safe to update this across multiple@@ -1742,4 +1760,18 @@ do { \ fmt, \ ##__VA_ARGS__)
+/**
- KUNIT_PARAM_GENERATOR() - Helper method for test parameter generators
required in parameterized tests.
- @name: prefix of the name for the test parameter generator function.
- @prev: a pointer to the previous test parameter, NULL for first parameter.
- @array: a user-supplied pointer to an array of test parameters.
- */
+#define KUNIT_PARAM_GENERATOR(name, array) \
static void *name##_gen_params(void *prev) \{ \typeof((array)[0]) * __next = prev ? ((typeof(__next)) prev) + 1 : (array); \return __next - (array) < ARRAY_SIZE((array)) ? __next : NULL; \}#endif /* _KUNIT_TEST_H */ diff --git a/lib/kunit/test.c b/lib/kunit/test.c index 750704abe89a..b70ab9b12f3b 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -127,6 +127,11 @@ unsigned int kunit_test_case_num(struct kunit_suite *suite, } EXPORT_SYMBOL_GPL(kunit_test_case_num);
+static void kunit_print_failed_param(struct kunit *test) +{
kunit_err(test, "\n\tTest failed at parameter: %d\n", test->current_param + 1);+}
Is this the only place where the param index is used? It might be helpful to show the index together with the test-case name, otherwise we get a series of test cases in the output which are all named the same which can be confusing.
Yes, this is the only place param index is used.
static void kunit_print_string_stream(struct kunit *test, struct string_stream *stream) { @@ -168,6 +173,8 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert) assert->format(assert, stream);
kunit_print_string_stream(test, stream);
if (test->param_values)kunit_print_failed_param(test); WARN_ON(string_stream_destroy(stream));} @@ -239,7 +246,18 @@ static void kunit_run_case_internal(struct kunit *test, } }
test_case->run_case(test);
if (!test_case->generate_params) {test_case->run_case(test);} else {test->param_values = test_case->generate_params(NULL);test->current_param = 0;while (test->param_values) {test_case->run_case(test);test->param_values = test_case->generate_params(test->param_values);test->current_param++;}}}
Looking forward to v4. :-)
Thanks, -- Marco
I will make all the suggested changes. Thanks!