From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DBC7CC2B9F4 for ; Mon, 14 Jun 2021 21:08:47 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A3F25611EE for ; Mon, 14 Jun 2021 21:08:47 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233356AbhFNVKu (ORCPT ); Mon, 14 Jun 2021 17:10:50 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:53030 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229771AbhFNVKt (ORCPT ); Mon, 14 Jun 2021 17:10:49 -0400 Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: tonyk) with ESMTPSA id 396061F42D9D Subject: Re: [PATCH v3 1/1] lib: Convert UUID runtime test to KUnit To: Daniel Latypov , Christoph Hellwig Cc: Andy Shevchenko , Linux Kernel Mailing List , Brendan Higgins , "open list:KERNEL SELFTEST FRAMEWORK" , KUnit Development , Shuah Khan , ~lkcamp/patches@lists.sr.ht, nfraprado@collabora.com, leandro.ribeiro@collabora.com, Vitor Massaru Iha , lucmaga@gmail.com, David Gow , tales.aparecida@gmail.com References: <20210610163959.71634-1-andrealmeid@collabora.com> <20210610163959.71634-2-andrealmeid@collabora.com> <20210614064205.GA29220@lst.de> From: =?UTF-8?Q?Andr=c3=a9_Almeida?= Message-ID: Date: Mon, 14 Jun 2021 18:08:37 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kselftest@vger.kernel.org Às 13:55 de 14/06/21, Daniel Latypov escreveu: > On Sun, Jun 13, 2021 at 11:42 PM Christoph Hellwig wrote: >> >>> +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? > > But putting them in a "kunit namespace" by prefixing them as such > would be misleading, IMO. > The tests live adjacent to the code they test and are owned by the > same maintainers, or at least that's the intent. > > And if the goal is just to find configs, then I don't see much > difference between "config.*KUNIT_TEST" and "config KUNIT_TEST.*" > >> >>> -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? > > No, this is not a pattern. > The structs can be renamed back. > The idea behind this renaming is to be more explicit about what this data is about: correct UUIDs inputs. >> >>> +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 *". > > There is no need for the cast or the brace, yes. > This is my fault. > > The documentation has both since I had thought that would make how it > works more clear: > https://www.kernel.org/doc/html/latest/dev-tools/kunit/usage.html#parameterized-testing > I don't really understand my past thought process... > Ok, I'll change my code to remove the cast and braces. I can also send a patch to rework this part of documentation. >> >> 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.