[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 at 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 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?

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