On Mon, Nov 23, 2020 at 10:32 AM Brendan Higgins <brendanhiggins@xxxxxxxxxx> wrote: > > On Mon, Nov 2, 2020 at 1:37 PM Daniel Latypov <dlatypov@xxxxxxxxxx> 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@xxxxxxxxxx> > > 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@xxxxxxxxxx> > > > --- > > 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 > >
