linux-kselftest.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Documentation: kunit: provide guidance for testing many inputs
@ 2020-11-02 21:36 Daniel Latypov
  2020-11-07  4:21 ` David Gow
  2020-11-23 18:32 ` Brendan Higgins
  0 siblings, 2 replies; 5+ messages in thread
From: Daniel Latypov @ 2020-11-02 21:36 UTC (permalink / raw)
  To: brendanhiggins, andriy.shevchenko
  Cc: davidgow, linux-kernel, linux-kselftest, skhan, Daniel Latypov

usage.rst goes into a detailed about faking out classes, but currently
lacks wording about how one might idiomatically test a range of inputs.

Give an example of how one might test a hash function via macros/helper
funcs and a table-driven test and very briefly discuss pros and cons.

Also highlight the KUNIT_EXPECT_*_MSG() variants (that aren't mentioned
elsewhere [1]) which are particularly useful in these situations.

It is also criminally underused at the moment, only appearing in 2
tests (both written by people involved in KUnit).

[1] not even on
https://www.kernel.org/doc/html/latest/dev-tools/kunit/api/test.html

Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
 Documentation/dev-tools/kunit/usage.rst | 66 +++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
index 62142a47488c..317390df2b96 100644
--- a/Documentation/dev-tools/kunit/usage.rst
+++ b/Documentation/dev-tools/kunit/usage.rst
@@ -451,6 +451,72 @@ We can now use it to test ``struct eeprom_buffer``:
 		destroy_eeprom_buffer(ctx->eeprom_buffer);
 	}
 
+Testing various inputs
+----------------------
+
+Testing just a few inputs might not be enough to have confidence that the code
+works correctly, e.g. for a hash function.
+
+In such cases, it can be helpful to have a helper macro or function, e.g. this
+fictitious example for ``md5sum(1)``
+
+.. code-block:: c
+
+	/* Note: the cast is to satisfy overly strict type-checking. */
+	#define TEST_MD5(in, want) \
+		md5sum(in, out); \
+		KUNIT_EXPECT_STREQ_MSG(test, (char *)out, want, "md5sum(%s)", in);
+
+	char out[16];
+	TEST_MD5("hello world",   "5eb63bbbe01eeed093cb22bb8f5acdc3");
+	TEST_MD5("hello world!",  "fc3ff98e8c6a0d3087d515c0473f8677");
+
+Note the use of ``KUNIT_EXPECT_STREQ_MSG`` to give more context when it fails
+and make it easier to track down. (Yes, in this example, ``want`` is likely
+going to be unique enough on its own).
+
+The ``_MSG`` variants are even more useful when the same expectation is called
+multiple times (in a loop or helper function) and thus the line number isn't
+enough to identify what failed, like below.
+
+In some cases, it can be helpful to write a *table-driven test* instead, e.g.
+
+.. code-block:: c
+
+	int i;
+	char out[16];
+
+	struct md5_test_case {
+		const char *str;
+		const char *md5;
+	};
+
+	struct md5_test_case cases[] = {
+		{
+			.str = "hello world",
+			.md5 = "5eb63bbbe01eeed093cb22bb8f5acdc3",
+		},
+		{
+			.str = "hello world!",
+			.md5 = "fc3ff98e8c6a0d3087d515c0473f8677",
+		},
+	};
+	for (i = 0; i < ARRAY_SIZE(cases); ++i) {
+		md5sum(cases[i].str, out);
+		KUNIT_EXPECT_STREQ_MSG(test, (char *)out, cases[i].md5,
+		                      "md5sum(%s)", cases[i].str);
+	}
+
+
+There's more boilerplate involved, but it can:
+
+* be more readable when there are multiple inputs/outputs thanks to field names,
+
+  * E.g. see ``fs/ext4/inode-test.c`` for an example of both.
+* reduce duplication if test cases can be shared across multiple tests.
+
+  * E.g. if we had a magical ``undo_md5sum`` function, we could reuse ``cases``.
+
 .. _kunit-on-non-uml:
 
 KUnit on non-UML architectures

base-commit: 77c8473edf7f7664137f555cfcdc8c460bbd947d
-- 
2.29.1.341.ge80a0c044ae-goog


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] Documentation: kunit: provide guidance for testing many inputs
  2020-11-02 21:36 [PATCH] Documentation: kunit: provide guidance for testing many inputs Daniel Latypov
@ 2020-11-07  4:21 ` David Gow
  2020-11-13 18:21   ` Daniel Latypov
  2020-11-23 18:32 ` Brendan Higgins
  1 sibling, 1 reply; 5+ messages in thread
From: David Gow @ 2020-11-07  4:21 UTC (permalink / raw)
  To: Daniel Latypov
  Cc: Brendan Higgins, Andy Shevchenko, Linux Kernel Mailing List,
	open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan

On Tue, Nov 3, 2020 at 5:37 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> usage.rst goes into a detailed about faking out classes, but currently

Nit: a detailed what?

> lacks wording about how one might idiomatically test a range of inputs.
>
> Give an example of how one might test a hash function via macros/helper
> funcs and a table-driven test and very briefly discuss pros and cons.
>
> Also highlight the KUNIT_EXPECT_*_MSG() variants (that aren't mentioned
> elsewhere [1]) which are particularly useful in these situations.
>
> It is also criminally underused at the moment, only appearing in 2
> tests (both written by people involved in KUnit).
>
> [1] not even on
> https://www.kernel.org/doc/html/latest/dev-tools/kunit/api/test.html

I suspect we'll eventually want to document the _MSG variants here as
well, though it will bloat the page somewhat. In any case, it can be
left to a separate patch.

>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---

Thanks for writing this -- it's definitely a common test pattern which
it'd be nice to encourage and explain a bit better.

Cheers,
-- David

>  Documentation/dev-tools/kunit/usage.rst | 66 +++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
>
> diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
> index 62142a47488c..317390df2b96 100644
> --- a/Documentation/dev-tools/kunit/usage.rst
> +++ b/Documentation/dev-tools/kunit/usage.rst
> @@ -451,6 +451,72 @@ We can now use it to test ``struct eeprom_buffer``:
>                 destroy_eeprom_buffer(ctx->eeprom_buffer);
>         }
>
> +Testing various inputs
> +----------------------
Nit: "various" isn't hugely descriptive here. Maybe something like
"Testing against multiple inputs" would be better?

> +
> +Testing just a few inputs might not be enough to have confidence that the code
> +works correctly, e.g. for a hash function.
> +
> +In such cases, it can be helpful to have a helper macro or function, e.g. this
> +fictitious example for ``md5sum(1)``
> +
> +.. code-block:: c
> +
> +       /* Note: the cast is to satisfy overly strict type-checking. */
> +       #define TEST_MD5(in, want) \
> +               md5sum(in, out); \
> +               KUNIT_EXPECT_STREQ_MSG(test, (char *)out, want, "md5sum(%s)", in);
> +
> +       char out[16];
> +       TEST_MD5("hello world",   "5eb63bbbe01eeed093cb22bb8f5acdc3");
> +       TEST_MD5("hello world!",  "fc3ff98e8c6a0d3087d515c0473f8677");
> +
> +Note the use of ``KUNIT_EXPECT_STREQ_MSG`` to give more context when it fails
> +and make it easier to track down. (Yes, in this example, ``want`` is likely
> +going to be unique enough on its own).
> +
> +The ``_MSG`` variants are even more useful when the same expectation is called
> +multiple times (in a loop or helper function) and thus the line number isn't
> +enough to identify what failed, like below.
> +
> +In some cases, it can be helpful to write a *table-driven test* instead, e.g.
> +
> +.. code-block:: c
> +
> +       int i;
> +       char out[16];
> +
> +       struct md5_test_case {
> +               const char *str;
> +               const char *md5;
> +       };
> +
> +       struct md5_test_case cases[] = {
> +               {
> +                       .str = "hello world",
> +                       .md5 = "5eb63bbbe01eeed093cb22bb8f5acdc3",
> +               },
> +               {
> +                       .str = "hello world!",
> +                       .md5 = "fc3ff98e8c6a0d3087d515c0473f8677",
> +               },
> +       };
> +       for (i = 0; i < ARRAY_SIZE(cases); ++i) {
> +               md5sum(cases[i].str, out);
> +               KUNIT_EXPECT_STREQ_MSG(test, (char *)out, cases[i].md5,
> +                                     "md5sum(%s)", cases[i].str);
> +       }
> +
> +
> +There's more boilerplate involved, but it can:
> +
> +* be more readable when there are multiple inputs/outputs thanks to field names,
> +
> +  * E.g. see ``fs/ext4/inode-test.c`` for an example of both.
> +* reduce duplication if test cases can be shared across multiple tests.
> +
> +  * E.g. if we had a magical ``undo_md5sum`` function, we could reuse ``cases``.
> +

This is a bit of a nitpick, but I don't think this is quite conveying
the usefulness of table-based testing. Maybe it's that a hypothetical
"undo_md5sum" is too unrealistic an example? Maybe, instead of having
both the macro-based and table-driven examples based around md5sum(),
the table-based one could use something more obviously invertible /
reusable, and include both in the example code. E.g, something akin to
toupper() and tolower() or some other conversion function. I think
having a better example here is probably more useful than having both
the table- and macro- driven examples test the same thing.


>  .. _kunit-on-non-uml:
>
>  KUnit on non-UML architectures
>
> base-commit: 77c8473edf7f7664137f555cfcdc8c460bbd947d
> --
> 2.29.1.341.ge80a0c044ae-goog
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Documentation: kunit: provide guidance for testing many inputs
  2020-11-07  4:21 ` David Gow
@ 2020-11-13 18:21   ` Daniel Latypov
  0 siblings, 0 replies; 5+ messages in thread
From: Daniel Latypov @ 2020-11-13 18:21 UTC (permalink / raw)
  To: David Gow
  Cc: Brendan Higgins, Andy Shevchenko, Linux Kernel Mailing List,
	open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan

On Fri, Nov 6, 2020 at 8:21 PM David Gow <davidgow@google.com> wrote:
>
> On Tue, Nov 3, 2020 at 5:37 AM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > usage.rst goes into a detailed about faking out classes, but currently
>
> Nit: a detailed what?

Thanks for the catch, added "detailed section" locally.

>
> > lacks wording about how one might idiomatically test a range of inputs.
> >
> > Give an example of how one might test a hash function via macros/helper
> > funcs and a table-driven test and very briefly discuss pros and cons.
> >
> > Also highlight the KUNIT_EXPECT_*_MSG() variants (that aren't mentioned
> > elsewhere [1]) which are particularly useful in these situations.
> >
> > It is also criminally underused at the moment, only appearing in 2
> > tests (both written by people involved in KUnit).
> >
> > [1] not even on
> > https://www.kernel.org/doc/html/latest/dev-tools/kunit/api/test.html
>
> I suspect we'll eventually want to document the _MSG variants here as
> well, though it will bloat the page somewhat. In any case, it can be
> left to a separate patch.

Agreed.

>
> >
> > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > ---
>
> Thanks for writing this -- it's definitely a common test pattern which
> it'd be nice to encourage and explain a bit better.

Apologies for the delayed response.
Noting here that having talked offline with David, this section will
have to change for parameterized testing (which is basically just
formalized support for table-driven tests).
But it seems it'll take a while to resolve the debate on TAP output,
so this docs change shouldn't be blocked on that going in.

>
> Cheers,
> -- David
>
> >  Documentation/dev-tools/kunit/usage.rst | 66 +++++++++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> >
> > diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
> > index 62142a47488c..317390df2b96 100644
> > --- a/Documentation/dev-tools/kunit/usage.rst
> > +++ b/Documentation/dev-tools/kunit/usage.rst
> > @@ -451,6 +451,72 @@ We can now use it to test ``struct eeprom_buffer``:
> >                 destroy_eeprom_buffer(ctx->eeprom_buffer);
> >         }
> >
> > +Testing various inputs
> > +----------------------
> Nit: "various" isn't hugely descriptive here. Maybe something like
> "Testing against multiple inputs" would be better?

Changed.
As you can tell from the name of this patch ("many inputs"), I had
been unsure what to put here.
"multiple inputs" works fine, I think.
 I had initially changed from that, since I had wanted to convey that
these patterns are more useful when you have a larger number of inputs
to go through.
But in hindsight "multiple inputs" is just more clear.

>
> > +
> > +Testing just a few inputs might not be enough to have confidence that the code
> > +works correctly, e.g. for a hash function.
> > +
> > +In such cases, it can be helpful to have a helper macro or function, e.g. this
> > +fictitious example for ``md5sum(1)``
> > +
> > +.. code-block:: c
> > +
> > +       /* Note: the cast is to satisfy overly strict type-checking. */
> > +       #define TEST_MD5(in, want) \
> > +               md5sum(in, out); \
> > +               KUNIT_EXPECT_STREQ_MSG(test, (char *)out, want, "md5sum(%s)", in);
> > +
> > +       char out[16];
> > +       TEST_MD5("hello world",   "5eb63bbbe01eeed093cb22bb8f5acdc3");
> > +       TEST_MD5("hello world!",  "fc3ff98e8c6a0d3087d515c0473f8677");
> > +
> > +Note the use of ``KUNIT_EXPECT_STREQ_MSG`` to give more context when it fails
> > +and make it easier to track down. (Yes, in this example, ``want`` is likely
> > +going to be unique enough on its own).
> > +
> > +The ``_MSG`` variants are even more useful when the same expectation is called
> > +multiple times (in a loop or helper function) and thus the line number isn't
> > +enough to identify what failed, like below.
> > +
> > +In some cases, it can be helpful to write a *table-driven test* instead, e.g.
> > +
> > +.. code-block:: c
> > +
> > +       int i;
> > +       char out[16];
> > +
> > +       struct md5_test_case {
> > +               const char *str;
> > +               const char *md5;
> > +       };
> > +
> > +       struct md5_test_case cases[] = {
> > +               {
> > +                       .str = "hello world",
> > +                       .md5 = "5eb63bbbe01eeed093cb22bb8f5acdc3",
> > +               },
> > +               {
> > +                       .str = "hello world!",
> > +                       .md5 = "fc3ff98e8c6a0d3087d515c0473f8677",
> > +               },
> > +       };
> > +       for (i = 0; i < ARRAY_SIZE(cases); ++i) {
> > +               md5sum(cases[i].str, out);
> > +               KUNIT_EXPECT_STREQ_MSG(test, (char *)out, cases[i].md5,
> > +                                     "md5sum(%s)", cases[i].str);
> > +       }
> > +
> > +
> > +There's more boilerplate involved, but it can:
> > +
> > +* be more readable when there are multiple inputs/outputs thanks to field names,
> > +
> > +  * E.g. see ``fs/ext4/inode-test.c`` for an example of both.
> > +* reduce duplication if test cases can be shared across multiple tests.
> > +
> > +  * E.g. if we had a magical ``undo_md5sum`` function, we could reuse ``cases``.
> > +
>
> This is a bit of a nitpick, but I don't think this is quite conveying
> the usefulness of table-based testing. Maybe it's that a hypothetical
> "undo_md5sum" is too unrealistic an example? Maybe, instead of having
> both the macro-based and table-driven examples based around md5sum(),
> the table-based one could use something more obviously invertible /
> reusable, and include both in the example code. E.g, something akin to
> toupper() and tolower() or some other conversion function. I think
> having a better example here is probably more useful than having both
> the table- and macro- driven examples test the same thing.

Heh, I was worried about this a bit as well.
Perhaps an inverse md5 breaks "suspension of disbelief" too much, even
in this hypothetical context.

I had considered toupper()/tolower() but they aren't truly inverses,
e.g. tolower(toupper("Hello")), and felt a bit too trivial perhaps.
I'm also unsure I would test these functions on strs as opposed to
chars (unless we're dealing with non-ascii), whereas I probably would
test a checksum like this.
I wouldn't be too opposed to switching over to tolower/toupper().

But perhaps something like

struct sha_test_case {
  const char *str;
  const char *sha1, *sha256;
};

and reusing the same table would be good enough to demonstrate a
different kind of "reuse" that is somewhat common for table-driven
tests?

>
>
> >  .. _kunit-on-non-uml:
> >
> >  KUnit on non-UML architectures
> >
> > base-commit: 77c8473edf7f7664137f555cfcdc8c460bbd947d
> > --
> > 2.29.1.341.ge80a0c044ae-goog
> >

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Documentation: kunit: provide guidance for testing many inputs
  2020-11-02 21:36 [PATCH] Documentation: kunit: provide guidance for testing many inputs Daniel Latypov
  2020-11-07  4:21 ` David Gow
@ 2020-11-23 18:32 ` Brendan Higgins
  2020-11-23 22:13   ` Daniel Latypov
  1 sibling, 1 reply; 5+ messages in thread
From: Brendan Higgins @ 2020-11-23 18:32 UTC (permalink / raw)
  To: Daniel Latypov
  Cc: Andy Shevchenko, David Gow, Linux Kernel Mailing List,
	open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan

On Mon, Nov 2, 2020 at 1:37 PM Daniel Latypov <dlatypov@google.com> wrote:
>
> usage.rst goes into a detailed about faking out classes, but currently
> lacks wording about how one might idiomatically test a range of inputs.
>
> Give an example of how one might test a hash function via macros/helper
> funcs and a table-driven test and very briefly discuss pros and cons.
>
> Also highlight the KUNIT_EXPECT_*_MSG() variants (that aren't mentioned
> elsewhere [1]) which are particularly useful in these situations.
>
> It is also criminally underused at the moment, only appearing in 2
> tests (both written by people involved in KUnit).
>
> [1] not even on
> https://www.kernel.org/doc/html/latest/dev-tools/kunit/api/test.html
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>

Aside from the minor comment I made below, I like the patch; it is a
definite improvement, but I think the test you wrote that ultimately
led to this documentation fix had more information in it than this
documentation. I think it only contains the pattern that you outlined
here, but I think it does include some other best practices. Maybe we
should add some more documentation patches with more code examples in
the future?

Anyway, like I said, I think this patch in and of itself looks pretty good.

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

> ---
>  Documentation/dev-tools/kunit/usage.rst | 66 +++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
>
> diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
> index 62142a47488c..317390df2b96 100644
> --- a/Documentation/dev-tools/kunit/usage.rst
> +++ b/Documentation/dev-tools/kunit/usage.rst
> @@ -451,6 +451,72 @@ We can now use it to test ``struct eeprom_buffer``:
>                 destroy_eeprom_buffer(ctx->eeprom_buffer);
>         }
>
> +Testing various inputs
> +----------------------

Since this, by my count, the second test pattern that we are
introducing here, could we maybe call that out with a subheading or a
new section or something? It would be nice if we could sort of build
up a cookbook of testing patterns.

> +Testing just a few inputs might not be enough to have confidence that the code
> +works correctly, e.g. for a hash function.
> +
> +In such cases, it can be helpful to have a helper macro or function, e.g. this
> +fictitious example for ``md5sum(1)``
> +
> +.. code-block:: c
> +
> +       /* Note: the cast is to satisfy overly strict type-checking. */
> +       #define TEST_MD5(in, want) \
> +               md5sum(in, out); \
> +               KUNIT_EXPECT_STREQ_MSG(test, (char *)out, want, "md5sum(%s)", in);
> +
> +       char out[16];
> +       TEST_MD5("hello world",   "5eb63bbbe01eeed093cb22bb8f5acdc3");
> +       TEST_MD5("hello world!",  "fc3ff98e8c6a0d3087d515c0473f8677");
> +
> +Note the use of ``KUNIT_EXPECT_STREQ_MSG`` to give more context when it fails
> +and make it easier to track down. (Yes, in this example, ``want`` is likely
> +going to be unique enough on its own).
> +
> +The ``_MSG`` variants are even more useful when the same expectation is called
> +multiple times (in a loop or helper function) and thus the line number isn't
> +enough to identify what failed, like below.
> +
> +In some cases, it can be helpful to write a *table-driven test* instead, e.g.
> +
> +.. code-block:: c
> +
> +       int i;
> +       char out[16];
> +
> +       struct md5_test_case {
> +               const char *str;
> +               const char *md5;
> +       };
> +
> +       struct md5_test_case cases[] = {
> +               {
> +                       .str = "hello world",
> +                       .md5 = "5eb63bbbe01eeed093cb22bb8f5acdc3",
> +               },
> +               {
> +                       .str = "hello world!",
> +                       .md5 = "fc3ff98e8c6a0d3087d515c0473f8677",
> +               },
> +       };
> +       for (i = 0; i < ARRAY_SIZE(cases); ++i) {
> +               md5sum(cases[i].str, out);
> +               KUNIT_EXPECT_STREQ_MSG(test, (char *)out, cases[i].md5,
> +                                     "md5sum(%s)", cases[i].str);
> +       }
> +
> +
> +There's more boilerplate involved, but it can:
> +
> +* be more readable when there are multiple inputs/outputs thanks to field names,
> +
> +  * E.g. see ``fs/ext4/inode-test.c`` for an example of both.
> +* reduce duplication if test cases can be shared across multiple tests.
> +
> +  * E.g. if we had a magical ``undo_md5sum`` function, we could reuse ``cases``.
> +
>  .. _kunit-on-non-uml:
>
>  KUnit on non-UML architectures
>
> base-commit: 77c8473edf7f7664137f555cfcdc8c460bbd947d
> --
> 2.29.1.341.ge80a0c044ae-goog
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Documentation: kunit: provide guidance for testing many inputs
  2020-11-23 18:32 ` Brendan Higgins
@ 2020-11-23 22:13   ` Daniel Latypov
  0 siblings, 0 replies; 5+ messages in thread
From: Daniel Latypov @ 2020-11-23 22:13 UTC (permalink / raw)
  To: Brendan Higgins
  Cc: Andy Shevchenko, David Gow, Linux Kernel Mailing List,
	open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan

On Mon, Nov 23, 2020 at 10:32 AM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> On Mon, Nov 2, 2020 at 1:37 PM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > usage.rst goes into a detailed about faking out classes, but currently
> > lacks wording about how one might idiomatically test a range of inputs.
> >
> > Give an example of how one might test a hash function via macros/helper
> > funcs and a table-driven test and very briefly discuss pros and cons.
> >
> > Also highlight the KUNIT_EXPECT_*_MSG() variants (that aren't mentioned
> > elsewhere [1]) which are particularly useful in these situations.
> >
> > It is also criminally underused at the moment, only appearing in 2
> > tests (both written by people involved in KUnit).
> >
> > [1] not even on
> > https://www.kernel.org/doc/html/latest/dev-tools/kunit/api/test.html
> >
> > Signed-off-by: Daniel Latypov <dlatypov@google.com>
>
> Aside from the minor comment I made below, I like the patch; it is a
> definite improvement, but I think the test you wrote that ultimately
> led to this documentation fix had more information in it than this
> documentation. I think it only contains the pattern that you outlined
> here, but I think it does include some other best practices. Maybe we
> should add some more documentation patches with more code examples in
> the future?
>
> Anyway, like I said, I think this patch in and of itself looks pretty good.
>
> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
>
> > ---
> >  Documentation/dev-tools/kunit/usage.rst | 66 +++++++++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> >
> > diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
> > index 62142a47488c..317390df2b96 100644
> > --- a/Documentation/dev-tools/kunit/usage.rst
> > +++ b/Documentation/dev-tools/kunit/usage.rst
> > @@ -451,6 +451,72 @@ We can now use it to test ``struct eeprom_buffer``:
> >                 destroy_eeprom_buffer(ctx->eeprom_buffer);
> >         }
> >
> > +Testing various inputs
> > +----------------------
>
> Since this, by my count, the second test pattern that we are
> introducing here, could we maybe call that out with a subheading or a
> new section or something? It would be nice if we could sort of build
> up a cookbook of testing patterns.

Good point, I noticed now the "Organization of this document" section
would need to be updated.
Perhaps something like

-This document is organized into two main sections: Testing and Isolating
-Behavior. The first covers what unit tests are and how to use KUnit to write
-them. The second covers how to use KUnit to isolate code and make it possible
-to unit test code that was otherwise un-unit-testable.
+This document is organized into two main sections: Testing and Common Patterns.
+The first covers what unit tests are and how to use KUnit to write them. The
+second covers common testing patterns, e.g. how to isolate code and make it
+possible to unit test code that was otherwise un-unit-testable.

I'll send out a V2 shortly, changing the example per David's
suggestion and with the above.

>
> > +Testing just a few inputs might not be enough to have confidence that the code
> > +works correctly, e.g. for a hash function.
> > +
> > +In such cases, it can be helpful to have a helper macro or function, e.g. this
> > +fictitious example for ``md5sum(1)``
> > +
> > +.. code-block:: c
> > +
> > +       /* Note: the cast is to satisfy overly strict type-checking. */
> > +       #define TEST_MD5(in, want) \
> > +               md5sum(in, out); \
> > +               KUNIT_EXPECT_STREQ_MSG(test, (char *)out, want, "md5sum(%s)", in);
> > +
> > +       char out[16];
> > +       TEST_MD5("hello world",   "5eb63bbbe01eeed093cb22bb8f5acdc3");
> > +       TEST_MD5("hello world!",  "fc3ff98e8c6a0d3087d515c0473f8677");
> > +
> > +Note the use of ``KUNIT_EXPECT_STREQ_MSG`` to give more context when it fails
> > +and make it easier to track down. (Yes, in this example, ``want`` is likely
> > +going to be unique enough on its own).
> > +
> > +The ``_MSG`` variants are even more useful when the same expectation is called
> > +multiple times (in a loop or helper function) and thus the line number isn't
> > +enough to identify what failed, like below.
> > +
> > +In some cases, it can be helpful to write a *table-driven test* instead, e.g.
> > +
> > +.. code-block:: c
> > +
> > +       int i;
> > +       char out[16];
> > +
> > +       struct md5_test_case {
> > +               const char *str;
> > +               const char *md5;
> > +       };
> > +
> > +       struct md5_test_case cases[] = {
> > +               {
> > +                       .str = "hello world",
> > +                       .md5 = "5eb63bbbe01eeed093cb22bb8f5acdc3",
> > +               },
> > +               {
> > +                       .str = "hello world!",
> > +                       .md5 = "fc3ff98e8c6a0d3087d515c0473f8677",
> > +               },
> > +       };
> > +       for (i = 0; i < ARRAY_SIZE(cases); ++i) {
> > +               md5sum(cases[i].str, out);
> > +               KUNIT_EXPECT_STREQ_MSG(test, (char *)out, cases[i].md5,
> > +                                     "md5sum(%s)", cases[i].str);
> > +       }
> > +
> > +
> > +There's more boilerplate involved, but it can:
> > +
> > +* be more readable when there are multiple inputs/outputs thanks to field names,
> > +
> > +  * E.g. see ``fs/ext4/inode-test.c`` for an example of both.
> > +* reduce duplication if test cases can be shared across multiple tests.
> > +
> > +  * E.g. if we had a magical ``undo_md5sum`` function, we could reuse ``cases``.
> > +
> >  .. _kunit-on-non-uml:
> >
> >  KUnit on non-UML architectures
> >
> > base-commit: 77c8473edf7f7664137f555cfcdc8c460bbd947d
> > --
> > 2.29.1.341.ge80a0c044ae-goog
> >

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-11-23 22:14 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-02 21:36 [PATCH] Documentation: kunit: provide guidance for testing many inputs Daniel Latypov
2020-11-07  4:21 ` David Gow
2020-11-13 18:21   ` Daniel Latypov
2020-11-23 18:32 ` Brendan Higgins
2020-11-23 22:13   ` Daniel Latypov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).