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

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

[Simon Glass]

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@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)$/);
 
-- 
2.19.1.1215.g8438c0b245-goog

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

[Heinrich Schuchardt]

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

Tests are needed for any new functionality. Nothing special about
u-classes here. The reviewer/custodian should take care that this is
observed.

Best regards

Heinrich

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

[Simon Glass]

Hi Heinrich,

On Wed, 21 Nov 2018 at 07:35, Heinrich Schuchardt <xypron.glpk@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@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.

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

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

Regards,
Simon

[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@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@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
> 

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

[Simon Glass]

Hi Heinrich,

On Fri, 23 Nov 2018 at 01:28, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 11/22/18 9:50 PM, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Wed, 21 Nov 2018 at 07:35, Heinrich Schuchardt <xypron.glpk@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@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?

Well, perhaps my patch will be accepted upstream. I am not sure how we
are supposed to patch this file in downstream projects.

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

Do you mean we should not have it?

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

Isn't this the whole point of checkpatch? We are trying to get the v1
series to be a close as possible to perfect, to avoid wasting reviewer
time, which is extremely precious, particularly in U-Boot.

Regards,
Simon