diff mbox series

[1/6] kunit: add parameter generation macro using description from array

Message ID 20231220151952.415232-2-benjamin@sipsolutions.net (mailing list archive)
State Accepted
Commit 292010ee509443a1383bb079a4fa80515aa89a7f
Delegated to: Brendan Higgins
Headers show
Series Add some more cfg80211 and mac80211 kunit tests | expand

Commit Message

Benjamin Berg Dec. 20, 2023, 3:19 p.m. UTC 
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>
---
 Documentation/dev-tools/kunit/usage.rst | 12 ++++--------
 include/kunit/test.h                    | 19 +++++++++++++++++++
 2 files changed, 23 insertions(+), 8 deletions(-)

Comments

David Gow Dec. 22, 2023, 10:02 a.m. UTC | #1 
On Wed, 20 Dec 2023 at 23:20, <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>
> ---

I'm generally pretty happy with this, though note the checkpatch warning below.

There was some discussion at plumbers about expanding the
parameterised test APIs, so we may need to adjust the implementation
of this down the line, but I don't think that'll happen for a while,
so don't worry.

With the warnings fixed, this is:

Reviewed-by: David Gow <davidgow@google.com>

I'm okay with this going in via the wireless tree if that's easier;
certainly there are some conflicts with the later patches in this
series and the kunit one.

Cheers,
-- David

>  Documentation/dev-tools/kunit/usage.rst | 12 ++++--------
>  include/kunit/test.h                    | 19 +++++++++++++++++++
>  2 files changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
> index c27e1646ecd9..b959e5befcbe 100644
> --- a/Documentation/dev-tools/kunit/usage.rst
> +++ b/Documentation/dev-tools/kunit/usage.rst
> @@ -566,13 +566,9 @@ By reusing the same ``cases`` array from above, we can write the test as a
>                 },
>         };
>
> -       // Need a helper function to generate a name for each test case.
> -       static void case_to_desc(const struct sha1_test_case *t, char *desc)
> -       {
> -               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 +584,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 20ed9f9275c9..2dfa851e1f88 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -1514,6 +1514,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);      \

checkpatch is complaining here:
ERROR: need consistent spacing around '*' (ctx:WxV)
#71: FILE: include/kunit/test.h:1528:

+               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.43.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/20231220151952.415232-2-benjamin%40sipsolutions.net.
Benjamin Berg Dec. 22, 2023, 2:59 p.m. UTC | #2 
On Fri, 2023-12-22 at 18:02 +0800, David Gow wrote:
> On Wed, 20 Dec 2023 at 23:20, <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>
> > ---
> 
> I'm generally pretty happy with this, though note the checkpatch
> warning below.
> 
> There was some discussion at plumbers about expanding the
> parameterised test APIs, so we may need to adjust the implementation
> of this down the line, but I don't think that'll happen for a while,
> so don't worry.
> 
> With the warnings fixed, this is:

I think the checkpatch warning is a false positive. It seems to confuse
the * as a multiplication due to typeof() looking like a function call
rather than a variable declaration.

Benjamin

> Reviewed-by: David Gow <davidgow@google.com>
> 
> I'm okay with this going in via the wireless tree if that's easier;
> certainly there are some conflicts with the later patches in this
> series and the kunit one.
> 
> Cheers,
> -- David
> 
> >  Documentation/dev-tools/kunit/usage.rst | 12 ++++--------
> >  include/kunit/test.h                    | 19 +++++++++++++++++++
> >  2 files changed, 23 insertions(+), 8 deletions(-)
> > 
> > diff --git a/Documentation/dev-tools/kunit/usage.rst
> > b/Documentation/dev-tools/kunit/usage.rst
> > index c27e1646ecd9..b959e5befcbe 100644
> > --- a/Documentation/dev-tools/kunit/usage.rst
> > +++ b/Documentation/dev-tools/kunit/usage.rst
> > @@ -566,13 +566,9 @@ By reusing the same ``cases`` array from
> > above, we can write the test as a
> >                 },
> >         };
> > 
> > -       // Need a helper function to generate a name for each test
> > case.
> > -       static void case_to_desc(const struct sha1_test_case *t,
> > char *desc)
> > -       {
> > -               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 +584,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 20ed9f9275c9..2dfa851e1f88 100644
> > --- a/include/kunit/test.h
> > +++ b/include/kunit/test.h
> > @@ -1514,6 +1514,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);      \
> 
> checkpatch is complaining here:
> ERROR: need consistent spacing around '*' (ctx:WxV)
> #71: FILE: include/kunit/test.h:1528:
> 
> +               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.43.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/20231220151952.415232-2-benjamin%40sipsolutions.net
> > .
David Gow Dec. 23, 2023, 5:09 a.m. UTC | #3 
On Fri, 22 Dec 2023 at 23:09, Benjamin Berg <benjamin@sipsolutions.net> wrote:
>
> On Fri, 2023-12-22 at 18:02 +0800, David Gow wrote:
> > On Wed, 20 Dec 2023 at 23:20, <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>
> > > ---
> >
> > I'm generally pretty happy with this, though note the checkpatch
> > warning below.
> >
> > There was some discussion at plumbers about expanding the
> > parameterised test APIs, so we may need to adjust the implementation
> > of this down the line, but I don't think that'll happen for a while,
> > so don't worry.
> >
> > With the warnings fixed, this is:
>
> I think the checkpatch warning is a false positive. It seems to confuse
> the * as a multiplication due to typeof() looking like a function call
> rather than a variable declaration.
>
> Benjamin
>

Ah, yeah: this appears to be due to checkpatch not handling nested ()
within a typeof().

Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David
diff mbox series

Patch

diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
index c27e1646ecd9..b959e5befcbe 100644
--- a/Documentation/dev-tools/kunit/usage.rst
+++ b/Documentation/dev-tools/kunit/usage.rst
@@ -566,13 +566,9 @@  By reusing the same ``cases`` array from above, we can write the test as a
 		},
 	};
 
-	// Need a helper function to generate a name for each test case.
-	static void case_to_desc(const struct sha1_test_case *t, char *desc)
-	{
-		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 +584,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 20ed9f9275c9..2dfa851e1f88 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -1514,6 +1514,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>