Message-ID: <4f915b8b8bee36a61ebea62ebf34c61845170ad5.camel@fi.rohmeurope.com>
Date:   Wed, 1 Apr 2020 08:45:05 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     "brendanhiggins@...gle.com" <brendanhiggins@...gle.com>
CC:     "tglx@...utronix.de" <tglx@...utronix.de>,
        "dan.j.williams@...el.com" <dan.j.williams@...el.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "talgi@...lanox.com" <talgi@...lanox.com>,
        "olteanv@...il.com" <olteanv@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "davidgow@...gle.com" <davidgow@...gle.com>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>,
        "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "rdunlap@...radead.org" <rdunlap@...radead.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "yamada.masahiro@...ionext.com" <yamada.masahiro@...ionext.com>,
        "Mutanen, Mikko" <Mikko.Mutanen@...rohmeurope.com>,
        "bp@...e.de" <bp@...e.de>,
        "mhiramat@...nel.org" <mhiramat@...nel.org>,
        "krzk@...nel.org" <krzk@...nel.org>,
        "mazziesaccount@...il.com" <mazziesaccount@...il.com>,
        "andy.shevchenko@...il.com" <andy.shevchenko@...il.com>,
        "andriy.shevchenko@...ux.intel.com" 
        <andriy.shevchenko@...ux.intel.com>,
        "skhan@...uxfoundation.org" <skhan@...uxfoundation.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "Laine, Markus" <Markus.Laine@...rohmeurope.com>,
        "vincenzo.frascino@....com" <vincenzo.frascino@....com>,
        "sre@...nel.org" <sre@...nel.org>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "zaslonko@...ux.ibm.com" <zaslonko@...ux.ibm.com>,
        "uwe@...ine-koenig.org" <uwe@...ine-koenig.org>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>
Subject: Re: [PATCH v7 04/10] lib/test_linear_ranges: add a test for the
 'linear_ranges'

Hello Brendan,

Thanks for taking a look at this :) Much appreciated! I have always
admired you guys who have the patience to do all the reviewing... It's
definitely not my favourite job :/

On Tue, 2020-03-31 at 11:08 -0700, Brendan Higgins wrote:
> On Tue, Mar 31, 2020 at 5:23 AM Matti Vaittinen
> <matti.vaittinen@...rohmeurope.com> wrote:
> >     Add a KUnit test for the linear_ranges helper.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
> 
> One minor nit, other than that:
> 
> Reviewed-by: Brendan Higgins <brendanhiggins@...gle.com>
> 
> > ---
> > 

/// Snip

> > +
> > +/* 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.

I think I have only been dealing with projects measuring absolute
coverage. I think seeing a coverage as %-number is mostly not
interesting to me. What I think could be interesting is showing the
code-paths test has walked through. I believe that code spots that
should be tested should be hand picked by a human. When we look at any
%-number, we do not know what kind of code the test has tested.

> You seem pretty passionate about this. Would you like to be included
> in our unit testing discussions in the future?

I think it would be nice :) I don't expect I will be active talker
there but I really like to know what direction things are proceeding in
general. And who knows, maybe I will have a word to say at times :) So
please, include me if it is not a big thing for you.

//Snip

> > +
> > +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.

Huh. I re-read this and almost agreed with you. Then I re-re-read this
and disagreed. Perhaps we should write an unit-test to test this ;)

The range1_sels and range1_vals arrays should always be of same size.
Thus the crash should not occur here. If RANGE1_NUM_VALS was bad then
we would get the crash already at

> > +               sel = range1_sels[i];

The linear_range_get_value_array() may return non zero value if value
contained in range1_sels[i] is not in the range - but range1_vals[i]
should still be valid memory.

Best Regards
	--Matti



> > --
> > 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 =]

Powered by blists - more mailing lists