From: Christoph Hellwig <hch@lst.de>
To: "André Almeida" <andrealmeid@collabora.com>
Cc: Christoph Hellwig <hch@lst.de>,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
linux-kernel@vger.kernel.org,
Brendan Higgins <brendanhiggins@google.com>,
linux-kselftest@vger.kernel.org, kunit-dev@googlegroups.com,
Shuah Khan <shuah@kernel.org>,
~lkcamp/patches@lists.sr.ht, nfraprado@collabora.com,
leandro.ribeiro@collabora.com,
Vitor Massaru Iha <vitor@massaru.org>,
lucmaga@gmail.com, David Gow <davidgow@google.com>,
Daniel Latypov <dlatypov@google.com>,
tales.aparecida@gmail.com
Subject: Re: [PATCH v3 1/1] lib: Convert UUID runtime test to KUnit
Date: Mon, 14 Jun 2021 08:42:05 +0200 [thread overview]
Message-ID: <20210614064205.GA29220@lst.de> (raw)
In-Reply-To: <20210610163959.71634-2-andrealmeid@collabora.com>
> +config UUID_KUNIT_TEST
> + tristate "Unit test for UUID" if !KUNIT_ALL_TESTS
> + depends on KUNIT
> + default KUNIT_ALL_TESTS
> + help
> + This builds the UUID unit test.
Does this first help line really add any value if we have this second
line:
> + Tests parsing functions for UUID/GUID strings.
?
> + If unsure, say N.
Not specific to this case, but IMHO we can drop this line for all kunit
tests as it is completely obvious.
> @@ -354,5 +353,6 @@ obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
> obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
> obj-$(CONFIG_BITS_TEST) += test_bits.o
> obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o
> +obj-$(CONFIG_UUID_KUNIT_TEST) += test_uuid.o
Another meta-comment on the kunit tests: Wouldn't it make more sense
to name them all as CONFIG_KUNIT_TEST_FOO to allow for easier grepping?
> -struct test_uuid_data {
> +struct test_data {
> const char *uuid;
> guid_t le;
> uuid_t be;
> };
>
> -static const struct test_uuid_data test_uuid_test_data[] = {
> +static const struct test_data correct_data[] = {
What is the reason for these renames? Is this a pattern used for
other kunit tests?
> +static void uuid_correct_le(struct kunit *test)
> {
> + guid_t le;
> + const struct test_data *data = (const struct test_data *)(test->param_value);
Overly long line. But as far as I can tell there is no need for the
case that causes this mess anyway given that param_value is a
"const void *".
Same for all the other instances of this.
> +static void uuid_wrong_le(struct kunit *test)
> {
> guid_t le;
> + const char **data = (const char **)(test->param_value);
No need for the second pair of braces. Same for various other instances.
next prev parent reply other threads:[~2021-06-14 6:42 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-10 16:39 [PATCH v3 0/1] lib: Convert UUID runtime test to KUnit André Almeida
2021-06-10 16:39 ` [PATCH v3 1/1] " André Almeida
2021-06-14 6:42 ` Christoph Hellwig [this message]
2021-06-14 16:55 ` Daniel Latypov
2021-06-14 21:08 ` André Almeida
2021-06-11 9:55 ` [PATCH v3 0/1] " Andy Shevchenko
2021-06-11 10:48 ` André Almeida
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210614064205.GA29220@lst.de \
--to=hch@lst.de \
--cc=andrealmeid@collabora.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=brendanhiggins@google.com \
--cc=davidgow@google.com \
--cc=dlatypov@google.com \
--cc=kunit-dev@googlegroups.com \
--cc=leandro.ribeiro@collabora.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=lucmaga@gmail.com \
--cc=nfraprado@collabora.com \
--cc=shuah@kernel.org \
--cc=tales.aparecida@gmail.com \
--cc=vitor@massaru.org \
--cc=~lkcamp/patches@lists.sr.ht \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).