diff mbox series

[RFC,v2,04/10] docs: submitting-patches: Introduce Tested-with:

Message ID 20231205184503.79769-5-Nikolai.Kondrashov@redhat.com
State New
Headers show
Series None | expand

Commit Message

Nikolai Kondrashov Dec. 5, 2023, 6:03 p.m. UTC
Introduce a new tag, 'Tested-with:', documented in the
Documentation/process/submitting-patches.rst file.

The tag is expected to contain the test suite command which was executed
for the commit, and to certify it passed. Additionally, it can contain a
URL pointing to the execution results, after a '#' character.

Prohibit the V: field from containing the '#' character correspondingly.

Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
---
 Documentation/process/submitting-patches.rst | 10 ++++++++++
 MAINTAINERS                                  |  2 +-
 scripts/checkpatch.pl                        |  4 ++--
 3 files changed, 13 insertions(+), 3 deletions(-)

Comments

Jonathan Corbet Dec. 5, 2023, 6:59 p.m. UTC | #1
Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com> writes:

> Introduce a new tag, 'Tested-with:', documented in the
> Documentation/process/submitting-patches.rst file.
>
> The tag is expected to contain the test suite command which was executed
> for the commit, and to certify it passed. Additionally, it can contain a
> URL pointing to the execution results, after a '#' character.
>
> Prohibit the V: field from containing the '#' character correspondingly.
>
> Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
> ---
>  Documentation/process/submitting-patches.rst | 10 ++++++++++
>  MAINTAINERS                                  |  2 +-
>  scripts/checkpatch.pl                        |  4 ++--
>  3 files changed, 13 insertions(+), 3 deletions(-)

I have to ask whether we *really* need to introduce yet another tag for
this.  How are we going to use this information?  Are we going to try to
make a tag for every way in which somebody might test a patch?

Thanks,

jon
Joe Perches Dec. 5, 2023, 7:07 p.m. UTC | #2
On Tue, 2023-12-05 at 11:59 -0700, Jonathan Corbet wrote:
> Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com> writes:
> 
> > Introduce a new tag, 'Tested-with:', documented in the
> > Documentation/process/submitting-patches.rst file.
[]
> I have to ask whether we *really* need to introduce yet another tag for
> this.  How are we going to use this information?  Are we going to try to
> make a tag for every way in which somebody might test a patch?

In general, I think
	Link: <to some url test result>
would be good enough.

And remember that all this goes stale after awhile
and that includes old test suites.
Geert Uytterhoeven Dec. 6, 2023, 10:07 a.m. UTC | #3
On Tue, Dec 5, 2023 at 8:07 PM Joe Perches <joe@perches.com> wrote:
> On Tue, 2023-12-05 at 11:59 -0700, Jonathan Corbet wrote:
> > Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com> writes:
> >
> > > Introduce a new tag, 'Tested-with:', documented in the
> > > Documentation/process/submitting-patches.rst file.
> []
> > I have to ask whether we *really* need to introduce yet another tag for
> > this.  How are we going to use this information?  Are we going to try to
> > make a tag for every way in which somebody might test a patch?
>
> In general, I think
>         Link: <to some url test result>
> would be good enough.

Exactly.  And if you put the test results (or a link) in your patch
below the "---", or in your cover letter, the "Link:" tag pointing to
lore (or something else, unfortunately) that most (but unfortunately
not all) maintainers already add when committing patches allows
anyone to find it.

> And remember that all this goes stale after awhile
> and that includes old test suites.

Yeah...

Isn't the purpose of a "Tested-with:" tag just for the maintainer to
know which patches have been tested with the test suite already, and
which haven't?  I expect reviewers/maintainers to scrutinize (extra)
patches that lack such a tag (or lack the same under the "---"),
and/or run the test suite theirselves.
I.e. does this serve any purpose _after_ the patch has been applied?

Gr{oetje,eeting}s,

                        Geert
Nikolai Kondrashov Dec. 6, 2023, 4:31 p.m. UTC | #4
On 12/5/23 20:59, Jonathan Corbet wrote:
> Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com> writes:
> 
>> Introduce a new tag, 'Tested-with:', documented in the
>> Documentation/process/submitting-patches.rst file.
>>
>> The tag is expected to contain the test suite command which was executed
>> for the commit, and to certify it passed. Additionally, it can contain a
>> URL pointing to the execution results, after a '#' character.
>>
>> Prohibit the V: field from containing the '#' character correspondingly.
>>
>> Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
>> ---
>>   Documentation/process/submitting-patches.rst | 10 ++++++++++
>>   MAINTAINERS                                  |  2 +-
>>   scripts/checkpatch.pl                        |  4 ++--
>>   3 files changed, 13 insertions(+), 3 deletions(-)
> 
> I have to ask whether we *really* need to introduce yet another tag for
> this.  How are we going to use this information?  Are we going to try to
> make a tag for every way in which somebody might test a patch?

How I understand the purpose of this is that, first, people want to encourage 
submitters to test their patches with the relevant test suites, and second, if 
they do, to tell them they did. That is all.

The idea of Tested-with: is to specify *which* test was executed, so I don't 
think we would need another tag.

However, I let people (all copied) who expressed interest in this in the first 
place, and had this discussed earlier, chime in.

I have no specific interest in this particular way, I just want kernel testing 
to improve. If it was for me, I'd rather encourage everyone to just use GitLab 
or GitHub, post MRs/PRs (like millions of other projects do, including other 
operating systems), have tests executed automatically, results recorded and 
archived automatically, commits linked to those results automatically, and not 
mess around with any tags :D

Nick
Nikolai Kondrashov Dec. 6, 2023, 4:46 p.m. UTC | #5
On 12/5/23 21:07, Joe Perches wrote:
> On Tue, 2023-12-05 at 11:59 -0700, Jonathan Corbet wrote:
>> Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com> writes:
>>
>>> Introduce a new tag, 'Tested-with:', documented in the
>>> Documentation/process/submitting-patches.rst file.
> []
>> I have to ask whether we *really* need to introduce yet another tag for
>> this.  How are we going to use this information?  Are we going to try to
>> make a tag for every way in which somebody might test a patch?
> 
> In general, I think
> 	Link: <to some url test result>
> would be good enough.
> 
> And remember that all this goes stale after awhile
> and that includes old test suites.

Yeah, things go stale, for sure. And Link: will work for specifying the test 
results (provided the contents says what the test was), but it doesn't help 
maintainers to know immediately which tests were executed and which weren't.

It also won't allow involving checkpatch.pl in checking the submitter ran all 
the required tests, and telling them to run whatever they didn't.

Nick
diff mbox series

Patch

diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
index f034feaf1369e..2004df2ac1b39 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -245,6 +245,16 @@  Execute the commands, if any, to test your changes.
 All commands must be executed from the root of the source tree. Each command
 outputs usage information, if an -h/--help option is specified.
 
+If a test suite you've executed completed successfully, add a ``Tested-with:
+<command>`` to the message of the commit you tested. E.g.::
+
+  Tested-with: tools/testing/kunit/run_checks.py
+
+Optionally, add a '#' character followed by a publicly-accessible URL
+containing the test results, if you make them available. E.g.::
+
+  Tested-with: tools/testing/kunit/run_checks.py # https://kernelci.org/test/2239874
+
 
 Select the recipients for your patch
 ------------------------------------
diff --git a/MAINTAINERS b/MAINTAINERS
index 68821eecf61cf..28fbb0eb335ba 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -63,7 +63,7 @@  Descriptions of section entries and preferred order
 	   executed to verify changes to the maintained subsystem.
 	   Must be executed from the root of the source tree.
 	   Must support the -h/--help option.
-	   Cannot contain '@' character.
+	   Cannot contain '@' or '#' characters.
 	   V: tools/testing/kunit/run_checks.py
 	   One test suite per line.
 
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a184e576c980b..bea602c30df5d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3686,9 +3686,9 @@  sub process {
 # check MAINTAINERS V: entries are valid
 			if ($rawline =~ /^\+V:\s*(.*)/) {
 				my $name = $1;
-				if ($name =~ /@/) {
+				if ($name =~ /[@#]/) {
 					ERROR("TEST_PROPOSAL_INVALID",
-					      "Test proposal cannot contain '\@' character\n" . $herecurr);
+					      "Test proposal cannot contain '\@' or '#' characters\n" . $herecurr);
 				}
 			}
 		}