| Message ID | 20231205184503.79769-5-Nikolai.Kondrashov@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | MAINTAINERS: Introduce V: entry for tests | expand |
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
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.
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
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
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 --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); } } }
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(-)