[U-Boot] [PATCH] RFC: checkpatch: Add a check for tests needed for uclasses

[Heinrich Schuchardt]

On 11/22/18 9:50 PM, Simon Glass wrote:
> Hi Heinrich,
> 
> On Wed, 21 Nov 2018 at 07:35, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>
>> On 11/18/18 4:16 PM, Simon Glass wrote:
>>> This is an experimental check for adding new uclasses without a test.
>>>
>>> I am not sure of the best way to add U-Boot-specific tests, although in
>>> this case, it would not fire on Linux.
>>>
>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>> ---
>>>
>>>  scripts/checkpatch.pl | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>> index 373094e59ef..2f9edb429d5 100755
>>> --- a/scripts/checkpatch.pl
>>> +++ b/scripts/checkpatch.pl
>>> @@ -3033,6 +3033,13 @@ sub process {
>>>                            "adding a line without newline at end of file\n" . $herecurr);
>>>               }
>>>
>>> +             # ask for a test if a new uclass ID is added
>>> +             if ($realfile =~ /uclass-id.h/ && $line =~ /^\+/) {
>>> +                     WARN("NEW_UCLASS",
>>> +                          "Possible new uclass - make sure to add a test in test/dm/<name>.c\n" . $herecurr);
>>> +             }
>>> +
>>> +
>>>  # check we are in a valid source file C or perl if not then ignore this hunk
>>>               next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);
>>>
>>>
>>
>> I would prefer if this script were a verbatim copy of the Linux
>> upstream. Joe Perches is continually working on it. So let's update to
>> 4.20 instead.
> 
> That's fine, but is not the point of this patch.

Your patch is making updates harder?

> 
>>
>> Tests are needed for any new functionality. Nothing special about
>> u-classes here. The reviewer/custodian should take care that this is
>> observed.
> 
> We have similar checks for updating MAINTAINERS files when
> adding/removing files. I think it is better for people to find out
> that they need to add tests before they send their patches.

This is an upstream function.

> 
> Also we have 73 uclasses but not all have tests, so some have clearly
> slipped through the cracks.

This is not specific for u-classes. With the same reasoning you could
warn on all patches creating a C function, "Did you create a test?" and
for any other change, "Did you update the tests?".

I am with you in that we should aim for 100 % test coverage. But I don't
like nag messages.

Regards

Heinrich

> 
> Regards,
> Simon
>