diff mbox series

[1/2] kunit: Add param generator macro for zero terminated arrays

Message ID 20230926220208.1423-1-michal.wajdeczko@intel.com
State New
Headers show
Series [1/2] kunit: Add param generator macro for zero terminated arrays | expand

Commit Message

Michal Wajdeczko Sept. 26, 2023, 10:02 p.m. UTC
The existing macro KUNIT_ARRAY_PARAM can produce parameter
generator function but only when we fully know the definition
of the array. However, there might be cases where we would like
to generate test params based on externaly defined array, which
is defined as zero-terminated array, like pci_driver.id_table.

Add helper macro KUNIT_ZERO_ARRAY_PARAM that can work with zero
terminated arrays and provide example how to use it.

$ ./tools/testing/kunit/kunit.py run \
	--kunitconfig ./lib/kunit/.kunitconfig *.example_params*

[ ] Starting KUnit Kernel (1/1)...
[ ] ============================================================
[ ] ========================= example  =========================
[ ] =================== example_params_test  ===================
[ ] [SKIPPED] example value 3
[ ] [PASSED] example value 2
[ ] [PASSED] example value 1
[ ] [SKIPPED] example value 0
[ ] =============== [PASSED] example_params_test ===============
[ ] =================== example_params_test  ===================
[ ] [SKIPPED] example value 3
[ ] [PASSED] example value 2
[ ] [PASSED] example value 1
[ ] =============== [PASSED] example_params_test ===============
[ ] ===================== [PASSED] example =====================
[ ] ============================================================
[ ] Testing complete. Ran 7 tests: passed: 4, skipped: 3

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           | 22 ++++++++++++++++++++++
 lib/kunit/kunit-example-test.c |  2 ++
 2 files changed, 24 insertions(+)

Comments

Rae Moar Sept. 29, 2023, 7:39 p.m. UTC | #1
On Tue, Sep 26, 2023 at 6:02 PM Michal Wajdeczko
<michal.wajdeczko@intel.com> wrote:
>
> The existing macro KUNIT_ARRAY_PARAM can produce parameter
> generator function but only when we fully know the definition
> of the array. However, there might be cases where we would like
> to generate test params based on externaly defined array, which
> is defined as zero-terminated array, like pci_driver.id_table.
>
> Add helper macro KUNIT_ZERO_ARRAY_PARAM that can work with zero
> terminated arrays and provide example how to use it.
>
> $ ./tools/testing/kunit/kunit.py run \
>         --kunitconfig ./lib/kunit/.kunitconfig *.example_params*
>
> [ ] Starting KUnit Kernel (1/1)...
> [ ] ============================================================
> [ ] ========================= example  =========================
> [ ] =================== example_params_test  ===================
> [ ] [SKIPPED] example value 3
> [ ] [PASSED] example value 2
> [ ] [PASSED] example value 1
> [ ] [SKIPPED] example value 0
> [ ] =============== [PASSED] example_params_test ===============
> [ ] =================== example_params_test  ===================
> [ ] [SKIPPED] example value 3
> [ ] [PASSED] example value 2
> [ ] [PASSED] example value 1
> [ ] =============== [PASSED] example_params_test ===============
> [ ] ===================== [PASSED] example =====================
> [ ] ============================================================
> [ ] Testing complete. Ran 7 tests: passed: 4, skipped: 3
>
> 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           | 22 ++++++++++++++++++++++
>  lib/kunit/kunit-example-test.c |  2 ++
>  2 files changed, 24 insertions(+)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 20ed9f9275c9..280113ceb6a6 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -1514,6 +1514,28 @@ do {                                                                            \
>                 return NULL;                                                                    \
>         }
>
> +/**
> + * KUNIT_ZERO_ARRAY_PARAM() - Define test parameter generator from a zero terminated array.
> + * @name:  prefix for the test parameter generator function.
> + * @array: zero terminated array of test parameters.
> + * @get_desc: function to convert param to description; NULL to use default
> + *
> + * Define function @name_gen_params which uses zero terminated @array to generate parameters.
> + */
> +#define KUNIT_ZERO_ARRAY_PARAM(name, array, get_desc)                                          \
> +       static const void *name##_gen_params(const void *prev, char *desc)                      \
> +       {                                                                                       \
> +               typeof((array)[0]) *__prev = prev;                                              \
> +               typeof(__prev) __next = __prev ? __prev + 1 : (array);                          \
> +               void (*__get_desc)(typeof(__next), char *) = get_desc;                          \
> +               for (; memchr_inv(__next, 0, sizeof(*__next)); __prev = __next++) {             \
> +                       if (__get_desc)                                                         \
> +                               __get_desc(__next, desc);                                       \
> +                       return __next;                                                          \
> +               }                                                                               \
> +               return NULL;                                                                    \
> +       }
> +

Hello!

This overall looks good to me. I am not sure how many uses there are
for zero-terminated arrays for parameterized tests.

However, since it seems to be a well designed feature, I think it
should be good to add this to KUnit.

Thanks for including an example! Just one other comment below.

-Rae

>  // TODO(dlatypov@google.com): consider eventually migrating users to explicitly
>  // include resource.h themselves if they need it.
>  #include <kunit/resource.h>
> diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
> index 6bb5c2ef6696..ad9ebcfd513e 100644
> --- a/lib/kunit/kunit-example-test.c
> +++ b/lib/kunit/kunit-example-test.c
> @@ -202,6 +202,7 @@ static void example_param_get_desc(const struct example_param *p, char *desc)
>  }
>
>  KUNIT_ARRAY_PARAM(example, example_params_array, example_param_get_desc);
> +KUNIT_ZERO_ARRAY_PARAM(example_zero, example_params_array, example_param_get_desc);
>
>  /*
>   * This test shows the use of params.
> @@ -246,6 +247,7 @@ static struct kunit_case example_test_cases[] = {
>         KUNIT_CASE(example_all_expect_macros_test),
>         KUNIT_CASE(example_static_stub_test),
>         KUNIT_CASE_PARAM(example_params_test, example_gen_params),
> +       KUNIT_CASE_PARAM(example_params_test, example_zero_gen_params),

I would prefer if the name of the new params test was a bit different
just to differentiate them. Maybe: example_params_zero_test

>         KUNIT_CASE_SLOW(example_slow_test),
>         {}
>  };
> --
> 2.25.1
>
David Gow Sept. 30, 2023, 8:58 a.m. UTC | #2
On Wed, 27 Sept 2023 at 06:02, Michal Wajdeczko
<michal.wajdeczko@intel.com> wrote:
>
> The existing macro KUNIT_ARRAY_PARAM can produce parameter
> generator function but only when we fully know the definition
> of the array. However, there might be cases where we would like
> to generate test params based on externaly defined array, which
> is defined as zero-terminated array, like pci_driver.id_table.

Hmm... I like the idea of this, but am a little wary of dealing with
zero-terminated arrays in a generic fashion. Some cases (pointers,
where we can just != NULL) are obvious,
but we could hit inconsistencies with things like padding, as things
like pci_driver.id_table seem to mostly be iterated over with things
like:
while (ids->vendor || ids->subvendor || ids->class_mask)

which not only ignores the padding, but also half of the fields. So
there may be a consistency issue there.

Though I suspect it's not likely to cause issues in practice.

Thoughts?
-- David
>
> Add helper macro KUNIT_ZERO_ARRAY_PARAM that can work with zero
> terminated arrays and provide example how to use it.
>
> $ ./tools/testing/kunit/kunit.py run \
>         --kunitconfig ./lib/kunit/.kunitconfig *.example_params*
>
> [ ] Starting KUnit Kernel (1/1)...
> [ ] ============================================================
> [ ] ========================= example  =========================
> [ ] =================== example_params_test  ===================
> [ ] [SKIPPED] example value 3
> [ ] [PASSED] example value 2
> [ ] [PASSED] example value 1
> [ ] [SKIPPED] example value 0
> [ ] =============== [PASSED] example_params_test ===============
> [ ] =================== example_params_test  ===================
> [ ] [SKIPPED] example value 3
> [ ] [PASSED] example value 2
> [ ] [PASSED] example value 1
> [ ] =============== [PASSED] example_params_test ===============
> [ ] ===================== [PASSED] example =====================
> [ ] ============================================================
> [ ] Testing complete. Ran 7 tests: passed: 4, skipped: 3
>
> 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           | 22 ++++++++++++++++++++++
>  lib/kunit/kunit-example-test.c |  2 ++
>  2 files changed, 24 insertions(+)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 20ed9f9275c9..280113ceb6a6 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -1514,6 +1514,28 @@ do {                                                                            \
>                 return NULL;                                                                    \
>         }
>
> +/**
> + * KUNIT_ZERO_ARRAY_PARAM() - Define test parameter generator from a zero terminated array.
> + * @name:  prefix for the test parameter generator function.
> + * @array: zero terminated array of test parameters.
> + * @get_desc: function to convert param to description; NULL to use default
> + *
> + * Define function @name_gen_params which uses zero terminated @array to generate parameters.
> + */
> +#define KUNIT_ZERO_ARRAY_PARAM(name, array, get_desc)                                          \
> +       static const void *name##_gen_params(const void *prev, char *desc)                      \
> +       {                                                                                       \
> +               typeof((array)[0]) *__prev = prev;                                              \
> +               typeof(__prev) __next = __prev ? __prev + 1 : (array);                          \
> +               void (*__get_desc)(typeof(__next), char *) = get_desc;                          \
> +               for (; memchr_inv(__next, 0, sizeof(*__next)); __prev = __next++) {             \

Are there any places where this might interact awkwardly with padding?
I _think_ it should be okay (variables with static lifetimes should
have padding initialised to zero), but there could be a case I'm
missing.


> +                       if (__get_desc)                                                         \
> +                               __get_desc(__next, desc);                                       \
> +                       return __next;                                                          \
> +               }                                                                               \
> +               return NULL;                                                                    \
> +       }
> +
>  // TODO(dlatypov@google.com): consider eventually migrating users to explicitly
>  // include resource.h themselves if they need it.
>  #include <kunit/resource.h>
> diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
> index 6bb5c2ef6696..ad9ebcfd513e 100644
> --- a/lib/kunit/kunit-example-test.c
> +++ b/lib/kunit/kunit-example-test.c
> @@ -202,6 +202,7 @@ static void example_param_get_desc(const struct example_param *p, char *desc)
>  }
>
>  KUNIT_ARRAY_PARAM(example, example_params_array, example_param_get_desc);
> +KUNIT_ZERO_ARRAY_PARAM(example_zero, example_params_array, example_param_get_desc);
>
>  /*
>   * This test shows the use of params.
> @@ -246,6 +247,7 @@ static struct kunit_case example_test_cases[] = {
>         KUNIT_CASE(example_all_expect_macros_test),
>         KUNIT_CASE(example_static_stub_test),
>         KUNIT_CASE_PARAM(example_params_test, example_gen_params),
> +       KUNIT_CASE_PARAM(example_params_test, example_zero_gen_params),
>         KUNIT_CASE_SLOW(example_slow_test),
>         {}
>  };
> --
> 2.25.1
>
Michal Wajdeczko Oct. 2, 2023, 4:17 p.m. UTC | #3
On 30.09.2023 10:58, David Gow wrote:
> On Wed, 27 Sept 2023 at 06:02, Michal Wajdeczko
> <michal.wajdeczko@intel.com> wrote:
>>
>> The existing macro KUNIT_ARRAY_PARAM can produce parameter
>> generator function but only when we fully know the definition
>> of the array. However, there might be cases where we would like
>> to generate test params based on externaly defined array, which
>> is defined as zero-terminated array, like pci_driver.id_table.
> 
> Hmm... I like the idea of this, but am a little wary of dealing with
> zero-terminated arrays in a generic fashion. Some cases (pointers,
> where we can just != NULL) are obvious,
> but we could hit inconsistencies with things like padding, as things
> like pci_driver.id_table seem to mostly be iterated over with things
> like:
> while (ids->vendor || ids->subvendor || ids->class_mask)
> 
> which not only ignores the padding, but also half of the fields. So
> there may be a consistency issue there.
> 
> Though I suspect it's not likely to cause issues in practice.
> 
> Thoughts?
> -- David
>>
>> Add helper macro KUNIT_ZERO_ARRAY_PARAM that can work with zero
>> terminated arrays and provide example how to use it.
>>
>> $ ./tools/testing/kunit/kunit.py run \
>>         --kunitconfig ./lib/kunit/.kunitconfig *.example_params*
>>
>> [ ] Starting KUnit Kernel (1/1)...
>> [ ] ============================================================
>> [ ] ========================= example  =========================
>> [ ] =================== example_params_test  ===================
>> [ ] [SKIPPED] example value 3
>> [ ] [PASSED] example value 2
>> [ ] [PASSED] example value 1
>> [ ] [SKIPPED] example value 0
>> [ ] =============== [PASSED] example_params_test ===============
>> [ ] =================== example_params_test  ===================
>> [ ] [SKIPPED] example value 3
>> [ ] [PASSED] example value 2
>> [ ] [PASSED] example value 1
>> [ ] =============== [PASSED] example_params_test ===============
>> [ ] ===================== [PASSED] example =====================
>> [ ] ============================================================
>> [ ] Testing complete. Ran 7 tests: passed: 4, skipped: 3
>>
>> 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           | 22 ++++++++++++++++++++++
>>  lib/kunit/kunit-example-test.c |  2 ++
>>  2 files changed, 24 insertions(+)
>>
>> diff --git a/include/kunit/test.h b/include/kunit/test.h
>> index 20ed9f9275c9..280113ceb6a6 100644
>> --- a/include/kunit/test.h
>> +++ b/include/kunit/test.h
>> @@ -1514,6 +1514,28 @@ do {                                                                            \
>>                 return NULL;                                                                    \
>>         }
>>
>> +/**
>> + * KUNIT_ZERO_ARRAY_PARAM() - Define test parameter generator from a zero terminated array.
>> + * @name:  prefix for the test parameter generator function.
>> + * @array: zero terminated array of test parameters.
>> + * @get_desc: function to convert param to description; NULL to use default
>> + *
>> + * Define function @name_gen_params which uses zero terminated @array to generate parameters.
>> + */
>> +#define KUNIT_ZERO_ARRAY_PARAM(name, array, get_desc)                                          \
>> +       static const void *name##_gen_params(const void *prev, char *desc)                      \
>> +       {                                                                                       \
>> +               typeof((array)[0]) *__prev = prev;                                              \
>> +               typeof(__prev) __next = __prev ? __prev + 1 : (array);                          \
>> +               void (*__get_desc)(typeof(__next), char *) = get_desc;                          \
>> +               for (; memchr_inv(__next, 0, sizeof(*__next)); __prev = __next++) {             \
> 
> Are there any places where this might interact awkwardly with padding?
> I _think_ it should be okay (variables with static lifetimes should
> have padding initialised to zero), but there could be a case I'm
> missing.

It looks that most of the existing code is relying on empty
initialization with = { 0 } or = { }, the latter known to be zero
initialized, including padding, in C23, so if we want to be pedantic we
may allow to provide a pointer to a table specific "is_end()" function,
that will detect end of the parameters array, which we could still
default to memchr_inv if custom solution is not needed:

#define KUNIT_ZERO_ARRAY_PARAM(name, array, get_desc, is_end)	\
...
	bool (*__is_end)(typeof(__next)) = is_end;		\
	for (; __is_end ? __is_end(__next) : 			\
	     !!memchr_inv(__next, 0, sizeof(*__next));		\
	     __prev = __next++) {       			\


KUNIT_ZERO_ARRAY_PARAM(example_zero, example_params_array,
			example_param_get_desc, NULL);
or

static bool example_param_valid(const struct example_param *next)
{
	return next->value;
}

KUNIT_ZERO_ARRAY_PARAM(example_zero, example_params_array,
			example_param_get_desc, example_param_valid);

then we shouldn't miss any case

Michal

> 
> 
>> +                       if (__get_desc)                                                         \
>> +                               __get_desc(__next, desc);                                       \
>> +                       return __next;                                                          \
>> +               }                                                                               \
>> +               return NULL;                                                                    \
>> +       }
>> +
>>  // TODO(dlatypov@google.com): consider eventually migrating users to explicitly
>>  // include resource.h themselves if they need it.
>>  #include <kunit/resource.h>
>> diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
>> index 6bb5c2ef6696..ad9ebcfd513e 100644
>> --- a/lib/kunit/kunit-example-test.c
>> +++ b/lib/kunit/kunit-example-test.c
>> @@ -202,6 +202,7 @@ static void example_param_get_desc(const struct example_param *p, char *desc)
>>  }
>>
>>  KUNIT_ARRAY_PARAM(example, example_params_array, example_param_get_desc);
>> +KUNIT_ZERO_ARRAY_PARAM(example_zero, example_params_array, example_param_get_desc);
>>
>>  /*
>>   * This test shows the use of params.
>> @@ -246,6 +247,7 @@ static struct kunit_case example_test_cases[] = {
>>         KUNIT_CASE(example_all_expect_macros_test),
>>         KUNIT_CASE(example_static_stub_test),
>>         KUNIT_CASE_PARAM(example_params_test, example_gen_params),
>> +       KUNIT_CASE_PARAM(example_params_test, example_zero_gen_params),
>>         KUNIT_CASE_SLOW(example_slow_test),
>>         {}
>>  };
>> --
>> 2.25.1
>>
diff mbox series

Patch

diff --git a/include/kunit/test.h b/include/kunit/test.h
index 20ed9f9275c9..280113ceb6a6 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -1514,6 +1514,28 @@  do {									       \
 		return NULL;									\
 	}
 
+/**
+ * KUNIT_ZERO_ARRAY_PARAM() - Define test parameter generator from a zero terminated array.
+ * @name:  prefix for the test parameter generator function.
+ * @array: zero terminated array of test parameters.
+ * @get_desc: function to convert param to description; NULL to use default
+ *
+ * Define function @name_gen_params which uses zero terminated @array to generate parameters.
+ */
+#define KUNIT_ZERO_ARRAY_PARAM(name, array, get_desc)						\
+	static const void *name##_gen_params(const void *prev, char *desc)			\
+	{											\
+		typeof((array)[0]) *__prev = prev;						\
+		typeof(__prev) __next = __prev ? __prev + 1 : (array);				\
+		void (*__get_desc)(typeof(__next), char *) = get_desc;				\
+		for (; memchr_inv(__next, 0, sizeof(*__next)); __prev = __next++) {		\
+			if (__get_desc)								\
+				__get_desc(__next, desc);					\
+			return __next;								\
+		}										\
+		return NULL;									\
+	}
+
 // TODO(dlatypov@google.com): consider eventually migrating users to explicitly
 // include resource.h themselves if they need it.
 #include <kunit/resource.h>
diff --git a/lib/kunit/kunit-example-test.c b/lib/kunit/kunit-example-test.c
index 6bb5c2ef6696..ad9ebcfd513e 100644
--- a/lib/kunit/kunit-example-test.c
+++ b/lib/kunit/kunit-example-test.c
@@ -202,6 +202,7 @@  static void example_param_get_desc(const struct example_param *p, char *desc)
 }
 
 KUNIT_ARRAY_PARAM(example, example_params_array, example_param_get_desc);
+KUNIT_ZERO_ARRAY_PARAM(example_zero, example_params_array, example_param_get_desc);
 
 /*
  * This test shows the use of params.
@@ -246,6 +247,7 @@  static struct kunit_case example_test_cases[] = {
 	KUNIT_CASE(example_all_expect_macros_test),
 	KUNIT_CASE(example_static_stub_test),
 	KUNIT_CASE_PARAM(example_params_test, example_gen_params),
+	KUNIT_CASE_PARAM(example_params_test, example_zero_gen_params),
 	KUNIT_CASE_SLOW(example_slow_test),
 	{}
 };