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=-14.4 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL 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 BE5E2C2D0EE for ; Tue, 31 Mar 2020 18:09:12 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 92986214D8 for ; Tue, 31 Mar 2020 18:09:12 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="Jf60Gj0y" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727752AbgCaSJL (ORCPT ); Tue, 31 Mar 2020 14:09:11 -0400 Received: from mail-pl1-f194.google.com ([209.85.214.194]:44539 "EHLO mail-pl1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725947AbgCaSJK (ORCPT ); Tue, 31 Mar 2020 14:09:10 -0400 Received: by mail-pl1-f194.google.com with SMTP id h11so8397229plr.11 for ; Tue, 31 Mar 2020 11:09:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=0m7IBpCXDOsGY+DSPyHx/xVYlTu36NeTdj5dYNKeXyM=; b=Jf60Gj0y7W+cyaBNFOzgemAgYw1uMEq3IlcSIzq9v1BofdlUZN/M99Ao+8+sMjnJYv qzRUJu/MiBIF1Ia72bC7VxMY4OEjyWro8FmpvWlvZQtcdp1xUYB8I1I3/GZDe5+SUhaF 9MoJSDrFKsUHsm8K4RArk7UtKmEMVkFZPNiz/KT34/NzYlZ6vw2Kap0Hiiq4VJ6w62uQ k5j84nspCKXerWktQfCkMgP2P9FhrQRQ2QgbQMvRaFOa62+ByjOxy4MJi92pU10yyBhb hXv+YiE26NM0X3tqKWAmZ4TVF3rJUT2XKx8EMkVCF7iuMFMPYzNB0YCbc4L4vsLBITbg 5Btw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=0m7IBpCXDOsGY+DSPyHx/xVYlTu36NeTdj5dYNKeXyM=; b=lRGgOaG004aI2ftoshEZu4VoKvQkKlyt3DR6R3uRLWLbh6KMUOoidv0Ri4vk/r7uE5 xhouXzcDt+o44jGEq6Nb5g4fOxzQqBYMtFfWbpNt/WMPIkC1AtmZ5lcYwTKBMl8Sh9D+ ZRrTLw7aODywjIPB0cWTH6XfluzF0+yTt8VkP8a6Cpbiq/XKAF9tTUSlX29uuPmTCmru MXBMMKU8ZDn5PEPFI9Sh7JmT9eAWR/ileXzEmM63hSdPwAiXGUK5KwFWfiSS251AcqiL PCP7nIGKm2QvD4aCnAKmb0+nJLsu8iRgURcdWNs1SPEYG9PM8mUgJpSGIIQJYp9ni0B5 RDeQ== X-Gm-Message-State: ANhLgQ2eXoG7FvJUukVPEdAvR8nttpD7tFQIw87uD8X9hud5ZQjluEze R1K4ePSEx23iyDp8i1TvUDmaYh4HLV+GsbqLoADGqw== X-Google-Smtp-Source: ADFU+vvzhxk52SK4jXu2EJF8XW24gPNMWVQvcEjxsdA0MOrrYuSfyVeAUnGaSwdYVlkamVi0/gm54g1mhEF/XKMfKts= X-Received: by 2002:a17:902:8ec1:: with SMTP id x1mr18956525plo.325.1585678148400; Tue, 31 Mar 2020 11:09:08 -0700 (PDT) MIME-Version: 1.0 References: <285da2166eadc1d46667dd9659d8dae74d28b0b9.1585656143.git.matti.vaittinen@fi.rohmeurope.com> In-Reply-To: <285da2166eadc1d46667dd9659d8dae74d28b0b9.1585656143.git.matti.vaittinen@fi.rohmeurope.com> From: Brendan Higgins Date: Tue, 31 Mar 2020 11:08:56 -0700 Message-ID: Subject: Re: [PATCH v7 04/10] lib/test_linear_ranges: add a test for the 'linear_ranges' To: Matti Vaittinen Cc: mazziesaccount@gmail.com, Greg Kroah-Hartman , Linus Walleij , Mikko Mutanen , Markus Laine , Andrew Morton , Andy Shevchenko , Andy Shevchenko , Ard Biesheuvel , Borislav Petkov , Changbin Du , Dan Williams , David Gow , "David S. Miller" , devicetree , Gary Hook , Herbert Xu , Krzysztof Kozlowski , Linux Kernel Mailing List , linux-pm@vger.kernel.org, Mark Rutland , Masahiro Yamada , Masami Hiramatsu , Mikhail Zaslonko , Randy Dunlap , Rob Herring , Sebastian Reichel , Shuah Khan , Tal Gilboa , Thomas Gleixner , =?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= , Vincenzo Frascino , Vladimir Oltean , Mark Brown , Liam Girdwood Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Mar 31, 2020 at 5:23 AM Matti Vaittinen wrote: > > Add a KUnit test for the linear_ranges helper. > > Signed-off-by: Matti Vaittinen One minor nit, other than that: Reviewed-by: Brendan Higgins > --- > > No changes since v6 > > lib/Kconfig.debug | 11 ++ > lib/Makefile | 1 + > lib/test_linear_ranges.c | 228 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 240 insertions(+) > create mode 100644 lib/test_linear_ranges.c > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index 69def4a9df00..32f355db4163 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -2053,6 +2053,17 @@ config LIST_KUNIT_TEST > > If unsure, say N. > > +config LINEAR_RANGES_TEST > + tristate "KUnit test for linear_ranges" > + depends on KUNIT > + help > + This builds the linear_ranges unit test, which runs on boot. > + Tests the linear_ranges logic correctness. > + For more information on KUnit and unit tests in general please refer > + to the KUnit documentation in Documentation/dev-tools/kunit/. > + > + If unsure, say N. > + > config TEST_UDELAY > tristate "udelay test driver" > help > diff --git a/lib/Makefile b/lib/Makefile > index 18c3d313872e..200aa1780f92 100644 > --- a/lib/Makefile > +++ b/lib/Makefile > @@ -301,3 +301,4 @@ obj-$(CONFIG_OBJAGG) += objagg.o > > # KUnit tests > obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o > +obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o > diff --git a/lib/test_linear_ranges.c b/lib/test_linear_ranges.c > new file mode 100644 > index 000000000000..676e0b8abcdd > --- /dev/null > +++ b/lib/test_linear_ranges.c > @@ -0,0 +1,228 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * KUnit test for the linear_ranges helper. > + * > + * Copyright (C) 2020, ROHM Semiconductors. > + * Author: Matti Vaittinen > + */ > +#include > + > +#include > + > +/* First things first. I deeply dislike unit-tests. I have seen all the hell > + * breaking loose when people who think the unit tests are "the silver bullet" > + * to kill bugs get to decide how a company should implement testing strategy... > + * > + * Believe me, it may get _really_ ridiculous. It is tempting to think that > + * walking through all the possible execution branches will nail down 100% of > + * bugs. This may lead to ideas about demands to get certain % of "test > + * coverage" - measured as line coverage. And that is one of the worst things > + * you can do. > + * > + * Ask people to provide line coverage and they do. I've seen clever tools > + * which generate test cases to test the existing functions - and by default > + * these tools expect code to be correct and just generate checks which are > + * passing when ran against current code-base. Run this generator and you'll get > + * tests that do not test code is correct but just verify nothing changes. > + * Problem is that testing working code is pointless. And if it is not > + * working, your test must not assume it is working. You won't catch any bugs > + * by such tests. What you can do is to generate a huge amount of tests. > + * Especially if you were are asked to proivde 100% line-coverage x_x. So what > + * does these tests - which are not finding any bugs now - do? I don't entirely disagree. I have worked on projects that do testing well where it actually makes development faster, and I have worked on projects that do testing poorly where it never improves code quality and is just an encumbrance, and I have never seen a project get to 100% coverage (nor would I want to). Do you feel differently about incremental coverage vs. absolute coverage? I have found incremental coverage to be a lot more valuable in my experiences. You seem pretty passionate about this. Would you like to be included in our unit testing discussions in the future? > + * They add inertia to every future development. I think it was Terry Pratchet > + * who wrote someone having same impact as thick syrup has to chronometre. > + * Excessive amount of unit-tests have this effect to development. If you do > + * actually find _any_ bug from code in such environment and try fixing it... > + * ...chances are you also need to fix the test cases. In sunny day you fix one > + * test. But I've done refactoring which resulted 500+ broken tests (which had > + * really zero value other than proving to managers that we do do "quality")... > + * > + * After this being said - there are situations where UTs can be handy. If you > + * have algorithms which take some input and should produce output - then you > + * can implement few, carefully selected simple UT-cases which test this. I've > + * previously used this for example for netlink and device-tree data parsing > + * functions. Feed some data examples to functions and verify the output is as > + * expected. I am not covering all the cases but I will see the logic should be > + * working. > + * > + * Here we also do some minor testing. I don't want to go through all branches > + * or test more or less obvious things - but I want to see the main logic is > + * working. And I definitely don't want to add 500+ test cases that break when > + * some simple fix is done x_x. So - let's only add few, well selected tests > + * which ensure as much logic is good as possible. > + */ > + > +/* > + * Test Range 1: > + * selectors: 2 3 4 5 6 > + * values (5): 10 20 30 40 50 > + * > + * Test Range 2: > + * selectors: 7 8 9 10 > + * values (4): 100 150 200 250 > + */ > + > +#define RANGE1_MIN 10 > +#define RANGE1_MIN_SEL 2 > +#define RANGE1_STEP 10 > + > +/* 2, 3, 4, 5, 6 */ > +static const unsigned int range1_sels[] = { RANGE1_MIN_SEL, RANGE1_MIN_SEL + 1, > + RANGE1_MIN_SEL + 2, > + RANGE1_MIN_SEL + 3, > + RANGE1_MIN_SEL + 4 }; > +/* 10, 20, 30, 40, 50 */ > +static const unsigned int range1_vals[] = { RANGE1_MIN, RANGE1_MIN + > + RANGE1_STEP, > + RANGE1_MIN + RANGE1_STEP * 2, > + RANGE1_MIN + RANGE1_STEP * 3, > + RANGE1_MIN + RANGE1_STEP * 4 }; > + > +#define RANGE2_MIN 100 > +#define RANGE2_MIN_SEL 7 > +#define RANGE2_STEP 50 > + > +/* 7, 8, 9, 10 */ > +static const unsigned int range2_sels[] = { RANGE2_MIN_SEL, RANGE2_MIN_SEL + 1, > + RANGE2_MIN_SEL + 2, > + RANGE2_MIN_SEL + 3 }; > +/* 100, 150, 200, 250 */ > +static const unsigned int range2_vals[] = { RANGE2_MIN, RANGE2_MIN + > + RANGE2_STEP, > + RANGE2_MIN + RANGE2_STEP * 2, > + RANGE2_MIN + RANGE2_STEP * 3 }; > + > +#define RANGE1_NUM_VALS (ARRAY_SIZE(range1_vals)) > +#define RANGE2_NUM_VALS (ARRAY_SIZE(range2_vals)) > +#define RANGE_NUM_VALS (RANGE1_NUM_VALS + RANGE2_NUM_VALS) > + > +#define RANGE1_MAX_SEL (RANGE1_MIN_SEL + RANGE1_NUM_VALS - 1) > +#define RANGE1_MAX_VAL (range1_vals[RANGE1_NUM_VALS - 1]) > + > +#define RANGE2_MAX_SEL (RANGE2_MIN_SEL + RANGE2_NUM_VALS - 1) > +#define RANGE2_MAX_VAL (range2_vals[RANGE2_NUM_VALS - 1]) > + > +#define SMALLEST_SEL RANGE1_MIN_SEL > +#define SMALLEST_VAL RANGE1_MIN > + > +static struct linear_range testr[] = { > + { > + .min = RANGE1_MIN, > + .min_sel = RANGE1_MIN_SEL, > + .max_sel = RANGE1_MAX_SEL, > + .step = RANGE1_STEP, > + }, { > + .min = RANGE2_MIN, > + .min_sel = RANGE2_MIN_SEL, > + .max_sel = RANGE2_MAX_SEL, > + .step = RANGE2_STEP > + }, > +}; > + > +static void range_test_get_value(struct kunit *test) > +{ > + int ret, i; > + unsigned int sel, val; > + > + for (i = 0; i < RANGE1_NUM_VALS; i++) { > + sel = range1_sels[i]; > + ret = linear_range_get_value_array(&testr[0], 2, sel, &val); > + KUNIT_EXPECT_EQ(test, 0, ret); nit: It looks like the next line might crash if this expectation fails. If this is the case, you might want to use a KUNIT_ASSERT_* here. > + KUNIT_EXPECT_EQ(test, val, range1_vals[i]); > + } > + for (i = 0; i < RANGE2_NUM_VALS; i++) { > + sel = range2_sels[i]; > + ret = linear_range_get_value_array(&testr[0], 2, sel, &val); > + KUNIT_EXPECT_EQ(test, 0, ret); > + KUNIT_EXPECT_EQ(test, val, range2_vals[i]); > + } > + ret = linear_range_get_value_array(&testr[0], 2, sel + 1, &val); > + KUNIT_EXPECT_NE(test, 0, ret); > +} > + > +static void range_test_get_selector_high(struct kunit *test) > +{ > + int ret, i; > + unsigned int sel; > + bool found; > + > + for (i = 0; i < RANGE1_NUM_VALS; i++) { > + ret = linear_range_get_selector_high(&testr[0], range1_vals[i], > + &sel, &found); > + KUNIT_EXPECT_EQ(test, 0, ret); > + KUNIT_EXPECT_EQ(test, sel, range1_sels[i]); > + KUNIT_EXPECT_TRUE(test, found); > + } > + > + ret = linear_range_get_selector_high(&testr[0], RANGE1_MAX_VAL + 1, > + &sel, &found); > + KUNIT_EXPECT_LE(test, ret, 0); > + > + ret = linear_range_get_selector_high(&testr[0], RANGE1_MIN - 1, > + &sel, &found); > + KUNIT_EXPECT_EQ(test, 0, ret); > + KUNIT_EXPECT_FALSE(test, found); > + KUNIT_EXPECT_EQ(test, sel, range1_sels[0]); > +} > + > +static void range_test_get_value_amount(struct kunit *test) > +{ > + int ret; > + > + ret = linear_range_values_in_range_array(&testr[0], 2); > + KUNIT_EXPECT_EQ(test, (int)RANGE_NUM_VALS, ret); > +} > + > +static void range_test_get_selector_low(struct kunit *test) > +{ > + int i, ret; > + unsigned int sel; > + bool found; > + > + for (i = 0; i < RANGE1_NUM_VALS; i++) { > + ret = linear_range_get_selector_low_array(&testr[0], 2, > + range1_vals[i], &sel, > + &found); > + KUNIT_EXPECT_EQ(test, 0, ret); > + KUNIT_EXPECT_EQ(test, sel, range1_sels[i]); > + KUNIT_EXPECT_TRUE(test, found); > + } > + for (i = 0; i < RANGE2_NUM_VALS; i++) { > + ret = linear_range_get_selector_low_array(&testr[0], 2, > + range2_vals[i], &sel, > + &found); > + KUNIT_EXPECT_EQ(test, 0, ret); > + KUNIT_EXPECT_EQ(test, sel, range2_sels[i]); > + KUNIT_EXPECT_TRUE(test, found); > + } > + > + /* > + * Seek value greater than range max => get_selector_*_low should > + * return Ok - but set found to false as value is not in range > + */ > + ret = linear_range_get_selector_low_array(&testr[0], 2, > + range2_vals[RANGE2_NUM_VALS - 1] + 1, > + &sel, &found); > + > + KUNIT_EXPECT_EQ(test, 0, ret); > + KUNIT_EXPECT_EQ(test, sel, range2_sels[RANGE2_NUM_VALS - 1]); > + KUNIT_EXPECT_FALSE(test, found); > +} > + > +static struct kunit_case range_test_cases[] = { > + KUNIT_CASE(range_test_get_value_amount), > + KUNIT_CASE(range_test_get_selector_high), > + KUNIT_CASE(range_test_get_selector_low), > + KUNIT_CASE(range_test_get_value), > + {}, > +}; > + > +static struct kunit_suite range_test_module = { > + .name = "linear-ranges-test", > + .test_cases = range_test_cases, > +}; > + > +kunit_test_suites(&range_test_module); > + > +MODULE_LICENSE("GPL"); > -- > 2.21.0 > > > -- > Matti Vaittinen, Linux device drivers > ROHM Semiconductors, Finland SWDC > Kiviharjunlenkki 1E > 90220 OULU > FINLAND > > ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ > Simon says - in Latin please. > ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~ > Thanks to Simon Glass for the translation =]