[U-Boot] [PATCH] RFC: checkpatch: Add a check for tests needed for uclasses
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)$/);
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 addedif ($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
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 addedif ($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
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 addedif ($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
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 addedif ($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
participants (2)
-
Heinrich Schuchardt -
Simon Glass