| Message ID | 20230904132139.103140-1-benjamin@sipsolutions.net (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | [1/2] kunit: add parameter generation macro using description from array | expand |
On Mon, 4 Sept 2023 at 21:22, <benjamin@sipsolutions.net> wrote: > > From: Benjamin Berg <benjamin.berg@intel.com> > > The existing KUNIT_ARRAY_PARAM macro requires a separate function to > get the description. However, in a lot of cases the description can > just be copied directly from the array. Add a second macro that > avoids having to write a static function just for a single strscpy. > > Signed-off-by: Benjamin Berg <benjamin.berg@intel.com> > --- Looks good to me: this will be much more convenient. The actual implementation looks spot on, just a small comment about the documentation change. It may make sense to write some tests and/or some follow-up patches to existing tests to use this macro, too. I'm just a little wary of introducing something totally unused. (I'm happy to do these myself if you don't have time, though.) Regardless, with the documentation fix, this is: Reviewed-by: David Gow <davidgow@google.com> Cheers, -- David > Documentation/dev-tools/kunit/usage.rst | 7 ++++--- > include/kunit/test.h | 19 +++++++++++++++++++ > 2 files changed, 23 insertions(+), 3 deletions(-) > > diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst > index c27e1646ecd9..fe8c28d66dfe 100644 > --- a/Documentation/dev-tools/kunit/usage.rst > +++ b/Documentation/dev-tools/kunit/usage.rst > @@ -571,8 +571,9 @@ By reusing the same ``cases`` array from above, we can write the test as a > { > strcpy(desc, t->str); > } > - // Creates `sha1_gen_params()` to iterate over `cases`. > - KUNIT_ARRAY_PARAM(sha1, cases, case_to_desc); > + // Creates `sha1_gen_params()` to iterate over `cases` while using > + // the struct member `str` for the case description. > + KUNIT_ARRAY_PARAM_DESC(sha1, cases, str); I'd suggest either getting rid of the case_to_desc function totally here, or show both the manual KUNIT_ARRAY_PARAM() example, and then point out explicitly that KUNIT_ARRAY_PARAM_DESC() can replace it. Otherwise we end up with a vestigial function which doesn't do anything and is confusing. > > // Looks no different from a normal test. > static void sha1_test(struct kunit *test) > @@ -588,7 +589,7 @@ By reusing the same ``cases`` array from above, we can write the test as a > } > > // Instead of KUNIT_CASE, we use KUNIT_CASE_PARAM and pass in the > - // function declared by KUNIT_ARRAY_PARAM. > + // function declared by KUNIT_ARRAY_PARAM or KUNIT_ARRAY_PARAM_DESC. > static struct kunit_case sha1_test_cases[] = { > KUNIT_CASE_PARAM(sha1_test, sha1_gen_params), > {} > diff --git a/include/kunit/test.h b/include/kunit/test.h > index 68ff01aee244..f60d11e41855 100644 > --- a/include/kunit/test.h > +++ b/include/kunit/test.h > @@ -1516,6 +1516,25 @@ do { \ > return NULL; \ > } > > +/** > + * KUNIT_ARRAY_PARAM_DESC() - Define test parameter generator from an array. > + * @name: prefix for the test parameter generator function. > + * @array: array of test parameters. > + * @desc_member: structure member from array element to use as description > + * > + * Define function @name_gen_params which uses @array to generate parameters. > + */ > +#define KUNIT_ARRAY_PARAM_DESC(name, array, desc_member) \ > + static const void *name##_gen_params(const void *prev, char *desc) \ > + { \ > + typeof((array)[0]) *__next = prev ? ((typeof(__next)) prev) + 1 : (array); \ > + if (__next - (array) < ARRAY_SIZE((array))) { \ > + strscpy(desc, __next->desc_member, KUNIT_PARAM_DESC_SIZE); \ > + 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> > -- > 2.41.0 > > -- > You received this message because you are subscribed to the Google Groups "KUnit Development" group. > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230904132139.103140-1-benjamin%40sipsolutions.net.
Hi, On Wed, 2023-09-06 at 14:45 +0800, David Gow wrote: > On Mon, 4 Sept 2023 at 21:22, <benjamin@sipsolutions.net> wrote: > > > > From: Benjamin Berg <benjamin.berg@intel.com> > > > > The existing KUNIT_ARRAY_PARAM macro requires a separate function > > to > > get the description. However, in a lot of cases the description can > > just be copied directly from the array. Add a second macro that > > avoids having to write a static function just for a single strscpy. > > > > Signed-off-by: Benjamin Berg <benjamin.berg@intel.com> > > --- > > Looks good to me: this will be much more convenient. The actual > implementation looks spot on, just a small comment about the > documentation change. > > It may make sense to write some tests and/or some follow-up patches to > existing tests to use this macro, too. I'm just a little wary of > introducing something totally unused. (I'm happy to do these myself if > you don't have time, though.) I agree. I am happy to submit one or more patches to change the existing users. The question would be how we pull such a change in. Should it be submitted separately for each subtree or can we pull them all in at the same time here? Benjamin > > Regardless, with the documentation fix, this is: > Reviewed-by: David Gow <davidgow@google.com> > > Cheers, > -- David > > > Documentation/dev-tools/kunit/usage.rst | 7 ++++--- > > include/kunit/test.h | 19 +++++++++++++++++++ > > 2 files changed, 23 insertions(+), 3 deletions(-) > > > > diff --git a/Documentation/dev-tools/kunit/usage.rst > > b/Documentation/dev-tools/kunit/usage.rst > > index c27e1646ecd9..fe8c28d66dfe 100644 > > --- a/Documentation/dev-tools/kunit/usage.rst > > +++ b/Documentation/dev-tools/kunit/usage.rst > > @@ -571,8 +571,9 @@ By reusing the same ``cases`` array from above, > > we can write the test as a > > { > > strcpy(desc, t->str); > > } > > - // Creates `sha1_gen_params()` to iterate over `cases`. > > - KUNIT_ARRAY_PARAM(sha1, cases, case_to_desc); > > + // Creates `sha1_gen_params()` to iterate over `cases` > > while using > > + // the struct member `str` for the case description. > > + KUNIT_ARRAY_PARAM_DESC(sha1, cases, str); > > I'd suggest either getting rid of the case_to_desc function totally > here, or show both the manual KUNIT_ARRAY_PARAM() example, and then > point out explicitly that KUNIT_ARRAY_PARAM_DESC() can replace it. > > Otherwise we end up with a vestigial function which doesn't do > anything and is confusing. > > > > > > // Looks no different from a normal test. > > static void sha1_test(struct kunit *test) > > @@ -588,7 +589,7 @@ By reusing the same ``cases`` array from above, > > we can write the test as a > > } > > > > // Instead of KUNIT_CASE, we use KUNIT_CASE_PARAM and pass > > in the > > - // function declared by KUNIT_ARRAY_PARAM. > > + // function declared by KUNIT_ARRAY_PARAM or > > KUNIT_ARRAY_PARAM_DESC. > > static struct kunit_case sha1_test_cases[] = { > > KUNIT_CASE_PARAM(sha1_test, sha1_gen_params), > > {} > > diff --git a/include/kunit/test.h b/include/kunit/test.h > > index 68ff01aee244..f60d11e41855 100644 > > --- a/include/kunit/test.h > > +++ b/include/kunit/test.h > > @@ -1516,6 +1516,25 @@ do > > { > > \ > > return > > NULL; > > \ > > } > > > > +/** > > + * KUNIT_ARRAY_PARAM_DESC() - Define test parameter generator from > > an array. > > + * @name: prefix for the test parameter generator function. > > + * @array: array of test parameters. > > + * @desc_member: structure member from array element to use as > > description > > + * > > + * Define function @name_gen_params which uses @array to generate > > parameters. > > + */ > > +#define KUNIT_ARRAY_PARAM_DESC(name, array, > > desc_member) \ > > + static const void *name##_gen_params(const void *prev, char > > *desc) \ > > + > > { > > \ > > + typeof((array)[0]) *__next = prev ? > > ((typeof(__next)) prev) + 1 : (array); \ > > + if (__next - (array) < ARRAY_SIZE((array))) > > { \ > > + strscpy(desc, __next->desc_member, > > KUNIT_PARAM_DESC_SIZE); \ > > + 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> > > -- > > 2.41.0 > > > > -- > > You received this message because you are subscribed to the Google > > Groups "KUnit Development" group. > > To unsubscribe from this group and stop receiving emails from it, > > send an email to kunit-dev+unsubscribe@googlegroups.com. > > To view this discussion on the web visit > > https://groups.google.com/d/msgid/kunit-dev/20230904132139.103140-1-benjamin%40sipsolutions.net > > . Intel Deutschland GmbH Registered Address: Am Campeon 10, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de> Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928
On Wed, 6 Sept 2023 at 15:07, Berg, Benjamin <benjamin.berg@intel.com> wrote: > > Hi, > > On Wed, 2023-09-06 at 14:45 +0800, David Gow wrote: > > On Mon, 4 Sept 2023 at 21:22, <benjamin@sipsolutions.net> wrote: > > > > > > From: Benjamin Berg <benjamin.berg@intel.com> > > > > > > The existing KUNIT_ARRAY_PARAM macro requires a separate function > > > to > > > get the description. However, in a lot of cases the description can > > > just be copied directly from the array. Add a second macro that > > > avoids having to write a static function just for a single strscpy. > > > > > > Signed-off-by: Benjamin Berg <benjamin.berg@intel.com> > > > --- > > > > Looks good to me: this will be much more convenient. The actual > > implementation looks spot on, just a small comment about the > > documentation change. > > > > It may make sense to write some tests and/or some follow-up patches to > > existing tests to use this macro, too. I'm just a little wary of > > introducing something totally unused. (I'm happy to do these myself if > > you don't have time, though.) > > I agree. I am happy to submit one or more patches to change the > existing users. The question would be how we pull such a change in. > Should it be submitted separately for each subtree or can we pull them > all in at the same time here? > > Benjamin > It depends a little bit on the test being modified: if it rarely sees conflicting changes, we can pull it in via KUnit, if it's being actively modified a lot, it's best to send it through separately. If you're not sure, just include them all in a series here, CC the test maintainers, and ask what tree they'd prefer it to go in via. It all usually works out in the end, and worst-case, if we miss one or two tests, we can update them separately. Cheers, -- David > > > > Regardless, with the documentation fix, this is: > > Reviewed-by: David Gow <davidgow@google.com> > > > > Cheers, > > -- David > > > > > Documentation/dev-tools/kunit/usage.rst | 7 ++++--- > > > include/kunit/test.h | 19 +++++++++++++++++++ > > > 2 files changed, 23 insertions(+), 3 deletions(-) > > > > > > diff --git a/Documentation/dev-tools/kunit/usage.rst > > > b/Documentation/dev-tools/kunit/usage.rst > > > index c27e1646ecd9..fe8c28d66dfe 100644 > > > --- a/Documentation/dev-tools/kunit/usage.rst > > > +++ b/Documentation/dev-tools/kunit/usage.rst > > > @@ -571,8 +571,9 @@ By reusing the same ``cases`` array from above, > > > we can write the test as a > > > { > > > strcpy(desc, t->str); > > > } > > > - // Creates `sha1_gen_params()` to iterate over `cases`. > > > - KUNIT_ARRAY_PARAM(sha1, cases, case_to_desc); > > > + // Creates `sha1_gen_params()` to iterate over `cases` > > > while using > > > + // the struct member `str` for the case description. > > > + KUNIT_ARRAY_PARAM_DESC(sha1, cases, str); > > > > I'd suggest either getting rid of the case_to_desc function totally > > here, or show both the manual KUNIT_ARRAY_PARAM() example, and then > > point out explicitly that KUNIT_ARRAY_PARAM_DESC() can replace it. > > > > Otherwise we end up with a vestigial function which doesn't do > > anything and is confusing. > > > > > > > > > > // Looks no different from a normal test. > > > static void sha1_test(struct kunit *test) > > > @@ -588,7 +589,7 @@ By reusing the same ``cases`` array from above, > > > we can write the test as a > > > } > > > > > > // Instead of KUNIT_CASE, we use KUNIT_CASE_PARAM and pass > > > in the > > > - // function declared by KUNIT_ARRAY_PARAM. > > > + // function declared by KUNIT_ARRAY_PARAM or > > > KUNIT_ARRAY_PARAM_DESC. > > > static struct kunit_case sha1_test_cases[] = { > > > KUNIT_CASE_PARAM(sha1_test, sha1_gen_params), > > > {} > > > diff --git a/include/kunit/test.h b/include/kunit/test.h > > > index 68ff01aee244..f60d11e41855 100644 > > > --- a/include/kunit/test.h > > > +++ b/include/kunit/test.h > > > @@ -1516,6 +1516,25 @@ do > > > { > > > \ > > > return > > > NULL; > > > \ > > > } > > > > > > +/** > > > + * KUNIT_ARRAY_PARAM_DESC() - Define test parameter generator from > > > an array. > > > + * @name: prefix for the test parameter generator function. > > > + * @array: array of test parameters. > > > + * @desc_member: structure member from array element to use as > > > description > > > + * > > > + * Define function @name_gen_params which uses @array to generate > > > parameters. > > > + */ > > > +#define KUNIT_ARRAY_PARAM_DESC(name, array, > > > desc_member) \ > > > + static const void *name##_gen_params(const void *prev, char > > > *desc) \ > > > + > > > { > > > \ > > > + typeof((array)[0]) *__next = prev ? > > > ((typeof(__next)) prev) + 1 : (array); \ > > > + if (__next - (array) < ARRAY_SIZE((array))) > > > { \ > > > + strscpy(desc, __next->desc_member, > > > KUNIT_PARAM_DESC_SIZE); \ > > > + 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> > > > -- > > > 2.41.0 > > > > > > -- > > > You received this message because you are subscribed to the Google > > > Groups "KUnit Development" group. > > > To unsubscribe from this group and stop receiving emails from it, > > > send an email to kunit-dev+unsubscribe@googlegroups.com. > > > To view this discussion on the web visit > > > https://groups.google.com/d/msgid/kunit-dev/20230904132139.103140-1-benjamin%40sipsolutions.net > > > . > > Intel Deutschland GmbH > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany > Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de> > Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva > Chairperson of the Supervisory Board: Nicole Lau > Registered Office: Munich > Commercial Register: Amtsgericht Muenchen HRB 186928
diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst index c27e1646ecd9..fe8c28d66dfe 100644 --- a/Documentation/dev-tools/kunit/usage.rst +++ b/Documentation/dev-tools/kunit/usage.rst @@ -571,8 +571,9 @@ By reusing the same ``cases`` array from above, we can write the test as a { strcpy(desc, t->str); } - // Creates `sha1_gen_params()` to iterate over `cases`. - KUNIT_ARRAY_PARAM(sha1, cases, case_to_desc); + // Creates `sha1_gen_params()` to iterate over `cases` while using + // the struct member `str` for the case description. + KUNIT_ARRAY_PARAM_DESC(sha1, cases, str); // Looks no different from a normal test. static void sha1_test(struct kunit *test) @@ -588,7 +589,7 @@ By reusing the same ``cases`` array from above, we can write the test as a } // Instead of KUNIT_CASE, we use KUNIT_CASE_PARAM and pass in the - // function declared by KUNIT_ARRAY_PARAM. + // function declared by KUNIT_ARRAY_PARAM or KUNIT_ARRAY_PARAM_DESC. static struct kunit_case sha1_test_cases[] = { KUNIT_CASE_PARAM(sha1_test, sha1_gen_params), {} diff --git a/include/kunit/test.h b/include/kunit/test.h index 68ff01aee244..f60d11e41855 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -1516,6 +1516,25 @@ do { \ return NULL; \ } +/** + * KUNIT_ARRAY_PARAM_DESC() - Define test parameter generator from an array. + * @name: prefix for the test parameter generator function. + * @array: array of test parameters. + * @desc_member: structure member from array element to use as description + * + * Define function @name_gen_params which uses @array to generate parameters. + */ +#define KUNIT_ARRAY_PARAM_DESC(name, array, desc_member) \ + static const void *name##_gen_params(const void *prev, char *desc) \ + { \ + typeof((array)[0]) *__next = prev ? ((typeof(__next)) prev) + 1 : (array); \ + if (__next - (array) < ARRAY_SIZE((array))) { \ + strscpy(desc, __next->desc_member, KUNIT_PARAM_DESC_SIZE); \ + 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>