Re: [PATCH v5 1/8] drivers: kunit: Generic helpers for test device creation

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 3/23/23 10:58, Greg Kroah-Hartman wrote:
> On Thu, Mar 23, 2023 at 07:17:40AM +0000, Vaittinen, Matti wrote:
>> On 3/22/23 20:57, Greg Kroah-Hartman wrote:
>>> On Wed, Mar 22, 2023 at 03:48:00PM +0200, Matti Vaittinen wrote:
>>>> Hi Greg,
>>>>
>>>> Thanks for looking at this.
>>>>
>>>> On 3/22/23 14:07, Greg Kroah-Hartman wrote:
>>>>> On Wed, Mar 22, 2023 at 11:05:55AM +0200, Matti Vaittinen wrote:

>> I am very conservative what comes to adding unit tests due to the huge
>> inertia they add to any further development. I usually only add tests to
>> APIs which I know won't require changing (I don't know such in-kernel
>> APIs)
> 
> So anything that is changing doesn't get a test? 

No. I think you misread me. I didn't say I don't like adding tests to 
code which changes. I said, I don't like adding tests to APIs which change.

  If you only test
> things that don't change then no tests fail, and so, why have the test
> at all?

Because implementation cascading into functions below an API may change 
even if the API stays unchanged.

> On the contrary, tests should be used to verify things that are changing
> all the time, to ensure that we don't break things.

This is only true when your test code stays valid. Problem with 
excessive amount of tests is that more we have callers for an API, 
harder changing that API becomes. I've seen a point where people stop 
fixing "unimportant" things just because the amount of work fixing all 
impacted UT-cases would take. I know that many things went wrong before 
that project ended up to the point - but what I picked up with me is 
that carelessly added UTs do really hinder further development.

  That's why we need
> them, not to just validate that old code still is going ok.
> 
> The driver core is changing, and so, I would love to see tests for it to
> ensure that I don't break anything over time.  That should NOT slow down
> development but rather, speed it up as it ensures that things still work
> properly.

I agree that there are cases where UTs are very handy and can add 
confidence that things work as intended. Still, my strong opinion is 
that people should consider what parts of code are really worth testing 
- and how to do the tests so that the amount of maintenance required by 
the tests stays low. It's definitely _not fun_ to do refactoring for 
minor improvement when 400+ unit-test cases break. It's a point when 
many developers start seeing fixing this minor culprit much less 
important... And when people stop fixing minor things ... major things 
start to be just around the corner.

> 
>> - or to functions which I think are error-prone. So, I am probably
>> one of the last persons adding UTs to code I don't know :)
> 
> That's fine, you don't have to add test code for stuff you don't know.
> 
> But again, do NOT abuse a platform device for this, that's not ok, and
> the in-kernel code that does do this should be fixed up.

As suggested by David in another mail - I'll go with the 
root_device_[un]register(). I'll drop this patch entirely. Thanks for 
help, this was once again very educating :)

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~