[RFC PATCH 0/3] MAINTAINERS: Introduce V: field for required tests

[RFC PATCH 0/3] MAINTAINERS: Introduce V: field for required tests

[Nikolai Kondrashov]

The idea of this RFC is to introduce a way to catalogue and document any tests
that should be executed for changes to a subsystem, as well as to make
checkpatch.pl require a tag in commit messages certifying they were, plus
hopefully make it easier to discover and run them.

This is following a discussion Veronika Kabatova started with a few
(addressed) people at the LPC last year (IIRC), where there was a good deal of
interest for something like this.

Apart from implementing basic support (surely to be improved), two sample
changes are added on top, adding a few test suites (roughly) based on what the
maintainers described earlier. I'm definitely not qualified for describing
them adequately, and don't have the time to dig deeper, but hopefully they
could serve as illustrations, and shouldn't be merged as is.

I would defer to maintainers of the corresponding subsystems and tests to
describe their tests and requirements better. Although I would accept
amendments too, if they prefer it that way.

One bug I know that's definitely there is handling removed files. The
scripts/get_maintainer.pl chokes on non-existing files, failing to output the
required test suites (I'm sure there's a good reason, but I couldn't see it).
My first idea is to only check for required tests upon encountering the '+++
<file>' line, and ignore the '/dev/null' file, but I hope the checkpatch.pl
maintainers could recommend a better way.

Anyway, tell me what you think, and I'll work on polishing this.

Thank you!
Nick
---
Nikolai Kondrashov (3):
      MAINTAINERS: Introduce V: field for required tests
      MAINTAINERS: Require kvm-xfstests smoke for ext4
      MAINTAINERS: Require kunit core tests for framework changes

 Documentation/process/submitting-patches.rst |  19 +++++
 Documentation/process/tests.rst              |  80 ++++++++++++++++++
 MAINTAINERS                                  |   8 ++
 scripts/checkpatch.pl                        | 118 ++++++++++++++++++++++++++-
 scripts/get_maintainer.pl                    |  17 +++-
 scripts/parse-maintainers.pl                 |   3 +-
 6 files changed, 241 insertions(+), 4 deletions(-)
---

[PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Nikolai Kondrashov]

Introduce a new 'V:' ("Verify") field to MAINTAINERS. The field accepts
a name of a test suite which is required to be executed for each
contribution to the subsystem.

Each referenced test suite is expected to be documented in the new
Documentation/process/tests.rst file, which must have enough structure
(documented inside) for the tools to make use of it. Apart from basic
data, each test can refer to its "superset" - a test suite which this
one is a part of. The expected use is to describe both a large test
suite and its subsets, so the former would also be accepted, if a
subsystem requires only a subset.

Introduce a new tag, 'Tested-with:', documented in the
Documentation/process/submitting-patches.rst file. The tag is expected
to reference the documented test suites, similarly to the 'V:' field,
and to certify that the submitter executed the test suite on the change,
and that it passed.

Make scripts/checkpatch.pl ensure any added V: fields reference
documented test suites only, and output a warning if a change to a
subsystem doesn't certify the required test suites were executed,
if any.

If the test suite description includes a "Command", then checkpatch.pl
will output it as the one executing the suite. The command should run
with only the kernel tree and the regular developer environment set up.
But, at the same time, could simply output instructions for installing
any extra dependencies (or pull some automatically). The idea is to
get the developer into feedback loop quicker and easier, so they have
something to run and iterate on, even if it involves installing some
more stuff first. Therefore it's a good idea to add such wrappers to the
kernel tree proper and refer to them from the tests.rst.

Extend scripts/get_maintainer.pl to support retrieving the V: fields,
and scripts/parse-maintainers.pl to maintain their ordering.

Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
---
 Documentation/process/submitting-patches.rst |  19 +++
 Documentation/process/tests.rst              |  35 ++++++
 MAINTAINERS                                  |   6 +
 scripts/checkpatch.pl                        | 118 ++++++++++++++++++-
 scripts/get_maintainer.pl                    |  17 ++-
 scripts/parse-maintainers.pl                 |   3 +-
 6 files changed, 194 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/process/tests.rst

diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
index 86d346bcb8ef0..8f0f3c9f753dd 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -228,6 +228,25 @@ You should be able to justify all violations that remain in your
 patch.
 
 
+Test your changes
+-----------------
+
+Test the patch to the best of your ability. Check the MAINTAINERS file for the
+subsystem(s) you are changing to see if there are any **V:** entries requiring
+particular test suites to be executed. If any are required, follow the
+instructions in the Documentation/process/tests.rst under the headings
+matching the V: entries.
+
+If you ran any test suites documented in the Documentation/process/tests.rst
+file, and they passed, add a 'Tested-with: <test_suite>' line to the messages
+of the commits you tested, one for every test suite, substituting
+'<test_suite>' with their names.
+
+If a subsystem you're changing requires a test suite to be executed and the
+commit lacks the 'Tested-with:' line referring to it (or its documented
+superset), scripts/checkpatch.pl will produce a WARNING reminding you to run
+it.
+
 Select the recipients for your patch
 ------------------------------------
 
diff --git a/Documentation/process/tests.rst b/Documentation/process/tests.rst
new file mode 100644
index 0000000000000..907311e91ec45
--- /dev/null
+++ b/Documentation/process/tests.rst
@@ -0,0 +1,35 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+.. _tests:
+
+Tests you can run
+=================
+
+There are many automated tests available for the Linux kernel, and some
+userspace tests which happen to also test the kernel. Here are some of them,
+along with the instructions on where to get them and how to run them for
+various purposes.
+
+This document has to follow a certain structure to allow tool access.
+Second-level headers (underscored with dashes '-') must contain test suite
+names, and the corresponding section must contain the test description.
+
+The test suites can be referred to by name, from the "V:" lines in the
+MAINTAINERS file, as well as from the "Tested-with:" tag in commit messages.
+
+The test suite description should contain the test documentation in general:
+where to get the test, how to run it, and how to interpret its results, but
+could also start with a "field list", with the following entries recognized by
+the tools (regardless of the case):
+
+:Summary: A single-line summary of the test suite
+:Superset: The name of the test suite this one is a subset of
+:Command: A shell command executing the test suite, not requiring setup
+          beyond the kernel tree and regular developer environment
+          (even if only to report what else needs setting up)
+
+Any other entries are accepted, but not processed. The following could be
+particularly useful:
+
+:Source: A URL pointing to the source code of the test suite
+:Docs: A URL pointing to further test suite documentation
diff --git a/MAINTAINERS b/MAINTAINERS
index 5c9f868e13b6e..2565c04f0490e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -59,6 +59,12 @@ Descriptions of section entries and preferred order
 	      matches patches or files that contain one or more of the words
 	      printk, pr_info or pr_err
 	   One regex pattern per line.  Multiple K: lines acceptable.
+	V: *Test* recommended for execution. The name of a test suite
+	   that should be executed for changes to the maintained subsystem.
+	   The test suite must be documented in a structured way in
+	   Documentation/process/tests.rst under a heading of the same name.
+	   V: xfstests
+	   One test suite per line.
 
 Maintainers List
 ----------------
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 25fdb7fda1128..92ee221caa4d3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -66,6 +66,9 @@ my $codespellfile = "/usr/share/codespell/dictionary.txt";
 my $user_codespellfile = "";
 my $conststructsfile = "$D/const_structs.checkpatch";
 my $docsfile = "$D/../Documentation/dev-tools/checkpatch.rst";
+my $testsrelfile = "Documentation/process/tests.rst";
+my $testsfile = "$D/../$testsrelfile";
+my %tests = ();
 my $typedefsfile;
 my $color = "auto";
 my $allow_c99_comments = 1; # Can be overridden by --ignore C99_COMMENT_TOLERANCE
@@ -282,6 +285,39 @@ sub load_docs {
 	close($docs);
 }
 
+sub load_tests {
+	open(my $tests, '<', "$testsfile")
+	    or warn "$P: Can't read the tests file $testsfile $!\n";
+
+	my $name = undef;
+	my $prev_line = undef;
+	my $in_field_list = 0;
+
+	while (<$tests>) {
+		my $line = $_;
+		$line =~ s/\s+$//;
+
+		# If the previous line was a second-level header (test name)
+		if ($line =~ /^-+$/ &&
+		    defined($prev_line) &&
+		    length($line) == length($prev_line)) {
+			$name = $prev_line;
+			$tests{$name} = {};
+			$in_field_list = 1;
+		# Else, if we're parsing the test's header field list
+		} elsif ($in_field_list) {
+			if ($line =~ /^:([^:]+):\s+(.*)/) {
+				$tests{$name}{lc($1)} = $2;
+			} else {
+				$in_field_list = !$line;
+			}
+		}
+
+		$prev_line = $line;
+	}
+	close($tests);
+}
+
 # Perl's Getopt::Long allows options to take optional arguments after a space.
 # Prevent --color by itself from consuming other arguments
 foreach (@ARGV) {
@@ -372,6 +408,7 @@ if ($color =~ /^[01]$/) {
 
 load_docs() if ($verbose);
 list_types(0) if ($list_types);
+load_tests();
 
 $fix = 1 if ($fix_inplace);
 $check_orig = $check;
@@ -1144,6 +1181,26 @@ sub is_maintained_obsolete {
 	return $maintained_status{$filename} =~ /obsolete/i;
 }
 
+# Test suites required per changed file
+our %file_required_tests = ();
+
+# Return a list of test suites required for execution for a particular file
+sub get_file_required_tests {
+	my ($filename) = @_;
+	my $file_required_tests;
+
+	return () if (!$tree || !(-e "$root/scripts/get_maintainer.pl"));
+
+	if (!exists($file_required_tests{$filename})) {
+		my $output = `perl $root/scripts/get_maintainer.pl --test --multiline --nogit --nogit-fallback -f $filename 2>&1`;
+		die "Failed retrieving tests required for changes to \"$filename\":\n$output" if ($?);
+		$file_required_tests{$filename} = [grep { !/@/ } split("\n", $output)]
+	}
+
+	$file_required_tests = $file_required_tests{$filename};
+	return @$file_required_tests;
+}
+
 sub is_SPDX_License_valid {
 	my ($license) = @_;
 
@@ -2689,6 +2746,9 @@ sub process {
 	my @setup_docs = ();
 	my $setup_docs = 0;
 
+	# Test suites which should not be required for execution
+	my %not_required_tests = ();
+
 	my $camelcase_file_seeded = 0;
 
 	my $checklicenseline = 1;
@@ -2907,6 +2967,27 @@ sub process {
 				}
 			}
 
+			# Check if tests are required for changes to the file
+			foreach my $name (get_file_required_tests($realfile)) {
+				next if exists $not_required_tests{$name};
+				my $test_ref = "\"$name\"";
+				my $summary = $tests{$name}{"summary"};
+				my $command = $tests{$name}{"command"};
+				my $instructions = "";
+				if (defined($summary)) {
+					$test_ref = "$summary ($test_ref)";
+				}
+				if (defined($command)) {
+					$instructions .= "\nRun the test with \"$command\".";
+				}
+				$instructions .= "\nSee $testsrelfile for instructions.";
+				WARN("TEST_REQUIRED",
+				     "Changes to $realfile require running $test_ref, " .
+				     "but no corresponding Tested-with: tag found." .
+				     "$instructions");
+				$not_required_tests{$name} = 1;
+			}
+
 			next;
 		}
 
@@ -3233,6 +3314,29 @@ sub process {
 			}
 		}
 
+# Check and accumulate executed test suites
+		if (!$in_commit_log && $line =~ /^\s*Tested-with:\s*(.*)/i) {
+			my $name = $1;
+			my $sub_found = 0;
+			if (!exists $tests{$name}) {
+				ERROR("TEST_NAME",
+				      "Test suite \"$name\" not documented in $testsrelfile\n" . $herecurr);
+			}
+			# Do not require this test suite and all its subsets
+			local *dont_require_test = sub {
+				my ($name) = @_;
+				$not_required_tests{$name} = 1;
+				foreach my $sub_name (keys %tests) {
+					my $sub_data = $tests{$sub_name};
+					my $superset = $sub_data->{"superset"};
+					if (defined($superset) and $superset eq $name) {
+						dont_require_test($sub_name);
+					}
+				}
+			};
+			dont_require_test($name);
+		}
+
 # Check email subject for common tools that don't need to be mentioned
 		if ($in_header_lines &&
 		    $line =~ /^Subject:.*\b(?:checkpatch|sparse|smatch)\b[^:]/i) {
@@ -3657,7 +3761,7 @@ sub process {
 				}
 			}
 # check MAINTAINERS entries for the right ordering too
-			my $preferred_order = 'MRLSWQBCPTFXNK';
+			my $preferred_order = 'MRLSWQBCPVTFXNK';
 			if ($rawline =~ /^\+[A-Z]:/ &&
 			    $prevrawline =~ /^[\+ ][A-Z]:/) {
 				$rawline =~ /^\+([A-Z]):\s*(.*)/;
@@ -3683,6 +3787,18 @@ sub process {
 					}
 				}
 			}
+# check MAINTAINERS V: entries are valid and refer to a documented test suite
+			if ($rawline =~ /^\+V:\s*(.*)/) {
+				my $name = $1;
+				if ($name =~ /@/) {
+					ERROR("TEST_NAME",
+					      "Test suite name cannot contain '@' character\n" . $herecurr);
+				}
+				if (!exists $tests{$name}) {
+					ERROR("TEST_NAME",
+					      "Test suite \"$name\" not documented in $testsrelfile\n" . $herecurr);
+				}
+			}
 		}
 
 		if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) &&
diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 16d8ac6005b6f..d4ae27b67becb 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -53,6 +53,7 @@ my $output_section_maxlen = 50;
 my $scm = 0;
 my $tree = 1;
 my $web = 0;
+my $test = 0;
 my $subsystem = 0;
 my $status = 0;
 my $letters = "";
@@ -270,6 +271,7 @@ if (!GetOptions(
 		'scm!' => \$scm,
 		'tree!' => \$tree,
 		'web!' => \$web,
+		'test!' => \$test,
 		'letters=s' => \$letters,
 		'pattern-depth=i' => \$pattern_depth,
 		'k|keywords!' => \$keywords,
@@ -319,13 +321,14 @@ if ($sections || $letters ne "") {
     $status = 0;
     $subsystem = 0;
     $web = 0;
+    $test = 0;
     $keywords = 0;
     $keywords_in_file = 0;
     $interactive = 0;
 } else {
-    my $selections = $email + $scm + $status + $subsystem + $web;
+    my $selections = $email + $scm + $status + $subsystem + $web + $test;
     if ($selections == 0) {
-	die "$P:  Missing required option: email, scm, status, subsystem or web\n";
+	die "$P:  Missing required option: email, scm, status, subsystem, web or test\n";
     }
 }
 
@@ -630,6 +633,7 @@ my %hash_list_to;
 my @list_to = ();
 my @scm = ();
 my @web = ();
+my @test = ();
 my @subsystem = ();
 my @status = ();
 my %deduplicate_name_hash = ();
@@ -661,6 +665,11 @@ if ($web) {
     output(@web);
 }
 
+if ($test) {
+    @test = uniq(@test);
+    output(@test);
+}
+
 exit($exit);
 
 sub self_test {
@@ -846,6 +855,7 @@ sub get_maintainers {
     @list_to = ();
     @scm = ();
     @web = ();
+    @test = ();
     @subsystem = ();
     @status = ();
     %deduplicate_name_hash = ();
@@ -1068,6 +1078,7 @@ MAINTAINER field selection options:
   --status => print status if any
   --subsystem => print subsystem name if any
   --web => print website(s) if any
+  --test => print test(s) if any
 
 Output type options:
   --separator [, ] => separator for multiple entries on 1 line
@@ -1378,6 +1389,8 @@ sub add_categories {
 		push(@scm, $pvalue . $suffix);
 	    } elsif ($ptype eq "W") {
 		push(@web, $pvalue . $suffix);
+	    } elsif ($ptype eq "V") {
+		push(@test, $pvalue . $suffix);
 	    } elsif ($ptype eq "S") {
 		push(@status, $pvalue . $suffix);
 	    }
diff --git a/scripts/parse-maintainers.pl b/scripts/parse-maintainers.pl
index 2ca4eb3f190d6..dbc2b79bcaa46 100755
--- a/scripts/parse-maintainers.pl
+++ b/scripts/parse-maintainers.pl
@@ -44,6 +44,7 @@ usage: $P [options] <pattern matching regexes>
       B:  URI for bug tracking/submission
       C:  URI for chat
       P:  URI or file for subsystem specific coding styles
+      V:  Test suite name
       T:  SCM tree type and location
       F:  File and directory pattern
       X:  File and directory exclusion pattern
@@ -73,7 +74,7 @@ sub by_category($$) {
 
 sub by_pattern($$) {
     my ($a, $b) = @_;
-    my $preferred_order = 'MRLSWQBCPTFXNK';
+    my $preferred_order = 'MRLSWQBCPVTFXNK';
 
     my $a1 = uc(substr($a, 0, 1));
     my $b1 = uc(substr($b, 0, 1));
-- 
2.42.0

[PATCH 2/3] MAINTAINERS: Require kvm-xfstests smoke for ext4

[Nikolai Kondrashov]

Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
---
 Documentation/process/tests.rst | 32 ++++++++++++++++++++++++++++++++
 MAINTAINERS                     |  1 +
 2 files changed, 33 insertions(+)

diff --git a/Documentation/process/tests.rst b/Documentation/process/tests.rst
index 907311e91ec45..9a9ea3fe65c37 100644
--- a/Documentation/process/tests.rst
+++ b/Documentation/process/tests.rst
@@ -33,3 +33,35 @@ particularly useful:
 
 :Source: A URL pointing to the source code of the test suite
 :Docs: A URL pointing to further test suite documentation
+
+xfstests
+--------
+
+:Summary: File system regression test suite
+:Source: git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git
+:Docs: https://github.com/tytso/xfstests-bld/blob/master/Documentation/what-is-xfstests.md
+
+As the name might imply, xfstests is a file system regression test suite which
+was originally developed by Silicon Graphics (SGI) for the XFS file system.
+Originally, xfstests, like XFS was only supported on the SGI's Irix operating
+system. When XFS was ported to Linux, so was xfstests, and now xfstests is
+only supported on Linux.
+
+Today, xfstests is used as a file system regression test suite for all of
+Linux's major file systems: xfs, ext2, ext4, cifs, btrfs, f2fs, reiserfs, gfs,
+jfs, udf, nfs, and tmpfs. Many file system maintainers will run a full set of
+xfstests before sending patches to Linus, and will require that any major
+changes be tested using xfstests before they are submitted for integration.
+
+The easiest way to start running xfstests is under KVM with xfstests-bld:
+https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
+
+kvm-xfstests smoke
+------------------
+
+:Summary: File system smoke tests
+:Superset: xfstests
+:Docs: https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
+
+The "kvm-xfstests smoke" is a minimal subset of xfstests for testing all major
+file systems, running under KVM.
diff --git a/MAINTAINERS b/MAINTAINERS
index 2565c04f0490e..f81a47d87ac26 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7974,6 +7974,7 @@ L:	linux-ext4@vger.kernel.org
 S:	Maintained
 W:	http://ext4.wiki.kernel.org
 Q:	http://patchwork.ozlabs.org/project/linux-ext4/list/
+V:	kvm-xfstests smoke
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git
 F:	Documentation/filesystems/ext4/
 F:	fs/ext4/
-- 
2.42.0

[PATCH 3/3] MAINTAINERS: Require kunit core tests for framework changes

[Nikolai Kondrashov]

Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
---
 Documentation/process/tests.rst | 13 +++++++++++++
 MAINTAINERS                     |  1 +
 2 files changed, 14 insertions(+)

diff --git a/Documentation/process/tests.rst b/Documentation/process/tests.rst
index 9a9ea3fe65c37..56a7911f69031 100644
--- a/Documentation/process/tests.rst
+++ b/Documentation/process/tests.rst
@@ -65,3 +65,16 @@ kvm-xfstests smoke
 
 The "kvm-xfstests smoke" is a minimal subset of xfstests for testing all major
 file systems, running under KVM.
+
+kunit
+-----
+
+:Summary: The complete set of KUnit unit tests
+:Command: tools/testing/kunit/kunit.py run --alltests
+
+kunit core
+----------
+
+:Summary: KUnit tests for the framework itself
+:Superset: kunit
+:Command: tools/testing/kunit/kunit.py run --kunitconfig lib/kunit
diff --git a/MAINTAINERS b/MAINTAINERS
index f81a47d87ac26..5f3261e96c90f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11626,6 +11626,7 @@ L:	linux-kselftest@vger.kernel.org
 L:	kunit-dev@googlegroups.com
 S:	Maintained
 W:	https://google.github.io/kunit-docs/third_party/kernel/docs/
+V:	kunit core
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git kunit
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git kunit-fixes
 F:	Documentation/dev-tools/kunit/
-- 
2.42.0

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Joe Perches]

On Wed, 2023-11-15 at 19:43 +0200, Nikolai Kondrashov wrote:
> Introduce a new 'V:' ("Verify") field to MAINTAINERS. The field accepts
> a name of a test suite which is required to be executed for each
> contribution to the subsystem.

Perhaps this is simply too much overhead
process requirements for most kernel work.

While the addition of V: seems ok, I think
putting the verification in checkpatch is
odd at best and the verification of test
execution should be a separate script.

Re: [PATCH 2/3] MAINTAINERS: Require kvm-xfstests smoke for ext4

[Darrick J. Wong]

On Wed, Nov 15, 2023 at 07:43:50PM +0200, Nikolai Kondrashov wrote:
> Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
> ---
>  Documentation/process/tests.rst | 32 ++++++++++++++++++++++++++++++++
>  MAINTAINERS                     |  1 +
>  2 files changed, 33 insertions(+)
> 
> diff --git a/Documentation/process/tests.rst b/Documentation/process/tests.rst
> index 907311e91ec45..9a9ea3fe65c37 100644
> --- a/Documentation/process/tests.rst
> +++ b/Documentation/process/tests.rst
> @@ -33,3 +33,35 @@ particularly useful:
>  
>  :Source: A URL pointing to the source code of the test suite
>  :Docs: A URL pointing to further test suite documentation
> +
> +xfstests
> +--------
> +
> +:Summary: File system regression test suite
> +:Source: git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git

You might as well use the https link to the fstests git repo.
https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git

> +:Docs: https://github.com/tytso/xfstests-bld/blob/master/Documentation/what-is-xfstests.md

Awkardly, this github link is nice for rendering the markdown as html,
but I think the canonical source of xfstests-bld is also kernel.org:

https://git.kernel.org/pub/scm/fs/ext2/xfstests-bld.git

> +
> +As the name might imply, xfstests is a file system regression test suite which
> +was originally developed by Silicon Graphics (SGI) for the XFS file system.
> +Originally, xfstests, like XFS was only supported on the SGI's Irix operating
> +system. When XFS was ported to Linux, so was xfstests, and now xfstests is
> +only supported on Linux.
> +
> +Today, xfstests is used as a file system regression test suite for all of
> +Linux's major file systems: xfs, ext2, ext4, cifs, btrfs, f2fs, reiserfs, gfs,
> +jfs, udf, nfs, and tmpfs. Many file system maintainers will run a full set of
> +xfstests before sending patches to Linus, and will require that any major
> +changes be tested using xfstests before they are submitted for integration.
> +
> +The easiest way to start running xfstests is under KVM with xfstests-bld:
> +https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
> +
> +kvm-xfstests smoke
> +------------------
> +
> +:Summary: File system smoke tests
> +:Superset: xfstests

Source: https://git.kernel.org/pub/scm/fs/ext2/xfstests-bld.git

?

> +:Docs: https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
> +
> +The "kvm-xfstests smoke" is a minimal subset of xfstests for testing all major
> +file systems, running under KVM.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2565c04f0490e..f81a47d87ac26 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7974,6 +7974,7 @@ L:	linux-ext4@vger.kernel.org
>  S:	Maintained
>  W:	http://ext4.wiki.kernel.org
>  Q:	http://patchwork.ozlabs.org/project/linux-ext4/list/
> +V:	kvm-xfstests smoke

I wouldn't mind one of these being added to the XFS entry, though I've
cc'd the current and past maintainer(s) of XFS for their input.

--D

>  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git
>  F:	Documentation/filesystems/ext4/
>  F:	fs/ext4/
> -- 
> 2.42.0
> 

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 680 bytes --]

On Wed, Nov 15, 2023 at 10:31:21AM -0800, Joe Perches wrote:
> On Wed, 2023-11-15 at 19:43 +0200, Nikolai Kondrashov wrote:
> > Introduce a new 'V:' ("Verify") field to MAINTAINERS. The field accepts
> > a name of a test suite which is required to be executed for each
> > contribution to the subsystem.

> Perhaps this is simply too much overhead
> process requirements for most kernel work.

> While the addition of V: seems ok, I think
> putting the verification in checkpatch is
> odd at best and the verification of test
> execution should be a separate script.

Verifying that the expected tags are present and valid does seem firmly
in what checkpatch's wheelhouse though?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 770 bytes --]

On Wed, Nov 15, 2023 at 07:43:49PM +0200, Nikolai Kondrashov wrote:

> Introduce a new tag, 'Tested-with:', documented in the
> Documentation/process/submitting-patches.rst file. The tag is expected
> to reference the documented test suites, similarly to the 'V:' field,
> and to certify that the submitter executed the test suite on the change,
> and that it passed.

This doesn't feel like it fits so well with git based flows - generally
the tags end up in git one way or another so there'll be a strong
tendency for this to end up getting added for one version and then
carried forward to the next version.  The way the tooling is at present
it doesn't really feel like there's a good point at which to insert the
tag.

I'm not sure exactly what'd be better though.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Konstantin Ryabitsev]

On Wed, Nov 15, 2023 at 07:43:49PM +0200, Nikolai Kondrashov wrote:
> Introduce a new tag, 'Tested-with:', documented in the
> Documentation/process/submitting-patches.rst file. The tag is expected
> to reference the documented test suites, similarly to the 'V:' field,
> and to certify that the submitter executed the test suite on the change,
> and that it passed.

I'm not sure it should be a tag that stays all the way through git commit.
How about making it a cover letter/first patch footer?

tested-with: <suite>

-K

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Nikolai Kondrashov]

On 11/15/23 20:31, Joe Perches wrote:
> On Wed, 2023-11-15 at 19:43 +0200, Nikolai Kondrashov wrote:
>> Introduce a new 'V:' ("Verify") field to MAINTAINERS. The field accepts
>> a name of a test suite which is required to be executed for each
>> contribution to the subsystem.
> 
> Perhaps this is simply too much overhead
> process requirements for most kernel work.
> 
> While the addition of V: seems ok, I think
> putting the verification in checkpatch is
> odd at best and the verification of test
> execution should be a separate script.

I agree that this extends checkpatch.pl responsibilities somewhat. In the 
sense that it requires you to do something beside changing the patch itself. 
OTOH, checkpatch.pl already requires Signed-off-by:, which prompts you to 
check and clear up your authorship, similarly requiring work outside the patch.

At the same time, you're supposed to test your changes anyway. Sometimes it's 
manual and one-off, but often times running an existing test suite is at least 
beneficial, if not required.

In a sense, this is not *checkpatch.pl* itself requiring testing, but 
subsystem maintainers (who are opting in), and checkpatch.pl simply provides 
convenient means and an entry point for raising attention to maintainer's 
requests, and making it easier to discover the tests.

It also does *not* verify test execution, only alerts the contributors to the 
need, and requires certification. Again, similar to Signed-off-by:.

Nick

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Nikolai Kondrashov]

On 11/15/23 22:14, Mark Brown wrote:
> On Wed, Nov 15, 2023 at 07:43:49PM +0200, Nikolai Kondrashov wrote:
> 
>> Introduce a new tag, 'Tested-with:', documented in the
>> Documentation/process/submitting-patches.rst file. The tag is expected
>> to reference the documented test suites, similarly to the 'V:' field,
>> and to certify that the submitter executed the test suite on the change,
>> and that it passed.
> 
> This doesn't feel like it fits so well with git based flows - generally
> the tags end up in git one way or another so there'll be a strong
> tendency for this to end up getting added for one version and then
> carried forward to the next version.  The way the tooling is at present
> it doesn't really feel like there's a good point at which to insert the
> tag.
> 
> I'm not sure exactly what'd be better though.

Yeah, I agree that's a bit of a problem. One that only automated 
tools/testing/CI could fully solve. Cough, git forges, cough.

OTOH, once you managed to run an automated suite once, it's much easier to do 
it again, and most of the time developers *want* their code to work and pass 
the tests (it's much easier than manual testing after all). So it's likely 
they will keep running them for new revisions, even though they might not 
notice they simply reused the previously-added Tested-with: tag.

Still, one way to make this better could be requiring a URL pointing at test 
results to follow the test suite name in the Tested-with: tag. Then the 
maintainer could check that they're indeed fresh.

This would be however getting way ahead of ourselves and, with the current 
(average) state of testing infra, hard to do. Perhaps sometime later.

For now, I think this could already go a long way towards having more (and 
better) testing.

Nick

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Nikolai Kondrashov]

On 11/15/23 22:38, Konstantin Ryabitsev wrote:
> On Wed, Nov 15, 2023 at 07:43:49PM +0200, Nikolai Kondrashov wrote:
>> Introduce a new tag, 'Tested-with:', documented in the
>> Documentation/process/submitting-patches.rst file. The tag is expected
>> to reference the documented test suites, similarly to the 'V:' field,
>> and to certify that the submitter executed the test suite on the change,
>> and that it passed.
> 
> I'm not sure it should be a tag that stays all the way through git commit.
> How about making it a cover letter/first patch footer?
> 
> tested-with: <suite>

Yes, that would be better indeed. However, checkpatch.pl doesn't process cover 
letters, and so we would have no automated way to advertise and nudge people 
towards testing.

Nick

P.S. Git forges do that for you by nature of actually running the tests 
themselves, automatically. *Ducks*

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 1107 bytes --]

On Wed, Nov 15, 2023 at 07:43:49PM +0200, Nikolai Kondrashov wrote:
> Make scripts/checkpatch.pl ensure any added V: fields reference
> documented test suites only, and output a warning if a change to a
> subsystem doesn't certify the required test suites were executed,
> if any.
> 
> If the test suite description includes a "Command", then checkpatch.pl
> will output it as the one executing the suite. The command should run
> with only the kernel tree and the regular developer environment set up.
> But, at the same time, could simply output instructions for installing
> any extra dependencies (or pull some automatically). The idea is to
> get the developer into feedback loop quicker and easier, so they have
> something to run and iterate on, even if it involves installing some
> more stuff first. Therefore it's a good idea to add such wrappers to the
> kernel tree proper and refer to them from the tests.rst.

Does it also apply to trivial patches (e.g. spelling or checkpatch fixes
as seen on drivers/staging/)?

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1028 bytes --]

On Thu, Nov 16, 2023 at 02:14:24PM +0200, Nikolai Kondrashov wrote:

> Yes, that would be better indeed. However, checkpatch.pl doesn't process
> cover letters, and so we would have no automated way to advertise and nudge
> people towards testing.

Back when I used to run checkpatch it seemed to cope, it obviously
wouldn't find much to look at in there but you could feed it an entire
series with cover letter and the cover letter wouldn't cause any extra
issues.

> P.S. Git forges do that for you by nature of actually running the tests
> themselves, automatically. *Ducks*

The ability of forges to run tests is not hugely relevant to large
portions of the kernel, for drivers you're wanting the tests to run on
the relevant hardware and even core changes will often need hardware
that exercises the relevant features to run.  In these areas you're more
just looking for someone to say that they've done relevant testing but
there's not a substantial difference between say a comment on a pull
request or a followup email.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Nikolai Kondrashov]

On 11/16/23 15:20, Bagas Sanjaya wrote:
> On Wed, Nov 15, 2023 at 07:43:49PM +0200, Nikolai Kondrashov wrote:
>> Make scripts/checkpatch.pl ensure any added V: fields reference
>> documented test suites only, and output a warning if a change to a
>> subsystem doesn't certify the required test suites were executed,
>> if any.
>>
>> If the test suite description includes a "Command", then checkpatch.pl
>> will output it as the one executing the suite. The command should run
>> with only the kernel tree and the regular developer environment set up.
>> But, at the same time, could simply output instructions for installing
>> any extra dependencies (or pull some automatically). The idea is to
>> get the developer into feedback loop quicker and easier, so they have
>> something to run and iterate on, even if it involves installing some
>> more stuff first. Therefore it's a good idea to add such wrappers to the
>> kernel tree proper and refer to them from the tests.rst.
> 
> Does it also apply to trivial patches (e.g. spelling or checkpatch fixes
> as seen on drivers/staging/)?

Do you mean, will checkpatch.pl suggest executing test suites for trivial 
patches as well? If so, then yes, of course. These are inevitable victims of 
such mechanisms in general, and it's hard to make an exception for them, but 
we have to consider the overall benefit of having more uniform testing vs. 
making trivial changes a bit more difficult.

Nick

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Bagas Sanjaya]

On 11/16/23 20:41, Nikolai Kondrashov wrote:
> On 11/16/23 15:20, Bagas Sanjaya wrote:
>> On Wed, Nov 15, 2023 at 07:43:49PM +0200, Nikolai Kondrashov wrote:
>>> Make scripts/checkpatch.pl ensure any added V: fields reference
>>> documented test suites only, and output a warning if a change to a
>>> subsystem doesn't certify the required test suites were executed,
>>> if any.
>>>
>>> If the test suite description includes a "Command", then checkpatch.pl
>>> will output it as the one executing the suite. The command should run
>>> with only the kernel tree and the regular developer environment set up.
>>> But, at the same time, could simply output instructions for installing
>>> any extra dependencies (or pull some automatically). The idea is to
>>> get the developer into feedback loop quicker and easier, so they have
>>> something to run and iterate on, even if it involves installing some
>>> more stuff first. Therefore it's a good idea to add such wrappers to the
>>> kernel tree proper and refer to them from the tests.rst.
>>
>> Does it also apply to trivial patches (e.g. spelling or checkpatch fixes
>> as seen on drivers/staging/)?
> 
> Do you mean, will checkpatch.pl suggest executing test suites for
> trivial patches as well? If so, then yes, of course. These are
> inevitable victims of such mechanisms in general, and it's hard to make
> an exception for them, but we have to consider the overall benefit of
> having more uniform testing vs. making trivial changes a bit more
> difficult.
> 

Yes, that's what I mean. Thanks anyway.

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Nikolai Kondrashov]

On 11/16/23 15:26, Mark Brown wrote:
> On Thu, Nov 16, 2023 at 02:14:24PM +0200, Nikolai Kondrashov wrote:
> 
>> Yes, that would be better indeed. However, checkpatch.pl doesn't process
>> cover letters, and so we would have no automated way to advertise and nudge
>> people towards testing.
> 
> Back when I used to run checkpatch it seemed to cope, it obviously
> wouldn't find much to look at in there but you could feed it an entire
> series with cover letter and the cover letter wouldn't cause any extra
> issues.

Ah, good to know, thank you. The question now is whether we can expect, or 
require, submitters to run checkpatch.pl on the complete patchset, cover 
letter included, *before* sending it.

>> P.S. Git forges do that for you by nature of actually running the tests
>> themselves, automatically. *Ducks*
> 
> The ability of forges to run tests is not hugely relevant to large
> portions of the kernel, for drivers you're wanting the tests to run on
> the relevant hardware and even core changes will often need hardware
> that exercises the relevant features to run.  In these areas you're more
> just looking for someone to say that they've done relevant testing but
> there's not a substantial difference between say a comment on a pull
> request or a followup email.

Agreed.

Still, there *are* largely hardware-independent (and thus maybe more 
impactful) parts of the kernel too.

Plus, you know yourself there's a bunch of companies willing (if not outright 
clamoring) to contribute their machine time for testing, provided some good 
will and authentication on the part of the contributors. And git forges 
provide good support for the latter. So perhaps *some* special hardware 
*could* be arranged there too, making it more useful.

Nick

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Greg Kroah-Hartman]

On Thu, Nov 16, 2023 at 08:20:18PM +0700, Bagas Sanjaya wrote:
> On Wed, Nov 15, 2023 at 07:43:49PM +0200, Nikolai Kondrashov wrote:
> > Make scripts/checkpatch.pl ensure any added V: fields reference
> > documented test suites only, and output a warning if a change to a
> > subsystem doesn't certify the required test suites were executed,
> > if any.
> > 
> > If the test suite description includes a "Command", then checkpatch.pl
> > will output it as the one executing the suite. The command should run
> > with only the kernel tree and the regular developer environment set up.
> > But, at the same time, could simply output instructions for installing
> > any extra dependencies (or pull some automatically). The idea is to
> > get the developer into feedback loop quicker and easier, so they have
> > something to run and iterate on, even if it involves installing some
> > more stuff first. Therefore it's a good idea to add such wrappers to the
> > kernel tree proper and refer to them from the tests.rst.
> 
> Does it also apply to trivial patches (e.g. spelling or checkpatch fixes
> as seen on drivers/staging/)?

You are assuming that drivers/staging/ has actual tests :)

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Geert Uytterhoeven]

Hi Bagas,

On Thu, Nov 16, 2023 at 2:20 PM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
> On Wed, Nov 15, 2023 at 07:43:49PM +0200, Nikolai Kondrashov wrote:
> > Make scripts/checkpatch.pl ensure any added V: fields reference
> > documented test suites only, and output a warning if a change to a
> > subsystem doesn't certify the required test suites were executed,
> > if any.
> >
> > If the test suite description includes a "Command", then checkpatch.pl
> > will output it as the one executing the suite. The command should run
> > with only the kernel tree and the regular developer environment set up.
> > But, at the same time, could simply output instructions for installing
> > any extra dependencies (or pull some automatically). The idea is to
> > get the developer into feedback loop quicker and easier, so they have
> > something to run and iterate on, even if it involves installing some
> > more stuff first. Therefore it's a good idea to add such wrappers to the
> > kernel tree proper and refer to them from the tests.rst.
>
> Does it also apply to trivial patches (e.g. spelling or checkpatch fixes
> as seen on drivers/staging/)?

After having seen the introduction of too many build failures, I'm
inclined to ask for an even stronger proof of testing for "trivial"
fixes for drivers/staging...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/3] MAINTAINERS: Require kvm-xfstests smoke for ext4

[Nikolai Kondrashov]

Thanks for the comments, Darrick!

On 11/15/23 20:58, Darrick J. Wong wrote:
> On Wed, Nov 15, 2023 at 07:43:50PM +0200, Nikolai Kondrashov wrote:
>> +xfstests
>> +--------
>> +
>> +:Summary: File system regression test suite
>> +:Source: git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git
> 
> You might as well use the https link to the fstests git repo.
> https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git

Sure! Queued for the next version.

>> +:Docs: https://github.com/tytso/xfstests-bld/blob/master/Documentation/what-is-xfstests.md
> 
> Awkardly, this github link is nice for rendering the markdown as html,
> but I think the canonical source of xfstests-bld is also kernel.org:
> 
> https://git.kernel.org/pub/scm/fs/ext2/xfstests-bld.git

Alright. Changed to 
https://git.kernel.org/pub/scm/fs/ext2/xfstests-bld.git/tree/Documentation/kvm-quickstart.md

And changed the kvm-xfstests docs link to 
https://git.kernel.org/pub/scm/fs/ext2/xfstests-bld.git/tree/Documentation/kvm-quickstart.md

>> +kvm-xfstests smoke
>> +------------------
>> +
>> +:Summary: File system smoke tests
>> +:Superset: xfstests
> 
> Source: https://git.kernel.org/pub/scm/fs/ext2/xfstests-bld.git
> 
> ?

Well, I wasn't sure what to put here either :D I would defer to you guys in 
this matter.

I'm actually not really sure we need the "Source:" field. I think maybe having 
just the "Docs" (HOWTO) field would less confusing. I.e. just go read the 
docs, they should tell you what and how to get.

I mean you got the sources, and then what? Look for the docs there yourself?

What do you think?

>> +:Docs: https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
>> +
>> +The "kvm-xfstests smoke" is a minimal subset of xfstests for testing all major
>> +file systems, running under KVM.
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 2565c04f0490e..f81a47d87ac26 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7974,6 +7974,7 @@ L:	linux-ext4@vger.kernel.org
>>   S:	Maintained
>>   W:	http://ext4.wiki.kernel.org
>>   Q:	http://patchwork.ozlabs.org/project/linux-ext4/list/
>> +V:	kvm-xfstests smoke
> 
> I wouldn't mind one of these being added to the XFS entry, though I've
> cc'd the current and past maintainer(s) of XFS for their input.

Sure, just give me a shout when you're ready and I'll add it :D

Thanks!
Nick

Re: [PATCH 2/3] MAINTAINERS: Require kvm-xfstests smoke for ext4

[Chandan Babu R]

On Wed, Nov 15, 2023 at 10:58:08 AM -0800, Darrick J. Wong wrote:
> On Wed, Nov 15, 2023 at 07:43:50PM +0200, Nikolai Kondrashov wrote:
>> Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
>> ---
>>  Documentation/process/tests.rst | 32 ++++++++++++++++++++++++++++++++
>>  MAINTAINERS                     |  1 +
>>  2 files changed, 33 insertions(+)
>> 
>> diff --git a/Documentation/process/tests.rst b/Documentation/process/tests.rst
>> index 907311e91ec45..9a9ea3fe65c37 100644
>> --- a/Documentation/process/tests.rst
>> +++ b/Documentation/process/tests.rst
>> @@ -33,3 +33,35 @@ particularly useful:
>>  
>>  :Source: A URL pointing to the source code of the test suite
>>  :Docs: A URL pointing to further test suite documentation
>> +
>> +xfstests
>> +--------
>> +
>> +:Summary: File system regression test suite
>> +:Source: git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git
>
> You might as well use the https link to the fstests git repo.
> https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git
>
>> +:Docs: https://github.com/tytso/xfstests-bld/blob/master/Documentation/what-is-xfstests.md
>
> Awkardly, this github link is nice for rendering the markdown as html,
> but I think the canonical source of xfstests-bld is also kernel.org:
>
> https://git.kernel.org/pub/scm/fs/ext2/xfstests-bld.git
>
>> +
>> +As the name might imply, xfstests is a file system regression test suite which
>> +was originally developed by Silicon Graphics (SGI) for the XFS file system.
>> +Originally, xfstests, like XFS was only supported on the SGI's Irix operating
>> +system. When XFS was ported to Linux, so was xfstests, and now xfstests is
>> +only supported on Linux.
>> +
>> +Today, xfstests is used as a file system regression test suite for all of
>> +Linux's major file systems: xfs, ext2, ext4, cifs, btrfs, f2fs, reiserfs, gfs,
>> +jfs, udf, nfs, and tmpfs. Many file system maintainers will run a full set of
>> +xfstests before sending patches to Linus, and will require that any major
>> +changes be tested using xfstests before they are submitted for integration.
>> +
>> +The easiest way to start running xfstests is under KVM with xfstests-bld:
>> +https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
>> +
>> +kvm-xfstests smoke
>> +------------------
>> +
>> +:Summary: File system smoke tests
>> +:Superset: xfstests
>
> Source: https://git.kernel.org/pub/scm/fs/ext2/xfstests-bld.git
>
> ?
>
>> +:Docs: https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
>> +
>> +The "kvm-xfstests smoke" is a minimal subset of xfstests for testing all major
>> +file systems, running under KVM.
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 2565c04f0490e..f81a47d87ac26 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7974,6 +7974,7 @@ L:	linux-ext4@vger.kernel.org
>>  S:	Maintained
>>  W:	http://ext4.wiki.kernel.org
>>  Q:	http://patchwork.ozlabs.org/project/linux-ext4/list/
>> +V:	kvm-xfstests smoke
>
> I wouldn't mind one of these being added to the XFS entry, though I've
> cc'd the current and past maintainer(s) of XFS for their input.
>
> --D
>

IMHO, For XFS, The value of "V" field should refer to xfstests rather than a
framework built around xfstests. This is because xfstests project contains the
actual tests and also we could have several frameworks (e.g. Kdevops) for
running xfstests.

I think "kvm-xfstests smoke" could be mentioned in
Documentation/process/tests.rst as one of the easier methods to execute
xfstests.

Also, We could add a statement in Documentation/process/tests.rst encouraging
the patch author to look into xfstests/tests/[generic|xfs]/group.list files to
pick and execute test groups which are applicable to areas of XFS
(e.g. realtime) being modified.


>>  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git
>>  F:	Documentation/filesystems/ext4/
>>  F:	fs/ext4/
>> -- 
>> 2.42.0
>> 

-- 
Chandan

Re: [PATCH 2/3] MAINTAINERS: Require kvm-xfstests smoke for ext4

[Theodore Ts'o]

On Fri, Nov 17, 2023 at 12:39:56PM +0530, Chandan Babu R wrote:
> IMHO, For XFS, The value of "V" field should refer to xfstests rather than a
> framework built around xfstests. This is because xfstests project contains the
> actual tests and also we could have several frameworks (e.g. Kdevops) for
> running xfstests.

For ext4, what I plan to do is to start requiring, in a soon-to-be
written:

	Documentation/process/maintainer-ext4.rst

that *all* patches (exempting spelling/grammer nits which only touch
comments and result in zero changes in the compiled .o file) will
require running the xfstests smoke test.  The prooblem is that for
newbie patch submitters, and for "drive-by" patches from, say, mm
developers, installing and configuring xfstests, and then figuring out
how to set up all of the config files, and/or environments variables,
before you get to running "./check -g smoke"P, is really f**king hard.

As far as other test frameworks are concerned, I suggest that you ask
a new college grad that your company has just hired, or a new intern,
or a new GSOC or Outreachy intern, to apply a patch to the upstream
linux tree --- given only the framework's documentation, and with
***no*** help from anyone who knows how to use the framework.  Just
sit on your hands, and try as best you can to keep your mouth
shut.... and wait.

I spent a lot of work to make the instructures here:

  https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md

easy enough that it meets this critera.  What should be in the V:
field for the MAINTAINERS field is less clear to me, because it's not
clear whether we want it to be Dead Stupid Easy for anyone capable of
creating a kernel patch can figure out how to run the tests.

My personal opinion is that for someone who running through the Kernel
Newbies' My First Kernel patch, to say, doing something trivial such
as changing "(a < b) ? a : b" to "min(a,b)" and then being able
compile kernel, and then be able to say, "it builds, ship it", and
then following the kernel documentation to send a patch upstream ---
the documentation and procedure necessary to do something better than
"it builds, ship it!" by that Kernel Newbie.

It's also my considered opinion that neither bare, upstream xfstests,
*nor* kdevops meets this criteria.  For example, kdevops may claim
that you only need a few commands:

     "make menuconfig"
     "make"
     "make bringup"
     "make linux"
     "make fstests"
     "make fstests-baseline"
     "make fstests-results"

... but to be honest, I never got past the 2nd step before *I* got
massively confused and decided there was better things to do with my
time.  First, the "make menuconfig" requires that you answer a whole
bunch of questions, and it's not clear how you are supposed to answer
them all.  Secondly "make" issues a huge number of cmomands, and then
fails because I didn't run them as root.  But I won't download random
git repositories and "make" as root.  It goes against the grain.  And
so after trying to figure out what it did, and whether or not it was
safe, I ultimetly gave up on it.

For upstream xfstests, ask a New College Grad fresh hire, or intern,
to read xfstests's README file, and ask them to set up xfstests,
without help.  Go ahead, I'll wait....

No doubt for someone who is willing to make a huge investment in time
to become a file system developer specializing in that subsystem, you
will eventually be able to figure it out.  And in the case of kdevops,
eventually I'd could set up my own VM, and install a kernel compile
toolchian, and all of kdevops's prequisits, and then run "make" and
"make linux" in that VM.  But that's a lot more than just "four
commands".

So as for *me*, I'm going to point people at:

https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md

because I've simplified a lot of things, up to and including have
kvm-xfstests automatically download the test appliance VM image from
kernel.org if necessary.  When it lists the handful of commands that
need to be run, it includes downloading all of the prequisit packages,
and there are no complex menu configuration steps, and doesn't require
running random make processes of Makefile and Makefile fragments as
root.

(And note that I keep the xfstests-bld repo's on kernel.org and
github.com both uptodate, and I prefer using the using the github.com
URL because it's easier for the new developer to read and understand
it.)

Ultimately, at least for my planned
Documentation/process/maintainer-ext4.rst, before I could make running
a smoke test mandatory, I needed to make sure that the quickstart
documentation for kvm-xfstests be made as simple and as fool-proof as
possible.  That was extremely important to me from a personal point
of view.

As far as what should go into Documentation/process/tests.rst, for
"kvm-xfstests smoke" this may depend on whether other file system
maintainers want to adopt something which is similarly simple for
first-time developers to run.

Also, I would assert that the proposed V: line in the Maintainer's
file does not mean that this is the only way to test the patch.  It is
just the minimal amount of testing that should be done, and using the
simplest test procedure that we would expect a non-subsystem developer
to be able to use.  It certainly wouldn't be the only way to run a
satisfactory amount of pre-submit testing.

For example, *I* wouldn't be using "kvm-xfstests smoke".  I'd be using
something like "gce-xfstests smoke", although that requires a bit more
setup.  Or I might do a much more substantive amount of testing, such
as "gce-xfstests ltm -c ext4/all -g auto".

Or I might establish a watch on a branch, via: "gce-xfstests ltm -c
ext4/all -g auto --repo https://github.com/tytso/ext4.git --watch
test", and then just push commits to the test branch.

And similarly, just because the V: line might say, "kvm-xfstests
smoke", someone could certainly use kdevops if they wanted to.  So
perhaps we need to be a bit clearer about what we expect the V: line
to mean?

Along these lines, we should perhaps be a bit more thoughtful about
the intended audience for Documentation/process/tests.rst.  I
personally wouldn't try ask first-time kernel developers to look at
the xfstests group files, because that's just way too complicated for
them.

And I had *assumed* that Documentation/process/tests.rst was not
primarily intended for sophistiocated file system developers who
wouldn't be afraid to start looking at the many ways that xfstests
could be configured.  But we should perhaps be a bit more explicit
about who the intended audience would be for a certain set up
Documentation files, since that will make it easier for us to come to
consensus about how that particular piece of documentation would be
worded.

As E.B. White (author of the book "The Elements of Style" was reputed
to have once said, "Always write with deep sympathy for the reader".
Which means we need to understand who the reader is, and to try to
walk in their shoes, first.

Cheers,

					- Ted

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Gustavo Padovan]

On Thursday, November 16, 2023 09:14 -03, Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com> wrote:

> On 11/15/23 22:38, Konstantin Ryabitsev wrote:
> > On Wed, Nov 15, 2023 at 07:43:49PM +0200, Nikolai Kondrashov wrote:
> >> Introduce a new tag, 'Tested-with:', documented in the
> >> Documentation/process/submitting-patches.rst file. The tag is expected
> >> to reference the documented test suites, similarly to the 'V:' field,
> >> and to certify that the submitter executed the test suite on the change,
> >> and that it passed.
> > 
> > I'm not sure it should be a tag that stays all the way through git commit.
> > How about making it a cover letter/first patch footer?
> > 
> > tested-with: <suite>
> 
> Yes, that would be better indeed. However, checkpatch.pl doesn't process cover 
> letters, and so we would have no automated way to advertise and nudge people 
> towards testing.
 
At this year's LPC, there was quite some discussion around improving testing information, so this patch of yours lands has a great timing. :)

All your are proposing here is pretty interesting both for developers and CI systems, but it seems that a "Tested-with:" tag and checkpatch.pl validation will take quite some time to consolidate.

Would it make sense to split just the part that adds the V: field to MAINTAINERS and submit that as separate patchset together with its documentation? That way we can enable subsystem to start annotating their test suites already.

I also wonder how to make for subsystems that will have different test suites (eg something in kselftests and an external test suite). Would an alternative be pointing to a Documentation page with detailed info?

- Gus

-- 
Gustavo Padovan
Kernel Lead
Collabora Ltd.

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Ricardo Cañuelo]

Hi Nikolai,

On mié, nov 15 2023 at 19:43:49, Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com> wrote:
> Introduce a new tag, 'Tested-with:', documented in the
> Documentation/process/submitting-patches.rst file. The tag is expected
> to reference the documented test suites, similarly to the 'V:' field,
> and to certify that the submitter executed the test suite on the change,
> and that it passed.

I think the 'V:' field in MAINTAINERS is a good addition to document
what developers are supposed to test for every subsystem, but in the
case of the per-commit "Tested-with:" tag, I think the real value of it
would be in using it for accountability and traceability purposes
instead, that is, to link to the actual results of the (automatic) tests
that were used to validate a commit.

This would provide two important features:

1. Rather than trusting that the tester did things right and that the
   test environment used was appropriate, we'd now have proof that the
   test results are as expected and a way to reproduce the steps.

2. A history of test results for future reference. When a regression is
   introduced, now we'd have more information about how things worked
   back when the test was still passing.

This is not trivial because tests vary a lot and we'd first need to
define which artifacts to link to, and because whatever is linked (test
commands, output log, results summary) would need to be stored
forever. But since we're doing that already for basically all kernel
mailing lists, I wonder if something like "public-inbox for test
results" could be possible some day.

Cheers,
Ricardo

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 573 bytes --]

On Mon, Nov 20, 2023 at 12:40:39PM +0000, Gustavo Padovan wrote:

> I also wonder how to make for subsystems that will have different test
> suites (eg something in kselftests and an external test suite). Would
> an alternative be pointing to a Documentation page with detailed info?

Why not just add multiple test suite lines like we do for other things
where there can be more than one example?

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] MAINTAINERS: Require kunit core tests for framework changes

[Daniel Latypov]

On Wed, Nov 15, 2023 at 9:52 AM Nikolai Kondrashov
<Nikolai.Kondrashov@redhat.com> wrote:
> +kunit core
> +----------
> +
> +:Summary: KUnit tests for the framework itself
> +:Superset: kunit
> +:Command: tools/testing/kunit/kunit.py run --kunitconfig lib/kunit

Note: we'd want this to instead be
  ./tools/testing/kunit/run_checks.py

That will run, in parallel
* kunit.py run --kunitconfig lib/kunit
* kunit_tool_test.py (the unit test for kunit.py)
* two python type-checkers, if installed

> diff --git a/MAINTAINERS b/MAINTAINERS
> index f81a47d87ac26..5f3261e96c90f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11626,6 +11626,7 @@ L:      linux-kselftest@vger.kernel.org
>  L:     kunit-dev@googlegroups.com
>  S:     Maintained
>  W:     https://google.github.io/kunit-docs/third_party/kernel/docs/
> +V:     kunit core

Maybe this topic should go in the main thread, but I wanted to call it
out here since this is a good concrete example.

I'm wondering if this entry could simply be
  V: ./tools/testing/kunit/run_checks.py

And what if, for ext4, we could have multiple of these like
  V: kvm-xfstests smoke
  V: tools/testing/kunit/kunit.py run --kunitconfig fs/ext4

I.e. what if we allowed the `V:` field to contain the command itself?

# Complexity of the tests

I appreciate the current "named test-set" approach since it encourages
documenting *why* the test command is applicable.
And for a lot of tests, it's not as simple as a binary GOOD/BAD
result, e.g. benchmarks.
Patch authors need to understand what they're testing, if they're
testing the right thing, etc.

But on the other hand, for simpler functional tests (e.g. KUnit),
maybe it's not necessary.
Ideally, KUnit tests should be written so the failure messages are
immediately actionable w/o having to read a couple paragraphs.
I.e. the test case names should make it clear what scenario they're
testing, or the test code should be sufficiently documented, etc.

# Variations on commands

And there might be a bunch of slight variations on these commands,
e.g. only different in terms of `--kunitconfig`.
We'd probably have at least 18, given
$ find -type f -name '.kunitconfig' | wc -l
18

And again using a kunit.py flag as an example, what if maintainers want KASAN?
  ./tools/testing/kunit/kunit.py run --kunitconfig lib/kunit
--kconfig_add CONFIG_KASAN=y
Or what if it's too annoying to split up a larger kunit suite, so I
ask people to run a subset?
  ./tools/testing/kunit/kunit.py run --kunitconfig lib/kunit <suite_glob>


P.S.
Tbh, I have always hoped we'd eventually have a field like
  V: <one-liner test command>

That is part of why I added all those features above (--kunitconfig,
--kconfig_add, glob support, run_checks.py, etc.).
I wanted kunit.py to be flexible enough that maintainers could state
their testing requirements as a one-liner that people can just
copy-paste and run.

Also, I recently talked to David Gow and I know he was interested in
adding another feature to kunit.py to fit this use case.
Namely, the ability to do something like
  kunit.py run --arches=x86_64,s390,...
and have it run the tests built across N different arches and maybe w/
M different kconfig options (e.g. a variation built w/ clang, etc.).

That would be another very powerful tool for maintainers to have.

Thanks so much for this patch series and starting this discussion!
Daniel

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Theodore Ts'o]

On Mon, Nov 20, 2023 at 02:30:49PM +0100, Ricardo Cañuelo wrote:
> 
> This is not trivial because tests vary a lot and we'd first need to
> define which artifacts to link to, and because whatever is linked (test
> commands, output log, results summary) would need to be stored
> forever. But since we're doing that already for basically all kernel
> mailing lists, I wonder if something like "public-inbox for test
> results" could be possible some day.

What we have at work is a way to upload the test results summary
(e.g., just KTAP result lines, or the xfstests junit XML) along with
test run metadata (e.g., what was the kernel commit on which the test
was run, and the test hardware), and this would be stored permanently.
Test artifacts is also preserved but for a limited amount of time
(e.g., some number of months or a year).

The difference in storage lifetimes is because the junit XML file
might be a few kilobytes to tens of kilobytes. but the test artifacts
might be a few megabytes to tens of megabytes.

Of course once you have this data, it becomes possible to detect when
a test may have regressed, or to detect flaky tests, and perhaps to
figure out if certain hardware configurations or kernel configurations
are more likely to trigger a particular test to fail.  So having all
of this data stored centrally would be really cool.  The only question
is who might be able to create such an infrastructure, and be able to
pay for the ongoing development and operational costs....

    	    	    		    		- Ted

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2467 bytes --]

On Mon, Nov 20, 2023 at 03:51:31PM -0500, Theodore Ts'o wrote:

> What we have at work is a way to upload the test results summary
> (e.g., just KTAP result lines, or the xfstests junit XML) along with
> test run metadata (e.g., what was the kernel commit on which the test
> was run, and the test hardware), and this would be stored permanently.
> Test artifacts is also preserved but for a limited amount of time
> (e.g., some number of months or a year).

> The difference in storage lifetimes is because the junit XML file
> might be a few kilobytes to tens of kilobytes. but the test artifacts
> might be a few megabytes to tens of megabytes.

This is the sort of thing that kcidb (which Nikolai works on) is good at
ingesting, I actually do push all my CI's test results into there
already:

   https://github.com/kernelci/kcidb/

(the dashboard is down currently.)  A few other projects including the
current KernelCI and RedHat's CKI push their data in there too, I'm sure
Nikolai would be delighted to get more people pushing data in.  The goal
is to merge this with the main KernelCI infrastructure, it's currently
separate while people figure out the whole big data thing.

> Of course once you have this data, it becomes possible to detect when
> a test may have regressed, or to detect flaky tests, and perhaps to
> figure out if certain hardware configurations or kernel configurations
> are more likely to trigger a particular test to fail.  So having all
> of this data stored centrally would be really cool.  The only question
> is who might be able to create such an infrastructure, and be able to
> pay for the ongoing development and operational costs....

The KernelCI LF project is funding kcidb with precisely this goal for
the reasons you outline, the data collection part seems to be relatively
mature at this point but AIUI there's a bunch of open questions with the
analysis and usage side, partly due to needing to find people to work on
it.  My understanding is that ingesting large data sets into cloud
providers is pretty tractable, as with a lot of this stuff it gets more
interesting trying to pull the data out and comprehend it in a practical
fashion.  It'd be really cool to see more people working on that side of
things.

On the submission side it'd be interesting to start collecting more data
about the test systems used to run things, might be useful to add a new
schema for that which can be referenced from the test schema.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Theodore Ts'o]

On Mon, Nov 20, 2023 at 10:27:33PM +0000, Mark Brown wrote:
> This is the sort of thing that kcidb (which Nikolai works on) is good at
> ingesting, I actually do push all my CI's test results into there
> already:
> 
>    https://github.com/kernelci/kcidb/
> 
> (the dashboard is down currently.)  A few other projects including the
> current KernelCI and RedHat's CKI push their data in there too, I'm sure
> Nikolai would be delighted to get more people pushing data in.  The goal
> is to merge this with the main KernelCI infrastructure, it's currently
> separate while people figure out the whole big data thing.

Looking at the kernelci, it appears that it's using a JSON submission
format.  Is there conversion scripts that take a KTAP test report, or
a Junit XML test report?

> The KernelCI LF project is funding kcidb with precisely this goal for
> the reasons you outline, the data collection part seems to be relatively
> mature at this point but AIUI there's a bunch of open questions with the
> analysis and usage side, partly due to needing to find people to work on
> it.

Indeed, this is the super hard part.  Having looked at the kernelci
web site, its dashboard isn't particularly useful for what I'm trying
to do with it.  For my part, when analyizing a single test run, the
kernelci dashboard isn't particularly helpful.  What I need is
something more like this:

ext4/4k: 554 tests, 48 skipped, 4301 seconds
ext4/1k: 550 tests, 3 failures, 51 skipped, 6739 seconds
  Failures: generic/051 generic/475 generic/476
ext4/ext3: 546 tests, 138 skipped, 4239 seconds
ext4/encrypt: 532 tests, 3 failures, 159 skipped, 3218 seconds
  Failures: generic/681 generic/682 generic/691
ext4/nojournal: 549 tests, 3 failures, 118 skipped, 4477 seconds
  Failures: ext4/301 ext4/304 generic/455
ext4/ext3conv: 551 tests, 49 skipped, 4655 seconds
ext4/adv: 551 tests, 4 failures, 56 skipped, 4987 seconds
  Failures: generic/477 generic/506
  Flaky: generic/269: 40% (2/5)   generic/455: 40% (2/5)
ext4/dioread_nolock: 552 tests, 48 skipped, 4538 seconds
ext4/data_journal: 550 tests, 2 failures, 120 skipped, 4401 seconds
  Failures: generic/455 generic/484
ext4/bigalloc_4k: 526 tests, 53 skipped, 4537 seconds
ext4/bigalloc_1k: 526 tests, 61 skipped, 4847 seconds
ext4/dax: 541 tests, 1 failures, 152 skipped, 3069 seconds
  Flaky: generic/269: 60% (3/5)
Totals: 6592 tests, 1053 skipped, 72 failures, 0 errors, 50577s

... which summarizes 6,592 tests in 20 lines, and for any test that
has failed, we rerun it four more times, so we can get an indication
of whether a test is a hard failure, or a flaky failure.

(I don't need to see all of the tests that passes; it's the test
failures or the test flakes that are significant.)

And then when comparing between multiple test runs, that's when I'm
interesting in see which tests may have regressed, or which tests may
have been fixed when going in between version A and version B.

And right now, kernelci doesn't have any of that.  So it might be hard
to convinced overloaded maintainers to upload test runs to kernelci,
when they don't see any immediate benefit of uploading the kernelci db.

There is a bit of a chicken-and-egg problem, since without the test
results getting uploaded, it's hard to get the analysis functionality
implemented, and without the analysis features, it's hard to get
developers to upload the data.

That being said, a number of file system developers probably have
several years worth of test results that we could probably give you.
I have hundreds of junit.xml files, with information about how kernel
version, what version of xfstesets, etc, that was used.  I'm happy to
make samples of it available for anyone who is interested.

Cheers,

						- Ted

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 6604 bytes --]

Thanks so much for doing this! I think everyone agrees that we need
_some_ way of documenting which tests to run, and I think this is our
best option.

In any case, this patch does a lot, and I'll comment on them
one-by-one. (It may be worth splitting this patch up into a few
separate bits, if only so that we can better separate the
uncontroversial bits from the open questions.)

On Thu, 16 Nov 2023 at 01:52, Nikolai Kondrashov
<Nikolai.Kondrashov@redhat.com> wrote:
>
> Introduce a new 'V:' ("Verify") field to MAINTAINERS. The field accepts
> a name of a test suite which is required to be executed for each
> contribution to the subsystem.

Yes -- this is exactly what I'd like. (As much as I'd love 'T' to have
been available. Alas...)

The other thing discussed at plumbers was to include this in the
'maintainer profile', but having it as a separate MAINTAINERS entry is
my preference, and is better for automation.

The question for what the tag actually contains brings us to...
>
> Each referenced test suite is expected to be documented in the new
> Documentation/process/tests.rst file, which must have enough structure
> (documented inside) for the tools to make use of it. Apart from basic
> data, each test can refer to its "superset" - a test suite which this
> one is a part of. The expected use is to describe both a large test
> suite and its subsets, so the former would also be accepted, if a
> subsystem requires only a subset.

I think this could work, but is a bit complicated.

My initial thought was to have this as a more free-form field, which
either contained a:
- Path to a command to run (e.g. tools/testing/kunit/run_checks.py)
- Path to a documentation file describing the test.
- URL to a page describing the test
- (Maybe) freeform text.

It's probably worth also looking at this proposal to auto-generate
similar documentation:
https://lore.kernel.org/linux-kselftest/cover.1689171160.git.mchehab@kernel.org/

The other question is how to handle outdated results when a new patch
revision is sent out. Personally, I think this is something we can
solve similarly to 'Reviewed-by', depending on the extent of the
changes and cost of the tests. I suspect for most automated tests,
this would mean never carrying the 'Tested-with' tag over, but if
testing it involved manually building and running kernels against 50
different hardware setups, I could imagine it making sense to not
re-do this if a new revision just changed a doc typo. If a URL is used
here, it could contain version info, too.

>
> Introduce a new tag, 'Tested-with:', documented in the
> Documentation/process/submitting-patches.rst file. The tag is expected
> to reference the documented test suites, similarly to the 'V:' field,
> and to certify that the submitter executed the test suite on the change,
> and that it passed.

I'm also 100% for this, though I'd considered it separately from the
MAINTAINERS change.

I think, in the ideal case, we want this to link to the results
somehow. kcidb would seem to be the obvious choice there.

Again, as a fallback, a plain text field would be useful to describe
cases where a patch was tested by some means other than a formal test
suite. This might not be ideal, but I'd still rather have people
describe that something "builds and boots on <x> hardware" than have
to guess if a patch was tested at all.

Of course, it'd then be up to maintainers to decide what they'd
accept: I'd expect that some would require there be a 'Tested-with'
header which links to valid results for the tests described in
MAINTAINERS.

>
> Make scripts/checkpatch.pl ensure any added V: fields reference
> documented test suites only, and output a warning if a change to a
> subsystem doesn't certify the required test suites were executed,
> if any.
>

I'd definitely want something like this to run at some point in the
patch-submission workflow. I think that, ultimately, we'll want to be
able to run some tests automatically (e.g., a git hook could run the
tests and add the 'Tested-with' line).

Personally, I'd like to require that all patches have a 'Tested-with'
field, even if there's not a corresponding 'V' MAINTAINERS entry, as
people should at least think of how something's tested, even if
there's not a formal 'test suite' for it. Though that seems a
longer-term goal


> If the test suite description includes a "Command", then checkpatch.pl
> will output it as the one executing the suite. The command should run
> with only the kernel tree and the regular developer environment set up.
> But, at the same time, could simply output instructions for installing
> any extra dependencies (or pull some automatically). The idea is to
> get the developer into feedback loop quicker and easier, so they have
> something to run and iterate on, even if it involves installing some
> more stuff first. Therefore it's a good idea to add such wrappers to the
> kernel tree proper and refer to them from the tests.rst.
>
> Extend scripts/get_maintainer.pl to support retrieving the V: fields,
> and scripts/parse-maintainers.pl to maintain their ordering.
>
> Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
> ---

The questions I think we need to answer to get this in are:
1. Do we want to split this up (and potentially land it
piece-by-piece), or is it more valuable to have a stricter, more
complete system from the get-go?
2. What format should the 'V' line take? If it is simply a test name,
do we use a doc as suggested (or one generated in part from some other
process), or something like a command name or URL? Can it just be
freeform text?
3. Should 'Tested-with' be a test name in the same format as 'V', a
URL to results (any URL, or just kcidb?), or freeform text? How does
this evolve with multiple versions of patches?
4. How should this be enforced? A warning (not an 'error') from
checkpatch? A separate script?

Personally, my gut feeling is that we should land the simplest, most
minimal version of this (the 'V' field, as freeform text) now, and
build on that as consensus and tooling permits. I'd probably also add
the 'Tested-with' or similar tag, as freeform text, too. I don't think
either of those would cause major problems if we needed to change or
restrict the format later; I imagine there won't be a huge need to
parse old commits for test data, and even if so, it wouldn't be too
hard to ignore any which don't conform to any stricter future
convention.

But I don't think there's anything fundamentally wrong with the full
plan as-is, so if everyone's happy with it, I'd not object to having
it.

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 1246 bytes --]

On Tue, 21 Nov 2023 at 14:05, Theodore Ts'o <tytso@mit.edu> wrote:
>
> On Mon, Nov 20, 2023 at 10:27:33PM +0000, Mark Brown wrote:
> > This is the sort of thing that kcidb (which Nikolai works on) is good at
> > ingesting, I actually do push all my CI's test results into there
> > already:
> >
> >    https://github.com/kernelci/kcidb/
> >
> > (the dashboard is down currently.)  A few other projects including the
> > current KernelCI and RedHat's CKI push their data in there too, I'm sure
> > Nikolai would be delighted to get more people pushing data in.  The goal
> > is to merge this with the main KernelCI infrastructure, it's currently
> > separate while people figure out the whole big data thing.
>
> Looking at the kernelci, it appears that it's using a JSON submission
> format.  Is there conversion scripts that take a KTAP test report, or
> a Junit XML test report?

The kunit.py script has a very basic KCIDB JSON exporter, via the
--json option. This can be used as a generic KTAP -> KCIDB converter
with
kunit.py parse --json

It definitely still needs some work (there are a bunch of bugs,
hardcoded fields for things KTAP doesn't expose, some other output may
get mixed in, etc), but does exist as a starting point.

-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 5138 bytes --]

On Tue, Nov 21, 2023 at 01:04:50AM -0500, Theodore Ts'o wrote:
> On Mon, Nov 20, 2023 at 10:27:33PM +0000, Mark Brown wrote:

> > This is the sort of thing that kcidb (which Nikolai works on) is good at
> > ingesting, I actually do push all my CI's test results into there
> > already:

> >    https://github.com/kernelci/kcidb/

> > (the dashboard is down currently.)  A few other projects including the
> > current KernelCI and RedHat's CKI push their data in there too, I'm sure
> > Nikolai would be delighted to get more people pushing data in.  The goal
> > is to merge this with the main KernelCI infrastructure, it's currently
> > separate while people figure out the whole big data thing.

> Looking at the kernelci, it appears that it's using a JSON submission
> format.  Is there conversion scripts that take a KTAP test report, or
> a Junit XML test report?

Probably - I know I've got something for KUnit which is annoyingly
difficult to publish for non-technical reasons and is a little broken
(things weren't visible in the dashboard when it was up which might mean
some missing field or a date set wrong).  My KTAP stuff is all mediated
through LAVA, that can push results into a web hook directly so it's
really easy to just add a notifier to your job and stream the results in
directly (I intend to push that into kcidb in my copious free time so
other people can use my code there).  It's relatively straightforward to
write these things.

> > The KernelCI LF project is funding kcidb with precisely this goal for
> > the reasons you outline, the data collection part seems to be relatively
> > mature at this point but AIUI there's a bunch of open questions with the
> > analysis and usage side, partly due to needing to find people to work on
> > it.

> Indeed, this is the super hard part.  Having looked at the kernelci
> web site, its dashboard isn't particularly useful for what I'm trying
> to do with it.  For my part, when analyizing a single test run, the
> kernelci dashboard isn't particularly helpful.  What I need is
> something more like this:
> 
> ext4/4k: 554 tests, 48 skipped, 4301 seconds
> ext4/1k: 550 tests, 3 failures, 51 skipped, 6739 seconds
>   Failures: generic/051 generic/475 generic/476

That should be achievable with the KernelCI stuff (which is different to
kcidb at present) - you're a lot of the way there with how kselftest is
currently reported modulo the list of failures which currently requires
you to drill down to a second level page.

> ... which summarizes 6,592 tests in 20 lines, and for any test that
> has failed, we rerun it four more times, so we can get an indication
> of whether a test is a hard failure, or a flaky failure.

> (I don't need to see all of the tests that passes; it's the test
> failures or the test flakes that are significant.)

The listing of tests does get a bit more complex when you mix in running
on different platforms.

> And then when comparing between multiple test runs, that's when I'm
> interesting in see which tests may have regressed, or which tests may
> have been fixed when going in between version A and version B.

Yup, that comparison stuff is useful.  The landing pages for individual
tests do have something there but not really anything higher level:

   https://linux.kernelci.org/test/case/id/655b0fa18dc4b7e0c47e4a88/

> And right now, kernelci doesn't have any of that.  So it might be hard
> to convinced overloaded maintainers to upload test runs to kernelci,
> when they don't see any immediate benefit of uploading the kernelci db.

Note that kcidb and KernelCI are currently different databases - with
the dashboard being done kcidb has no UI at all.  Personally I'm pushing
my data in on the basis that it costs me basically nothing to do so
given that I'm already running the tests.

> There is a bit of a chicken-and-egg problem, since without the test
> results getting uploaded, it's hard to get the analysis functionality
> implemented, and without the analysis features, it's hard to get
> developers to upload the data.

I think if we get tooling in place so that people can just run a script,
add a flag to their tools or whatever to ingest results from the
standard testsuites the barrier to reporting becomes sufficiently low
that it's more of a "why not?" type thing.

There's also other things we can do beyond big data analysis, some of
which are a lot easier - for example checking other people's CI results
for your branch before sending or accepting a pull request (if you've
got a one stop shop to pull data from that's a lot easier than if you
have to go round a bunch of places).

> That being said, a number of file system developers probably have
> several years worth of test results that we could probably give you.
> I have hundreds of junit.xml files, with information about how kernel
> version, what version of xfstesets, etc, that was used.  I'm happy to
> make samples of it available for anyone who is interested.

Right, I've likewise got a pile of results I can reprocess at will.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Nikolai Kondrashov]

Hi Ricardo,

On 11/20/23 15:30, Ricardo Cañuelo wrote:
> On mié, nov 15 2023 at 19:43:49, Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com> wrote:
>> Introduce a new tag, 'Tested-with:', documented in the
>> Documentation/process/submitting-patches.rst file. The tag is expected
>> to reference the documented test suites, similarly to the 'V:' field,
>> and to certify that the submitter executed the test suite on the change,
>> and that it passed.
> 
> I think the 'V:' field in MAINTAINERS is a good addition to document
> what developers are supposed to test for every subsystem, but in the
> case of the per-commit "Tested-with:" tag, I think the real value of it
> would be in using it for accountability and traceability purposes
> instead, that is, to link to the actual results of the (automatic) tests
> that were used to validate a commit.
> 
> This would provide two important features:
> 
> 1. Rather than trusting that the tester did things right and that the
>     test environment used was appropriate, we'd now have proof that the
>     test results are as expected and a way to reproduce the steps.
> 
> 2. A history of test results for future reference. When a regression is
>     introduced, now we'd have more information about how things worked
>     back when the test was still passing.
> 
> This is not trivial because tests vary a lot and we'd first need to
> define which artifacts to link to, and because whatever is linked (test
> commands, output log, results summary) would need to be stored
> forever. But since we're doing that already for basically all kernel
> mailing lists, I wonder if something like "public-inbox for test
> results" could be possible some day.

I agree that it would be good to have a record of the actual test results
uploaded somewhere. For the start, I think it's fine to just have them
uploaded to places like Dropbox or Google Drive, or whatever can be accessed
with an unauthenticated URL.

Otherwise I'm seriously considering opening up submissions to KCIDB for the
general (authenticated) public (with pre-moderation and whitelisting). That
would require a bunch of work, though. We already have basic artifact
mirroring system, but it relies on the submitter hosting the files somewhere
in the first place. So we'd have to add some file upload service to that. And
then we'd have to think really hard on how to keep the access public, and at
the same time not to go bankrupt from somebody scraping our archive in S3
storage. Any help would be super-welcome!

I think we can make space for the results URL after the test name in the
Tested-with: tag. We can probably make up some syntax for the V: field that
would say if the URL is required or not, but it could just be always accepted.
It will be the maintainer's call to require it or not.

I think it should be up to the test to define what their output should be, and
it would be very hard (and not that useful) to unify them in a single output
format (speaking from Red Hat's CKI experience executing many different suites
for the kernel). The test name in the Tested-with: tag would help identify the
format, if necessary.

Nick

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Nikolai Kondrashov]

Hi Theodore,

On 11/20/23 22:51, Theodore Ts'o wrote:
> On Mon, Nov 20, 2023 at 02:30:49PM +0100, Ricardo Cañuelo wrote:
>>
>> This is not trivial because tests vary a lot and we'd first need to
>> define which artifacts to link to, and because whatever is linked (test
>> commands, output log, results summary) would need to be stored
>> forever. But since we're doing that already for basically all kernel
>> mailing lists, I wonder if something like "public-inbox for test
>> results" could be possible some day.
> 
> What we have at work is a way to upload the test results summary
> (e.g., just KTAP result lines, or the xfstests junit XML) along with
> test run metadata (e.g., what was the kernel commit on which the test
> was run, and the test hardware), and this would be stored permanently.
> Test artifacts is also preserved but for a limited amount of time
> (e.g., some number of months or a year).
> 
> The difference in storage lifetimes is because the junit XML file
> might be a few kilobytes to tens of kilobytes. but the test artifacts
> might be a few megabytes to tens of megabytes.
> 
> Of course once you have this data, it becomes possible to detect when
> a test may have regressed, or to detect flaky tests, and perhaps to
> figure out if certain hardware configurations or kernel configurations
> are more likely to trigger a particular test to fail.  So having all
> of this data stored centrally would be really cool.  The only question
> is who might be able to create such an infrastructure, and be able to
> pay for the ongoing development and operational costs....

Yes, I agree, having public result storage would be awesome. I think KCIDB is 
relatively-well positioned to take on some of that work. We have the POC 
dashboard already. Well, *had* until somebody scraped it and exhausted our 
cloud budget, I'm working on making it cheaper before bringing it back.

Meanwhile you can get a glimpse of it in one of my slide decks:
https://static.sched.com/hosted_files/eoss2023/ef/Kernel%20CI%20%E2%80%93%20How%20Far%20Can%20It%20Go%20%E2%80%93%20EOSS%202023.pdf

We also have an artifact-mirroring system (quite basic at the moment), 
expecting the submitter to already have the files hosted somewhere. We would 
need to add a file upload service to that.

Finally, I'm considering opening up submissions to (Google-authenticated) 
public, as opposed to just CI systems, so we could support this. That's still 
more work though.

Regarding analysis, I have plenty of ideas for that too, and even more ideas 
are explored by others lately. I just need to bring back the dashboard and 
find the time for all of it :D

Anyone interested in helping with any of this?

Nick

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1772 bytes --]

On Tue, Nov 21, 2023 at 06:36:10PM +0800, David Gow wrote:

> The other question is how to handle outdated results when a new patch
> revision is sent out. Personally, I think this is something we can
> solve similarly to 'Reviewed-by', depending on the extent of the
> changes and cost of the tests. I suspect for most automated tests,
> this would mean never carrying the 'Tested-with' tag over, but if
> testing it involved manually building and running kernels against 50
> different hardware setups, I could imagine it making sense to not
> re-do this if a new revision just changed a doc typo. If a URL is used
> here, it could contain version info, too.

One thing with Reviewed-by that's a bit different to testing is that
Reviewed-by is generally invalidated by doing a change to the specific
patch that needs at least a commit --amend.

> Personally, I'd like to require that all patches have a 'Tested-with'
> field, even if there's not a corresponding 'V' MAINTAINERS entry, as
> people should at least think of how something's tested, even if
> there's not a formal 'test suite' for it. Though that seems a
> longer-term goal

A requirement feels like it'd be pretty painful for my workflow, or at
least result in me adding the thing in hope of what I'm actually going
to do rather than as a result of the testing - all my CI stuff
(including what I do for outgoing patches) is keyed off the git commits
being tested so updating the commits to reflect testing would have
unfortunate side effects.

> The questions I think we need to answer to get this in are:
> 1. Do we want to split this up (and potentially land it
> piece-by-piece), or is it more valuable to have a stricter, more
> complete system from the get-go?

I think splitting things makes sense.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[kernel test robot]

Hi Nikolai,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.7-rc2 next-20231121]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Nikolai-Kondrashov/MAINTAINERS-Introduce-V-field-for-required-tests/20231116-015419
base:   linus/master
patch link:    https://lore.kernel.org/r/20231115175146.9848-2-Nikolai.Kondrashov%40redhat.com
patch subject: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests
reproduce: (https://download.01.org/0day-ci/archive/20231122/202311220843.vh7WyxDF-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311220843.vh7WyxDF-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> Documentation/process/tests.rst: WARNING: document isn't included in any toctree

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/3] MAINTAINERS: Require kvm-xfstests smoke for ext4

[Nikolai Kondrashov]

On 11/20/23 00:54, Theodore Ts'o wrote:
> So as for *me*, I'm going to point people at:
> 
> https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md

...

> (And note that I keep the xfstests-bld repo's on kernel.org and
> github.com both uptodate, and I prefer using the using the github.com
> URL because it's easier for the new developer to read and understand
> it.)

I already queued a switch to the kernel.org URL, which Darrick has suggested.
I'll drop it now, but you guys would have to figure it out between yourselves,
which one you want :D

Personally, I agree that the one on GitHub is more reader-friendly, FWIW.

> And similarly, just because the V: line might say, "kvm-xfstests
> smoke", someone could certainly use kdevops if they wanted to.  So
> perhaps we need to be a bit clearer about what we expect the V: line
> to mean?

I tried to handle some of that with the "subsets", so that you can run a wider
test suite and still pass the Tested-with: check. I think this has to be
balanced between allowing all the possible ways to run the tests and a
reasonable way to certify the commit was tested automatically.

E.g. name the test "xfstests", and list all the ways it can be executed, thus
communicating that it should still say "Tested-with: xfstests" regardless of
the way. And if there is a smaller required subset, name it just "xfstests
smoke" and list all the ways it can be run, including the simplest
"kvm-xfstests smoke", but accept just "Tested-with: xfstests-smoke".

I'm likely getting things wrong, but I hope you get what I'm trying to say.

> Along these lines, we should perhaps be a bit more thoughtful about
> the intended audience for Documentation/process/tests.rst.  I
> personally wouldn't try ask first-time kernel developers to look at
> the xfstests group files, because that's just way too complicated for
> them.
> 
> And I had *assumed* that Documentation/process/tests.rst was not
> primarily intended for sophistiocated file system developers who
> wouldn't be afraid to start looking at the many ways that xfstests
> could be configured.  But we should perhaps be a bit more explicit
> about who the intended audience would be for a certain set up
> Documentation files, since that will make it easier for us to come to
> consensus about how that particular piece of documentation would be
> worded.
> 
> As E.B. White (author of the book "The Elements of Style" was reputed
> to have once said, "Always write with deep sympathy for the reader".
> Which means we need to understand who the reader is, and to try to
> walk in their shoes, first.

Amen to that! Apart from the newbies and just people working on other
subsystems, we should also remember to be kinder to ourselves and keep our own
tools easier to use. So perhaps just say "newbies should be able to follow
tests.rst", and enjoy it :D

Ultimately, I think the (admittedly elusive) target should be the ability to
just plop a command line into every V: entry, running something from the tree
itself. Meanwhile, we would need the stepping stone of tests.rst, or something
like that, to walk people through whatever setup is required.

I'll see how we can accommodate the commands in the V: directly, though.

Nick

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Theodore Ts'o]

On Tue, Nov 21, 2023 at 01:27:44PM +0000, Mark Brown wrote:
> > (I don't need to see all of the tests that passes; it's the test
> > failures or the test flakes that are significant.)
> 
> The listing of tests does get a bit more complex when you mix in running
> on different platforms.

Yeah, that's part of the aggregation reports problem.  Given a
particular test, say, generic/475, I'd love to see a summary of which
file system config (e.g., ext4/4k, ext4/1k) and which architectures a
particular test is failing or which is flaky.  Right now, I do this
manually using a combination of a mutt mail reader (the test summaries
are e-mailed to me), and emacs....

> I think if we get tooling in place so that people can just run a script,
> add a flag to their tools or whatever to ingest results from the
> standard testsuites the barrier to reporting becomes sufficiently low
> that it's more of a "why not?" type thing.

Sure, I'm happy to add something like that to my test runners:
kvm-xfstests, gce-xfstests, and android-xfstests.  Then anyone who
uses my test runner infrastructure would get uploading for free.  We
might need to debate whether I enable uploading as something which is
enabled by default or not (I can imagine some people won't wanting to
upload information to a public site, lest it leak information about an
upcoming mobile handset :-), but that's a minor point.

Personally, I'm not going to have time to look into this for a while,
but... patches welcome.  Or even something that takes a junit xml
file, and uploads it to the kcidb.  If someone can make something like
that available, I should be able to take it the rest of the way.

Cheers,

						- Ted

Re: [PATCH 2/3] MAINTAINERS: Require kvm-xfstests smoke for ext4

[Darrick J. Wong]

On Wed, Nov 22, 2023 at 04:44:58PM +0200, Nikolai Kondrashov wrote:
> On 11/20/23 00:54, Theodore Ts'o wrote:
> > So as for *me*, I'm going to point people at:
> > 
> > https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
> 
> ...
> 
> > (And note that I keep the xfstests-bld repo's on kernel.org and
> > github.com both uptodate, and I prefer using the using the github.com
> > URL because it's easier for the new developer to read and understand
> > it.)
> 
> I already queued a switch to the kernel.org URL, which Darrick has suggested.
> I'll drop it now, but you guys would have to figure it out between yourselves,
> which one you want :D
> 
> Personally, I agree that the one on GitHub is more reader-friendly, FWIW.

For xfstests-bld links, I'm ok with whichever domain Ted wants.

> > And similarly, just because the V: line might say, "kvm-xfstests
> > smoke", someone could certainly use kdevops if they wanted to.  So
> > perhaps we need to be a bit clearer about what we expect the V: line
> > to mean?
> 
> I tried to handle some of that with the "subsets", so that you can run a wider
> test suite and still pass the Tested-with: check. I think this has to be
> balanced between allowing all the possible ways to run the tests and a
> reasonable way to certify the commit was tested automatically.
> 
> E.g. name the test "xfstests", and list all the ways it can be executed, thus
> communicating that it should still say "Tested-with: xfstests" regardless of
> the way. And if there is a smaller required subset, name it just "xfstests
> smoke" and list all the ways it can be run, including the simplest
> "kvm-xfstests smoke", but accept just "Tested-with: xfstests-smoke".
> 
> I'm likely getting things wrong, but I hope you get what I'm trying to say.

Not entirely -- for drive-by contributions and obvious bugfixes, a quick
"V: xfstests-bld: kvm-xfstests smoke" / "V: fstests: ./check -g smoke"
run is probably sufficient.

(Insofar as n00bs running ./check isn't sufficient, but that's something
that fstests needs to solve...)

For nontrivial code tidying, the author really ought to run the whole
test suite.  It's still an open question as to whether xfs tidying
should run the full fuzz suite too, since that increases the runtime
from overnightish to a week.

For /new features/, the developer(s) ought to come up with a testing
plan and run that by the community.  Eventually those will merge into
fstests or ktest or wherever.

--D

> > Along these lines, we should perhaps be a bit more thoughtful about
> > the intended audience for Documentation/process/tests.rst.  I
> > personally wouldn't try ask first-time kernel developers to look at
> > the xfstests group files, because that's just way too complicated for
> > them.
> > 
> > And I had *assumed* that Documentation/process/tests.rst was not
> > primarily intended for sophistiocated file system developers who
> > wouldn't be afraid to start looking at the many ways that xfstests
> > could be configured.  But we should perhaps be a bit more explicit
> > about who the intended audience would be for a certain set up
> > Documentation files, since that will make it easier for us to come to
> > consensus about how that particular piece of documentation would be
> > worded.
> > 
> > As E.B. White (author of the book "The Elements of Style" was reputed
> > to have once said, "Always write with deep sympathy for the reader".
> > Which means we need to understand who the reader is, and to try to
> > walk in their shoes, first.
> 
> Amen to that! Apart from the newbies and just people working on other
> subsystems, we should also remember to be kinder to ourselves and keep our own
> tools easier to use. So perhaps just say "newbies should be able to follow
> tests.rst", and enjoy it :D
> 
> Ultimately, I think the (admittedly elusive) target should be the ability to
> just plop a command line into every V: entry, running something from the tree
> itself. Meanwhile, we would need the stepping stone of tests.rst, or something
> like that, to walk people through whatever setup is required.
> 
> I'll see how we can accommodate the commands in the V: directly, though.
> 
> Nick
> 

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Nikolai Kondrashov]

On 11/21/23 12:36, David Gow wrote:
> Thanks so much for doing this! I think everyone agrees that we need
> _some_ way of documenting which tests to run, and I think this is our
> best option.

Awesome :D

> In any case, this patch does a lot, and I'll comment on them
> one-by-one. (It may be worth splitting this patch up into a few
> separate bits, if only so that we can better separate the
> uncontroversial bits from the open questions.)

Yeah, I'll try to figure out a less controversial split.

> On Thu, 16 Nov 2023 at 01:52, Nikolai Kondrashov
>> Each referenced test suite is expected to be documented in the new
>> Documentation/process/tests.rst file, which must have enough structure
>> (documented inside) for the tools to make use of it. Apart from basic
>> data, each test can refer to its "superset" - a test suite which this
>> one is a part of. The expected use is to describe both a large test
>> suite and its subsets, so the former would also be accepted, if a
>> subsystem requires only a subset.
> 
> I think this could work, but is a bit complicated.
> 
> My initial thought was to have this as a more free-form field, which
> either contained a:
> - Path to a command to run (e.g. tools/testing/kunit/run_checks.py)
> - Path to a documentation file describing the test.
> - URL to a page describing the test
> - (Maybe) freeform text.

I think we should be careful about having too much flexibility here, if we
want to have tools process this. I mean, we would then have to formalize and
define the syntax for all the cases, which could then become too obscure for
humans to easily read and write.

As I said elsewhere, our ultimate (even if unachievable) target should be to
have commands to execute (instead of long setup and execution instructions),
for *all* V: entries. So that definitely should be supported. The current
patch already supports putting a command in the tests.rst to be printed by
checkpatch.pl. Perhaps we can allow putting it into the V: entry directly. I
have one idea on how to do that.

OTOH, I think there's value in an overall catalogue of tests and having
easily-accessible documentation for that command (even if brief), and that's
what going for the command via tests.rst allows.

Regarding a path to the documentation file, that goes against the idea of a
catalogue file, again, so I'm reluctant of letting it go. Same goes for a
documentation URL. Both of these can be expressed via the catalogue too.

I wonder if it could be useful to add another option to get_maintainer.pl, or
implement a new script, which would just dump the referenced test catalogue
sections for a patchset, for a longer-form read than what checkpatch.pl can
produce, but faster than scanning the whole catalogue personally.

> It's probably worth also looking at this proposal to auto-generate
> similar documentation:
> https://lore.kernel.org/linux-kselftest/cover.1689171160.git.mchehab@kernel.org/

IIRC, Mauro has mentioned this effort to me at EOSS in Prague, but I still
haven't found the time to look at it closely. I think this is a worthy effort
overall, but at a somewhat lower level. What I would like to be in the
tests.rst is a basic intro and help to get each corresponding test suite
running, to help breach the gap between trying to contribute and having your
contribution tested with what maintainers prescribe. The docs in tests.rst can
point to the more detailed docs, in turn. I also think it's good to have a
document with an overview of what tests exist in general and which areas they
address.

> The other question is how to handle outdated results when a new patch
> revision is sent out. Personally, I think this is something we can
> solve similarly to 'Reviewed-by', depending on the extent of the
> changes and cost of the tests. I suspect for most automated tests,
> this would mean never carrying the 'Tested-with' tag over, but if
> testing it involved manually building and running kernels against 50
> different hardware setups, I could imagine it making sense to not
> re-do this if a new revision just changed a doc typo. If a URL is used
> here, it could contain version info, too.

Yeah, that would be painful.

> Paste encouragement for in-tree test suites triggered by git forges here <

I think what Ricardo is suggesting in another branch, regarding adding result
URLs, could work. So it would be nice to account for that in the change, but
the policy would probably depend on subsystem/maintainer/the change in question.

>> Introduce a new tag, 'Tested-with:', documented in the
>> Documentation/process/submitting-patches.rst file. The tag is expected
>> to reference the documented test suites, similarly to the 'V:' field,
>> and to certify that the submitter executed the test suite on the change,
>> and that it passed.
> 
> I'm also 100% for this, though I'd considered it separately from the
> MAINTAINERS change.
> 
> I think, in the ideal case, we want this to link to the results
> somehow. kcidb would seem to be the obvious choice there.
> 
> Again, as a fallback, a plain text field would be useful to describe
> cases where a patch was tested by some means other than a formal test
> suite. This might not be ideal, but I'd still rather have people
> describe that something "builds and boots on <x> hardware" than have
> to guess if a patch was tested at all.
> 
> Of course, it'd then be up to maintainers to decide what they'd
> accept: I'd expect that some would require there be a 'Tested-with'
> header which links to valid results for the tests described in
> MAINTAINERS.

I'm thinking that maybe we should just not *require* a valid reference to a
documented test in the Tested-with: field. I.e. only verify that all the V:
values are listed, but ignore everything unknown.

>> Make scripts/checkpatch.pl ensure any added V: fields reference
>> documented test suites only, and output a warning if a change to a
>> subsystem doesn't certify the required test suites were executed,
>> if any.
> 
> I'd definitely want something like this to run at some point in the
> patch-submission workflow. I think that, ultimately, we'll want to be
> able to run some tests automatically (e.g., a git hook could run the
> tests and add the 'Tested-with' line).
> 
> Personally, I'd like to require that all patches have a 'Tested-with'
> field, even if there's not a corresponding 'V' MAINTAINERS entry, as
> people should at least think of how something's tested, even if
> there's not a formal 'test suite' for it. Though that seems a
> longer-term goal

Yeah, that would be nice from a maintainer's POV, but probably not very popular.

>> If the test suite description includes a "Command", then checkpatch.pl
>> will output it as the one executing the suite. The command should run
>> with only the kernel tree and the regular developer environment set up.
>> But, at the same time, could simply output instructions for installing
>> any extra dependencies (or pull some automatically). The idea is to
>> get the developer into feedback loop quicker and easier, so they have
>> something to run and iterate on, even if it involves installing some
>> more stuff first. Therefore it's a good idea to add such wrappers to the
>> kernel tree proper and refer to them from the tests.rst.
>>
>> Extend scripts/get_maintainer.pl to support retrieving the V: fields,
>> and scripts/parse-maintainers.pl to maintain their ordering.
>>
>> Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
>> ---
> 
> The questions I think we need to answer to get this in are:
> 1. Do we want to split this up (and potentially land it
> piece-by-piece), or is it more valuable to have a stricter, more
> complete system from the get-go?

I'll see what I can do about splitting it.

> 2. What format should the 'V' line take? If it is simply a test name,
> do we use a doc as suggested (or one generated in part from some other
> process), or something like a command name or URL? Can it just be
> freeform text?
> 3. Should 'Tested-with' be a test name in the same format as 'V', a
> URL to results (any URL, or just kcidb?), or freeform text? How does
> this evolve with multiple versions of patches?

I don't think it's useful to restrict this to kcidb. I think the tools should
generally allow anything there, but verify the entries by MAINTAINERS are
there, as I write above.

> 4. How should this be enforced? A warning (not an 'error') from
> checkpatch? A separate script?
> 
> Personally, my gut feeling is that we should land the simplest, most
> minimal version of this (the 'V' field, as freeform text) now, and
> build on that as consensus and tooling permits. I'd probably also add
> the 'Tested-with' or similar tag, as freeform text, too. I don't think
> either of those would cause major problems if we needed to change or
> restrict the format later; I imagine there won't be a huge need to
> parse old commits for test data, and even if so, it wouldn't be too
> hard to ignore any which don't conform to any stricter future
> convention.
> 
> But I don't think there's anything fundamentally wrong with the full
> plan as-is, so if everyone's happy with it, I'd not object to having
> it.

I agree that the minimum support (just the freeform V:/Tested-with:) would be
easier to get merged, but would also be somewhat toothless. I think some sort
of test documentation/catalogue adds value, and a message from checkpatch.pl
even more so, as it greatly aids test discoverability, and I is a major point
of the change. We can lower the WARN to INFO to reduce resistance, or even
maybe make the level configurable in the V: field.

Anyway, I'll work on splitting this.

Thanks a lot for the extensive and insightful comments, David!

Nick

Re: [PATCH 3/3] MAINTAINERS: Require kunit core tests for framework changes

[Nikolai Kondrashov]

On 11/20/23 20:48, Daniel Latypov wrote:
> On Wed, Nov 15, 2023 at 9:52 AM Nikolai Kondrashov
> <Nikolai.Kondrashov@redhat.com> wrote:
>> +kunit core
>> +----------
>> +
>> +:Summary: KUnit tests for the framework itself
>> +:Superset: kunit
>> +:Command: tools/testing/kunit/kunit.py run --kunitconfig lib/kunit
> 
> Note: we'd want this to instead be
>    ./tools/testing/kunit/run_checks.py
> 
> That will run, in parallel
> * kunit.py run --kunitconfig lib/kunit
> * kunit_tool_test.py (the unit test for kunit.py)
> * two python type-checkers, if installed

Noted, queued!

>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index f81a47d87ac26..5f3261e96c90f 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11626,6 +11626,7 @@ L:      linux-kselftest@vger.kernel.org
>>   L:     kunit-dev@googlegroups.com
>>   S:     Maintained
>>   W:     https://google.github.io/kunit-docs/third_party/kernel/docs/
>> +V:     kunit core
> 
> Maybe this topic should go in the main thread, but I wanted to call it
> out here since this is a good concrete example.
> 
> I'm wondering if this entry could simply be
>    V: ./tools/testing/kunit/run_checks.py
> 
> And what if, for ext4, we could have multiple of these like
>    V: kvm-xfstests smoke
>    V: tools/testing/kunit/kunit.py run --kunitconfig fs/ext4
> 
> I.e. what if we allowed the `V:` field to contain the command itself?
> 
> # Complexity of the tests
> 
> I appreciate the current "named test-set" approach since it encourages
> documenting *why* the test command is applicable.
> And for a lot of tests, it's not as simple as a binary GOOD/BAD
> result, e.g. benchmarks.
> Patch authors need to understand what they're testing, if they're
> testing the right thing, etc.
> 
> But on the other hand, for simpler functional tests (e.g. KUnit),
> maybe it's not necessary.
> Ideally, KUnit tests should be written so the failure messages are
> immediately actionable w/o having to read a couple paragraphs.
> I.e. the test case names should make it clear what scenario they're
> testing, or the test code should be sufficiently documented, etc.
> 
> # Variations on commands
> 
> And there might be a bunch of slight variations on these commands,
> e.g. only different in terms of `--kunitconfig`.
> We'd probably have at least 18, given
> $ find -type f -name '.kunitconfig' | wc -l
> 18
> 
> And again using a kunit.py flag as an example, what if maintainers want KASAN?
>    ./tools/testing/kunit/kunit.py run --kunitconfig lib/kunit
> --kconfig_add CONFIG_KASAN=y
> Or what if it's too annoying to split up a larger kunit suite, so I
> ask people to run a subset?
>    ./tools/testing/kunit/kunit.py run --kunitconfig lib/kunit <suite_glob>
> 
> 
> P.S.
> Tbh, I have always hoped we'd eventually have a field like
>    V: <one-liner test command>
> 
> That is part of why I added all those features above (--kunitconfig,
> --kconfig_add, glob support, run_checks.py, etc.).
> I wanted kunit.py to be flexible enough that maintainers could state
> their testing requirements as a one-liner that people can just
> copy-paste and run.
> 
> Also, I recently talked to David Gow and I know he was interested in
> adding another feature to kunit.py to fit this use case.
> Namely, the ability to do something like
>    kunit.py run --arches=x86_64,s390,...
> and have it run the tests built across N different arches and maybe w/
> M different kconfig options (e.g. a variation built w/ clang, etc.).
> 
> That would be another very powerful tool for maintainers to have.
> 
> Thanks so much for this patch series and starting this discussion!

I'm a bit squeamish about just letting commands into the V: fields, as it 
hurts discoverability of the documentation (or even just a simple description 
of what the command does), and then checkpatch.pl would have a problem 
identifying the modified command in Tested-with:.

OTOH, we're all hackers here, and I understand where these arguments are 
coming from, and as I said in other branches, I think command-first should be 
our ultimate target, not instructions-first. Also, if each of these commands 
supports the -h/--help options and manages to make sense in their output, it 
makes things somewhat better.

All-in-all, I think we can have both the already-implemented "V: test name -> 
catalogue -> command" route, and the "V: command" one.

Something like this for the commands:

     V: tools/testing/kunit/run_checks.py

and

     V: "kvm-xfstests smoke"

for test names referencing the catalogue.

Then make checkpatch.pl verify only the presence of Tested-with: tag for the 
latter, and leave verifying the (more fluid) commands to humans.

Thanks for all the comments, explanations, and details, Daniel!

Nick

Re: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests

[Nikolai Kondrashov]

Hi Gustavo,

On 11/20/23 14:40, Gustavo Padovan wrote:
> On Thursday, November 16, 2023 09:14 -03, Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com> wrote:
> 
>> On 11/15/23 22:38, Konstantin Ryabitsev wrote:
>>> On Wed, Nov 15, 2023 at 07:43:49PM +0200, Nikolai Kondrashov wrote:
>>>> Introduce a new tag, 'Tested-with:', documented in the
>>>> Documentation/process/submitting-patches.rst file. The tag is expected
>>>> to reference the documented test suites, similarly to the 'V:' field,
>>>> and to certify that the submitter executed the test suite on the change,
>>>> and that it passed.
>>>
>>> I'm not sure it should be a tag that stays all the way through git commit.
>>> How about making it a cover letter/first patch footer?
>>>
>>> tested-with: <suite>
>>
>> Yes, that would be better indeed. However, checkpatch.pl doesn't process cover
>> letters, and so we would have no automated way to advertise and nudge people
>> towards testing.
>   
> At this year's LPC, there was quite some discussion around improving testing information, so this patch of yours lands has a great timing. :)

Lucky me :D

> All your are proposing here is pretty interesting both for developers and CI systems, but it seems that a "Tested-with:" tag and checkpatch.pl validation will take quite some time to consolidate.
> 
> Would it make sense to split just the part that adds the V: field to MAINTAINERS and submit that as separate patchset together with its documentation? That way we can enable subsystem to start annotating their test suites already.

Yeah, I'll try to split this along controversy lines in the next version.

> I also wonder how to make for subsystems that will have different test suites (eg something in kselftests and an external test suite). Would an alternative be pointing to a Documentation page with detailed info?

As Mark already noted, you can always just put more V: entries there, but you
can also create a "meta-test" in the catalogue listing all the different test
suites, and reference it from the V: entry, if you'd like.

Nick

Re: [PATCH 2/3] MAINTAINERS: Require kvm-xfstests smoke for ext4

[Nikolai Kondrashov]

On 11/22/23 18:17, Darrick J. Wong wrote:
> On Wed, Nov 22, 2023 at 04:44:58PM +0200, Nikolai Kondrashov wrote:
>> On 11/20/23 00:54, Theodore Ts'o wrote:
>> I already queued a switch to the kernel.org URL, which Darrick has suggested.
>> I'll drop it now, but you guys would have to figure it out between yourselves,
>> which one you want :D
>>
>> Personally, I agree that the one on GitHub is more reader-friendly, FWIW.
> 
> For xfstests-bld links, I'm ok with whichever domain Ted wants.

Great! I just hope I can keep track of all the requests :D

>>> And similarly, just because the V: line might say, "kvm-xfstests
>>> smoke", someone could certainly use kdevops if they wanted to.  So
>>> perhaps we need to be a bit clearer about what we expect the V: line
>>> to mean?
>>
>> I tried to handle some of that with the "subsets", so that you can run a wider
>> test suite and still pass the Tested-with: check. I think this has to be
>> balanced between allowing all the possible ways to run the tests and a
>> reasonable way to certify the commit was tested automatically.
>>
>> E.g. name the test "xfstests", and list all the ways it can be executed, thus
>> communicating that it should still say "Tested-with: xfstests" regardless of
>> the way. And if there is a smaller required subset, name it just "xfstests
>> smoke" and list all the ways it can be run, including the simplest
>> "kvm-xfstests smoke", but accept just "Tested-with: xfstests-smoke".
>>
>> I'm likely getting things wrong, but I hope you get what I'm trying to say.
> 
> Not entirely -- for drive-by contributions and obvious bugfixes, a quick
> "V: xfstests-bld: kvm-xfstests smoke" / "V: fstests: ./check -g smoke"
> run is probably sufficient.
> 
> (Insofar as n00bs running ./check isn't sufficient, but that's something
> that fstests needs to solve...)
> 
> For nontrivial code tidying, the author really ought to run the whole
> test suite.  It's still an open question as to whether xfs tidying
> should run the full fuzz suite too, since that increases the runtime
> from overnightish to a week.
> 
> For /new features/, the developer(s) ought to come up with a testing
> plan and run that by the community.  Eventually those will merge into
> fstests or ktest or wherever.

Of course, makes sense. Thank you!

Nick

Re: [PATCH 2/3] MAINTAINERS: Require kvm-xfstests smoke for ext4

[Dave Chinner]

On Wed, Nov 22, 2023 at 08:17:46AM -0800, Darrick J. Wong wrote:
> On Wed, Nov 22, 2023 at 04:44:58PM +0200, Nikolai Kondrashov wrote:
> > On 11/20/23 00:54, Theodore Ts'o wrote:
> > > So as for *me*, I'm going to point people at:
> > > 
> > > https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
> > 
> > ...
> > 
> > > (And note that I keep the xfstests-bld repo's on kernel.org and
> > > github.com both uptodate, and I prefer using the using the github.com
> > > URL because it's easier for the new developer to read and understand
> > > it.)
> > 
> > I already queued a switch to the kernel.org URL, which Darrick has suggested.
> > I'll drop it now, but you guys would have to figure it out between yourselves,
> > which one you want :D
> > 
> > Personally, I agree that the one on GitHub is more reader-friendly, FWIW.
> 
> For xfstests-bld links, I'm ok with whichever domain Ted wants.
> 
> > > And similarly, just because the V: line might say, "kvm-xfstests
> > > smoke", someone could certainly use kdevops if they wanted to.  So
> > > perhaps we need to be a bit clearer about what we expect the V: line
> > > to mean?
> > 
> > I tried to handle some of that with the "subsets", so that you can run a wider
> > test suite and still pass the Tested-with: check. I think this has to be
> > balanced between allowing all the possible ways to run the tests and a
> > reasonable way to certify the commit was tested automatically.
> > 
> > E.g. name the test "xfstests", and list all the ways it can be executed, thus
> > communicating that it should still say "Tested-with: xfstests" regardless of
> > the way. And if there is a smaller required subset, name it just "xfstests
> > smoke" and list all the ways it can be run, including the simplest
> > "kvm-xfstests smoke", but accept just "Tested-with: xfstests-smoke".
> > 
> > I'm likely getting things wrong, but I hope you get what I'm trying to say.
> 
> Not entirely -- for drive-by contributions and obvious bugfixes, a quick
> "V: xfstests-bld: kvm-xfstests smoke" / "V: fstests: ./check -g smoke"
> run is probably sufficient.

For trivial drive-by contributions and obvious bug fixes, I think
this is an unnecessary burden on these potential contributors. If
it's trivial, then there's little burden on the reviewer/maintainer
to actually test it, whilst there is significant burden on the
one-off contributor to set up an entirely new, unfamiliar testing
environment just to fix something trivial.

If you want every patch tested, the follow the lead of the btrfs
developers: set up a CI mechanism on github and ask people to submit
changes there first and then provide a link to the successful test
completion ticket with the patch submission.

> (Insofar as n00bs running ./check isn't sufficient, but that's something
> that fstests needs to solve...)
> 
> For nontrivial code tidying, the author really ought to run the whole
> test suite.  It's still an open question as to whether xfs tidying
> should run the full fuzz suite too, since that increases the runtime
> from overnightish to a week.

Yes, the auto group tests should be run before non-trivial patch
sets are submitted. That is the entire premise of the the auto group
existing - it's the set of regression tests we expect to pass for
any change.

However, the full on disk format fuzz tests should not be required
to be run. Asking people to run tests that take a week for general
cleanups and code quality improvements is just crazy talk - the
cost-benefit equation is all out of whack here, especially if the
changes have no interaction with the on-disk format at all.

IMO, extensive fuzz testing is something that should be done
post-integration - requiring individual developers to run
tests that take at least a week to run before they can submit a
patchset for review/inclusion is an excessive burden, and we don't
need every developer to run such tests every time they do something
even slightly non-trivial.

It is the job of the release manager to co-ordinate with the testing
resources to run extensive, long running tests after code has been
integrated into the tree. Forcing individual developers to run this
sort of testing just isn't an efficient use of resources....

> For /new features/, the developer(s) ought to come up with a testing
> plan and run that by the community.  Eventually those will merge into
> fstests or ktest or wherever.

That's how it already works, isn't it?

-Dave.
-- 
Dave Chinner
david@fromorbit.com

[RFC PATCH v2 00/10] MAINTAINERS: Introduce V: entry for tests

[Nikolai Kondrashov]

Alright, here's a second version, attempting to address as many concerns as
possible. It's likely I've missed something, though.

Changes from v1:

* Make scripts/get_maintainer.pl survive querying missing files, giving a
  warning instead. This is necessary to enable scripts/checkpatch.pl to query
  MAINTAINERS about files being deleted.
* Start with the minimal change just documenting the V: entry, which accepts
  test commands directly, and tweaking the tools to deal with that.
* However, require the commands accept the -h/--help option so that users have
  an easier time getting *some* help. The run_checks.py missing that is the
  reason why the patch proposing it for kunit subsystem is marked "DONOTMERGE"
  in this version. We can drop that requirement, or soften the language, if
  there's opposition.
* Have a *separate* patch documenting 'Tested-with:' as the next (early)
  change. Mention that you can add a '#' followed by a results URL, on the
  end. Adjust the V: docs/checks to exclude '#'.
* Have a *separate* patch making scripts/checkpatch.pl propose the execution
  of the test suite defined in MAINTAINERS whenever the corresponding
  subsystem is changed.
* However, use 'CHECK', instead of 'WARNING', to allow submitters specify the
  exact (and potentially slightly different) command they used, and not have
  checkpatch.pl complain too loudly that they didn't run the (exact
  MAINTAINERS-specified) command. This unfortunately means that unless you use
  --strict, you won't see the message. We'll try to address that in a new
  change at the end.
* Have a *separate* patch introducing the test catalog and accepting
  references to that everywhere, with a special syntax to distinguish them
  from verbatim/direct commands. The syntax is prepending the test name with a
  '*' (just like C pointer dereference). Make checkpatch.pl handle that.
* Drop the recommendation to have the "Docs" and "Sources" fields in test
  descriptions, as the description text should focus on giving a good
  introduction and not prompt the user to go somewhere else immediately. They
  both can be referenced in the text where and how is appropriate.
* Generally keep the previous changes adding V: entries and test suite docs,
  and try to accommodate all the requests, but refine the "Summary" fields to
  fit the checkpatch.pl messages better.
* Have a separate patch cataloguing the complete kunit suite.
* Finally, add a patch introducing the "proposal strength" keywords
  (SUGGESTED/RECOMMENDED/REQUIRED) to the syntax of V: entries, which directly
  affect which level of checkpatch.pl message missing 'Tested-with:' tags
  would generate: CHECK/WARNING/ERROR respectively. This allows subsystems to
  disable checkpatch.pl WARNINGS/ERRORS, and keep their test proposals
  inobtrusive, if they so wish. E.g. if they expect people to change their
  commands often. At the same time allow stricter workflows for subsystems
  with more uniform testing. Or e.g. for subsystems which expect the tests to
  explain their parameters in their output, and the submitters to upload and
  link their results in their 'Tested-with:' tags.

That seems to be all, but I'm sure I forgot something :D

Anyway, send me more corrections and I'll try to address them, but it's likely
going to happen next year only.

Nick
---
Nikolai Kondrashov (9):
      get_maintainer: Survive querying missing files
      MAINTAINERS: Introduce V: entry for tests
      MAINTAINERS: Propose kunit core tests for framework changes
      docs: submitting-patches: Introduce Tested-with:
      checkpatch: Propose tests to execute
      MAINTAINERS: Support referencing test docs in V:
      MAINTAINERS: Propose kvm-xfstests smoke for ext4
      docs: tests: Document kunit in general
      MAINTAINERS: Add proposal strength to V: entries

Mark Brown (1):
      MAINTAINERS: Propose kunit tests for regmap

 Documentation/process/index.rst              |   1 +
 Documentation/process/submitting-patches.rst |  46 +++++++
 Documentation/process/tests.rst              |  96 +++++++++++++++
 MAINTAINERS                                  |  17 +++
 scripts/checkpatch.pl                        | 174 ++++++++++++++++++++++++++-
 scripts/get_maintainer.pl                    |  23 +++-
 scripts/parse-maintainers.pl                 |   3 +-
 7 files changed, 355 insertions(+), 5 deletions(-)
---

[RFC PATCH v2 01/10] get_maintainer: Survive querying missing files

[Nikolai Kondrashov]

Do not die, but only warn when scripts/get_maintainer.pl is asked to
retrieve information about a missing file.

This allows scripts/checkpatch.pl to query MAINTAINERS while processing
patches which are removing files.

Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
---
 scripts/get_maintainer.pl | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 16d8ac6005b6f..37901c2298388 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -541,7 +541,11 @@ foreach my $file (@ARGV) {
 	if ((-d $file)) {
 	    $file =~ s@([^/])$@$1/@;
 	} elsif (!(-f $file)) {
-	    die "$P: file '${file}' not found\n";
+	    if ($from_filename) {
+		warn "$P: file '${file}' not found\n";
+	    } else {
+		die "$P: file '${file}' not found\n";
+	    }
 	}
     }
     if ($from_filename && (vcs_exists() && !vcs_file_exists($file))) {
-- 
2.42.0

[RFC PATCH v2 02/10] MAINTAINERS: Introduce V: entry for tests

[Nikolai Kondrashov]

Introduce a new 'V:' ("Verify") entry to MAINTAINERS. The entry accepts
a test suite command which is proposed to be executed for each
contribution to the subsystem.

Extend scripts/get_maintainer.pl to support retrieving the V: entries
when '--test' option is specified.

Require the entry values to not contain the '@' character, so they could
be distinguished from emails (always) output by get_maintainer.pl. Make
scripts/checkpatch.pl check that they don't.

Update entry ordering in both scripts/checkpatch.pl and
scripts/parse-maintainers.pl.

Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
---
 Documentation/process/submitting-patches.rst | 18 ++++++++++++++++++
 MAINTAINERS                                  |  7 +++++++
 scripts/checkpatch.pl                        | 10 +++++++++-
 scripts/get_maintainer.pl                    | 17 +++++++++++++++--
 scripts/parse-maintainers.pl                 |  3 ++-
 5 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
index 86d346bcb8ef0..f034feaf1369e 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -228,6 +228,24 @@ You should be able to justify all violations that remain in your
 patch.
 
 
+Test your changes
+-----------------
+
+Test the patch to the best of your ability. Check the MAINTAINERS file for the
+subsystem(s) you are changing to see if there are any **V:** entries
+proposing particular test suite commands. E.g.::
+
+  V: tools/testing/kunit/run_checks.py
+
+Supplying the ``--test`` option to ``scripts/get_maintainer.pl`` adds **V:**
+entries to its output.
+
+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.
+
+
 Select the recipients for your patch
 ------------------------------------
 
diff --git a/MAINTAINERS b/MAINTAINERS
index 788be9ab5b733..e6d0777e21657 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -59,6 +59,13 @@ Descriptions of section entries and preferred order
 	      matches patches or files that contain one or more of the words
 	      printk, pr_info or pr_err
 	   One regex pattern per line.  Multiple K: lines acceptable.
+	V: *Test suite* proposed for execution. The command that could be
+	   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.
+	   V: tools/testing/kunit/run_checks.py
+	   One test suite per line.
 
 Maintainers List
 ----------------
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 25fdb7fda1128..a184e576c980b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3657,7 +3657,7 @@ sub process {
 				}
 			}
 # check MAINTAINERS entries for the right ordering too
-			my $preferred_order = 'MRLSWQBCPTFXNK';
+			my $preferred_order = 'MRLSWQBCPVTFXNK';
 			if ($rawline =~ /^\+[A-Z]:/ &&
 			    $prevrawline =~ /^[\+ ][A-Z]:/) {
 				$rawline =~ /^\+([A-Z]):\s*(.*)/;
@@ -3683,6 +3683,14 @@ sub process {
 					}
 				}
 			}
+# check MAINTAINERS V: entries are valid
+			if ($rawline =~ /^\+V:\s*(.*)/) {
+				my $name = $1;
+				if ($name =~ /@/) {
+					ERROR("TEST_PROPOSAL_INVALID",
+					      "Test proposal cannot contain '\@' character\n" . $herecurr);
+				}
+			}
 		}
 
 		if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) &&
diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 37901c2298388..804215a7477db 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -53,6 +53,7 @@ my $output_section_maxlen = 50;
 my $scm = 0;
 my $tree = 1;
 my $web = 0;
+my $test = 0;
 my $subsystem = 0;
 my $status = 0;
 my $letters = "";
@@ -270,6 +271,7 @@ if (!GetOptions(
 		'scm!' => \$scm,
 		'tree!' => \$tree,
 		'web!' => \$web,
+		'test!' => \$test,
 		'letters=s' => \$letters,
 		'pattern-depth=i' => \$pattern_depth,
 		'k|keywords!' => \$keywords,
@@ -319,13 +321,14 @@ if ($sections || $letters ne "") {
     $status = 0;
     $subsystem = 0;
     $web = 0;
+    $test = 0;
     $keywords = 0;
     $keywords_in_file = 0;
     $interactive = 0;
 } else {
-    my $selections = $email + $scm + $status + $subsystem + $web;
+    my $selections = $email + $scm + $status + $subsystem + $web + $test;
     if ($selections == 0) {
-	die "$P:  Missing required option: email, scm, status, subsystem or web\n";
+	die "$P:  Missing required option: email, scm, status, subsystem, web or test\n";
     }
 }
 
@@ -634,6 +637,7 @@ my %hash_list_to;
 my @list_to = ();
 my @scm = ();
 my @web = ();
+my @test = ();
 my @subsystem = ();
 my @status = ();
 my %deduplicate_name_hash = ();
@@ -665,6 +669,11 @@ if ($web) {
     output(@web);
 }
 
+if ($test) {
+    @test = uniq(@test);
+    output(@test);
+}
+
 exit($exit);
 
 sub self_test {
@@ -850,6 +859,7 @@ sub get_maintainers {
     @list_to = ();
     @scm = ();
     @web = ();
+    @test = ();
     @subsystem = ();
     @status = ();
     %deduplicate_name_hash = ();
@@ -1072,6 +1082,7 @@ MAINTAINER field selection options:
   --status => print status if any
   --subsystem => print subsystem name if any
   --web => print website(s) if any
+  --test => print test(s) if any
 
 Output type options:
   --separator [, ] => separator for multiple entries on 1 line
@@ -1382,6 +1393,8 @@ sub add_categories {
 		push(@scm, $pvalue . $suffix);
 	    } elsif ($ptype eq "W") {
 		push(@web, $pvalue . $suffix);
+	    } elsif ($ptype eq "V") {
+		push(@test, $pvalue . $suffix);
 	    } elsif ($ptype eq "S") {
 		push(@status, $pvalue . $suffix);
 	    }
diff --git a/scripts/parse-maintainers.pl b/scripts/parse-maintainers.pl
index 2ca4eb3f190d6..dbc2b79bcaa46 100755
--- a/scripts/parse-maintainers.pl
+++ b/scripts/parse-maintainers.pl
@@ -44,6 +44,7 @@ usage: $P [options] <pattern matching regexes>
       B:  URI for bug tracking/submission
       C:  URI for chat
       P:  URI or file for subsystem specific coding styles
+      V:  Test suite name
       T:  SCM tree type and location
       F:  File and directory pattern
       X:  File and directory exclusion pattern
@@ -73,7 +74,7 @@ sub by_category($$) {
 
 sub by_pattern($$) {
     my ($a, $b) = @_;
-    my $preferred_order = 'MRLSWQBCPTFXNK';
+    my $preferred_order = 'MRLSWQBCPVTFXNK';
 
     my $a1 = uc(substr($a, 0, 1));
     my $b1 = uc(substr($b, 0, 1));
-- 
2.42.0

[RFC PATCH v2 03/10] MAINTAINERS: Propose kunit core tests for framework changes

[Nikolai Kondrashov]

DONOTMERGE: The command in question should support -h/--help option.

Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index e6d0777e21657..68821eecf61cf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11624,6 +11624,7 @@ L:	linux-kselftest@vger.kernel.org
 L:	kunit-dev@googlegroups.com
 S:	Maintained
 W:	https://google.github.io/kunit-docs/third_party/kernel/docs/
+V:	tools/testing/kunit/run_checks.py
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git kunit
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git kunit-fixes
 F:	Documentation/dev-tools/kunit/
-- 
2.42.0

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

[Nikolai Kondrashov]

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(-)

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);
 				}
 			}
 		}
-- 
2.42.0

[RFC PATCH v2 05/10] checkpatch: Propose tests to execute

[Nikolai Kondrashov]

Make scripts/checkpatch.pl output a 'CHECK' advertising any test suites
proposed for the changed subsystems, and prompting their execution.

Using 'CHECK', instead of 'WARNING', or 'ERROR', because test suite
commands executed for testing can generally be off by an option/argument
or two, depending on the situation, while still satisfying the
maintainer requirements, but failing the comparison with the V: entry
and raising alarm unnecessarily.

However, see the later patch adding the proposal strength to the V:
entry and allowing raising the severity of the message for those who'd
like that.

Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
---
 scripts/checkpatch.pl | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bea602c30df5d..1da617e1edb5f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1144,6 +1144,29 @@ sub is_maintained_obsolete {
 	return $maintained_status{$filename} =~ /obsolete/i;
 }
 
+# Test suites proposed per changed file
+our %files_proposed_tests = ();
+
+# Return a list of test suites proposed for execution for a particular file
+sub get_file_proposed_tests {
+	my ($filename) = @_;
+	my $file_proposed_tests;
+
+	return () if (!$tree || !(-e "$root/scripts/get_maintainer.pl"));
+
+	if (!exists($files_proposed_tests{$filename})) {
+		my $command = "perl $root/scripts/get_maintainer.pl --test --multiline --nogit --nogit-fallback -f $filename";
+		# Ignore warnings on stderr
+		my $output = `$command 2>/dev/null`;
+		# But regenerate stderr on failure
+		die "Failed retrieving tests proposed for changes to \"$filename\":\n" . `$command 2>&1 >/dev/null` if ($?);
+		$files_proposed_tests{$filename} = [grep { !/@/ } split("\n", $output)]
+	}
+
+	$file_proposed_tests = $files_proposed_tests{$filename};
+	return @$file_proposed_tests;
+}
+
 sub is_SPDX_License_valid {
 	my ($license) = @_;
 
@@ -2689,6 +2712,9 @@ sub process {
 	my @setup_docs = ();
 	my $setup_docs = 0;
 
+	# Test suites which should not be proposed for execution
+	my %dont_propose_tests = ();
+
 	my $camelcase_file_seeded = 0;
 
 	my $checklicenseline = 1;
@@ -2907,6 +2933,17 @@ sub process {
 				}
 			}
 
+			# Check if tests are proposed for changes to the file
+			foreach my $test (get_file_proposed_tests($realfile)) {
+				next if exists $dont_propose_tests{$test};
+				CHK("TEST_PROPOSAL",
+				    "Running the following test suite is proposed for changes to $realfile:\n" .
+				    "$test\n" .
+				    "Add the following to the tested commit's message, IF IT PASSES:\n" .
+				    "Tested-with: $test\n");
+				$dont_propose_tests{$test} = 1;
+			}
+
 			next;
 		}
 
@@ -3233,6 +3270,12 @@ sub process {
 			}
 		}
 
+# Check and accumulate executed test suites (stripping URLs off the end)
+		if (!$in_commit_log && $line =~ /^\s*Tested-with:\s*(.*?)\s*#.*$/i) {
+			# Do not propose this certified-passing test suite
+			$dont_propose_tests{$1} = 1;
+		}
+
 # Check email subject for common tools that don't need to be mentioned
 		if ($in_header_lines &&
 		    $line =~ /^Subject:.*\b(?:checkpatch|sparse|smatch)\b[^:]/i) {
-- 
2.42.0

[RFC PATCH v2 06/10] MAINTAINERS: Support referencing test docs in V:

[Nikolai Kondrashov]

Support referencing test suite documentation in the V: entries of
MAINTAINERS file. Use the '*<name>' syntax (like C pointer dereference),
where '<name>' is a second-level heading in the new
Documentation/process/tests.rst file, with the suite's description.
This syntax allows distinguishing the references from test commands.

Add a boiler-plate Documentation/process/tests.rst file, describing a
way to add structured info to the test suites in the form of field
lists. Apart from a "summary" and "command" fields, they can also
contain a "superset" field specifying the superset of the test suite,
helping reuse documentation and express both wider and narrower test
sets.

Make scripts/checkpatch.pl load the tests from the file, along with the
structured data, validate the references in MAINTAINERS, dereference
them, and output the test suite information in the CHECK messages
whenever the corresponding subsystems are changed. But only if there was
no corresponding Tested-with: tag in the commit message, certifying it
was executed successfully already.

This is supposed to help propose executing test suites which cannot be
executed immediately, and need extra setup, as well as provide a place
for extra documentation and information on directly-available suites.

Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
---
 Documentation/process/index.rst              |   1 +
 Documentation/process/submitting-patches.rst |  21 +++-
 Documentation/process/tests.rst              |  41 +++++++
 MAINTAINERS                                  |   9 +-
 scripts/checkpatch.pl                        | 122 +++++++++++++++++--
 5 files changed, 177 insertions(+), 17 deletions(-)
 create mode 100644 Documentation/process/tests.rst

diff --git a/Documentation/process/index.rst b/Documentation/process/index.rst
index a1daa309b58d0..3eda2e7432fdb 100644
--- a/Documentation/process/index.rst
+++ b/Documentation/process/index.rst
@@ -49,6 +49,7 @@ Other guides to the community that are of interest to most developers are:
    :maxdepth: 1
 
    changes
+   tests
    stable-api-nonsense
    management-style
    stable-kernel-rules
diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
index 2004df2ac1b39..45bd1a713ef33 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -233,27 +233,42 @@ Test your changes
 
 Test the patch to the best of your ability. Check the MAINTAINERS file for the
 subsystem(s) you are changing to see if there are any **V:** entries
-proposing particular test suite commands. E.g.::
+proposing particular test suites, either directly as commands, or via
+documentation references.
+
+Test suite references start with a ``*`` (similar to C pointer dereferencing),
+followed by the name of the test suite, which would be documented in the
+Documentation/process/tests.rst under the corresponding heading. E.g.::
+
+  V: *xfstests
+
+Anything not starting with a ``*`` is considered a command. E.g.::
 
   V: tools/testing/kunit/run_checks.py
 
 Supplying the ``--test`` option to ``scripts/get_maintainer.pl`` adds **V:**
 entries to its output.
 
-Execute the commands, if any, to test your changes.
+Execute the (referenced) test suites, 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.::
+<command>`` or ``Tested-with: *<reference>`` to the message of the commit you
+tested. E.g.::
 
   Tested-with: tools/testing/kunit/run_checks.py
 
+or::
+
+  Tested-with: *xfstests
+
 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
+  Tested-with: *xfstests # https://kernelci.org/test/2239324
 
 
 Select the recipients for your patch
diff --git a/Documentation/process/tests.rst b/Documentation/process/tests.rst
new file mode 100644
index 0000000000000..4ae5000e811c8
--- /dev/null
+++ b/Documentation/process/tests.rst
@@ -0,0 +1,41 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+.. _tests:
+
+Tests you can run
+=================
+
+There are many automated tests available for the Linux kernel, and some
+userspace tests which happen to also test the kernel. Here are some of them,
+along with the instructions on where to get them and how to run them for
+various purposes.
+
+This document has to follow a certain structure to allow tool access.
+Second-level headers (underscored with dashes '-') must contain test suite
+names, and the corresponding section must contain the test description.
+
+The test suites can be referenced by name, preceded with a '*', in the "V:"
+lines in the MAINTAINERS file, as well as in the "Tested-with:" tag in commit
+messages. E.g::
+
+  V: *xfstests
+
+and::
+
+  Tested-with: *xfstests
+
+Additionally, test suite names cannot contain '@' or '#' characters, the same
+as "V:" entries.
+
+The test suite description should contain the test documentation in general:
+where to get the test, how to run it, and how to interpret its results, but
+could also start with a "field list", containing single-line entries, with the
+following ones recognized by the tools (regardless of the case):
+
+:Summary: a single-line summary of the test suite (singular, non-capitalized)
+:Superset: the name of the test suite this one is a subset of
+:Command: a shell command executing the test suite, not requiring setup
+          beyond the kernel tree and regular developer environment
+          (even if only to report what else needs setting up)
+
+Any other entries are accepted, but not processed.
diff --git a/MAINTAINERS b/MAINTAINERS
index 28fbb0eb335ba..3ed15d8327919 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -60,11 +60,14 @@ Descriptions of section entries and preferred order
 	      printk, pr_info or pr_err
 	   One regex pattern per line.  Multiple K: lines acceptable.
 	V: *Test suite* proposed for execution. The command that could be
-	   executed to verify changes to the maintained subsystem.
-	   Must be executed from the root of the source tree.
-	   Must support the -h/--help option.
+	   executed to verify changes to the maintained subsystem, or a reference
+	   to a test suite documented in Documentation/process/tests.txt.
+	   Commands must be executed from the root of the source tree.
+	   Commands must support the -h/--help option.
+	   References must be preceded with a '*'.
 	   Cannot contain '@' or '#' characters.
 	   V: tools/testing/kunit/run_checks.py
+	   V: *xfstests
 	   One test suite per line.
 
 Maintainers List
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 1da617e1edb5f..bfeb4c33b5424 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -66,6 +66,9 @@ my $codespellfile = "/usr/share/codespell/dictionary.txt";
 my $user_codespellfile = "";
 my $conststructsfile = "$D/const_structs.checkpatch";
 my $docsfile = "$D/../Documentation/dev-tools/checkpatch.rst";
+my $testsrelfile = "Documentation/process/tests.rst";
+my $testsfile = "$D/../$testsrelfile";
+my %tests = ();
 my $typedefsfile;
 my $color = "auto";
 my $allow_c99_comments = 1; # Can be overridden by --ignore C99_COMMENT_TOLERANCE
@@ -282,6 +285,39 @@ sub load_docs {
 	close($docs);
 }
 
+sub load_tests {
+	open(my $tests, '<', "$testsfile")
+	    or warn "$P: Can't read the tests file $testsfile $!\n";
+
+	my $name = undef;
+	my $prev_line = undef;
+	my $in_field_list = 0;
+
+	while (<$tests>) {
+		my $line = $_;
+		$line =~ s/\s+$//;
+
+		# If the previous line was a second-level header (test name)
+		if ($line =~ /^-+$/ &&
+		    defined($prev_line) &&
+		    length($line) == length($prev_line)) {
+			$name = $prev_line;
+			$tests{$name} = {};
+			$in_field_list = 1;
+		# Else, if we're parsing the test's header field list
+		} elsif ($in_field_list) {
+			if ($line =~ /^:([^:]+):\s+(.*)/) {
+				$tests{$name}{lc($1)} = $2;
+			} else {
+				$in_field_list = !$line;
+			}
+		}
+
+		$prev_line = $line;
+	}
+	close($tests);
+}
+
 # Perl's Getopt::Long allows options to take optional arguments after a space.
 # Prevent --color by itself from consuming other arguments
 foreach (@ARGV) {
@@ -372,6 +408,7 @@ if ($color =~ /^[01]$/) {
 
 load_docs() if ($verbose);
 list_types(0) if ($list_types);
+load_tests();
 
 $fix = 1 if ($fix_inplace);
 $check_orig = $check;
@@ -1160,10 +1197,22 @@ sub get_file_proposed_tests {
 		my $output = `$command 2>/dev/null`;
 		# But regenerate stderr on failure
 		die "Failed retrieving tests proposed for changes to \"$filename\":\n" . `$command 2>&1 >/dev/null` if ($?);
-		$files_proposed_tests{$filename} = [grep { !/@/ } split("\n", $output)]
+		$file_proposed_tests = [grep { !/@/ } split("\n", $output)];
+		# Validate and normalize all references
+		for my $index (0 .. scalar @$file_proposed_tests - 1) {
+			my $test = $file_proposed_tests->[$index];
+			if ($test =~ /^\*\s*(.*)$/) {
+				my $name = $1;
+				die "Test $name referenced in MAINTAINERS not found in $testsrelfile\n"
+					if (!exists $tests{$name});
+				$file_proposed_tests->[$index] = "*" . $name;
+			}
+		}
+		$files_proposed_tests{$filename} = $file_proposed_tests;
+	} else {
+		$file_proposed_tests = $files_proposed_tests{$filename};
 	}
 
-	$file_proposed_tests = $files_proposed_tests{$filename};
 	return @$file_proposed_tests;
 }
 
@@ -2936,11 +2985,33 @@ sub process {
 			# Check if tests are proposed for changes to the file
 			foreach my $test (get_file_proposed_tests($realfile)) {
 				next if exists $dont_propose_tests{$test};
-				CHK("TEST_PROPOSAL",
-				    "Running the following test suite is proposed for changes to $realfile:\n" .
-				    "$test\n" .
-				    "Add the following to the tested commit's message, IF IT PASSES:\n" .
-				    "Tested-with: $test\n");
+				my $name;
+				my $title;
+				my $command;
+				my $message;
+				# If this is a reference to a documented test suite
+				if ($test =~ /^\*\s*(.*)/) {
+					$name = $1;
+					$title = $tests{$name}{"summary"} // "$name test suite";
+					$command = $tests{$name}{"command"};
+				# Else it's a test command
+				} else {
+					$title = "test suite";
+					$command = $test;
+				}
+				if ($command) {
+					$message = "Execute the $title " .
+						"proposed for verifying changes to $realfile:\n" .
+						"$command\n";
+				} else {
+					$message = "The $title is proposed for verifying changes to $realfile\n";
+				}
+				if ($name) {
+					$message .= "See instructions under \"$name\" in $testsrelfile\n";
+				}
+				$message .= "Add the following to the tested commit's message, " .
+					"IF IT PASSES:\nTested-with: $test\n";
+				CHK("TEST_PROPOSAL", $message);
 				$dont_propose_tests{$test} = 1;
 			}
 
@@ -3272,8 +3343,28 @@ sub process {
 
 # Check and accumulate executed test suites (stripping URLs off the end)
 		if (!$in_commit_log && $line =~ /^\s*Tested-with:\s*(.*?)\s*#.*$/i) {
-			# Do not propose this certified-passing test suite
-			$dont_propose_tests{$1} = 1;
+			my $test = $1;
+			# If the test is a reference
+			if ($test =~ /^\*\s*(.*)$/) {
+				# Do not propose (normalized references to)
+				# the test and its subsets
+				local *dont_propose_test_name = sub {
+					my ($name) = @_;
+					$dont_propose_tests{"*" . $name} = 1;
+					foreach my $sub_name (keys %tests) {
+						my $sub_data = $tests{$sub_name};
+						my $superset = $sub_data->{"superset"};
+						if (defined($superset) and $superset eq $name) {
+							dont_propose_test($sub_name);
+						}
+					}
+				};
+				dont_propose_test_name($1);
+			# Else it's a command
+			} else {
+				# Do not propose the test
+				$dont_propose_tests{$test} = 1;
+			}
 		}
 
 # Check email subject for common tools that don't need to be mentioned
@@ -3728,8 +3819,17 @@ sub process {
 			}
 # check MAINTAINERS V: entries are valid
 			if ($rawline =~ /^\+V:\s*(.*)/) {
-				my $name = $1;
-				if ($name =~ /[@#]/) {
+				my $entry = $1;
+				# If this is a valid entry value
+				if ($entry =~ /^[^@#]*$/) {
+					# If the test in the entry is a reference
+					if ($entry =~ /^\*\s*(.*)$/) {
+						my $name = $1;
+						ERROR("TEST_PROPOSAL_INVALID",
+						      "Test $name referenced in MAINTAINERS not found in $testsrelfile\n" .
+						      $herecurr) if (!exists $tests{$name});
+					}
+				} else {
 					ERROR("TEST_PROPOSAL_INVALID",
 					      "Test proposal cannot contain '\@' or '#' characters\n" . $herecurr);
 				}
-- 
2.42.0

[RFC PATCH v2 07/10] MAINTAINERS: Propose kvm-xfstests smoke for ext4

[Nikolai Kondrashov]

Propose the "kvm-xfstests smoke" test suite for changes to the EXT4 FILE
SYSTEM subsystem, as discussed previously with maintainers.

Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
---
 Documentation/process/tests.rst | 32 ++++++++++++++++++++++++++++++++
 MAINTAINERS                     |  1 +
 2 files changed, 33 insertions(+)

diff --git a/Documentation/process/tests.rst b/Documentation/process/tests.rst
index 4ae5000e811c8..cfaf937dc4d5f 100644
--- a/Documentation/process/tests.rst
+++ b/Documentation/process/tests.rst
@@ -39,3 +39,35 @@ following ones recognized by the tools (regardless of the case):
           (even if only to report what else needs setting up)
 
 Any other entries are accepted, but not processed.
+
+xfstests
+--------
+
+:Summary: file system regression test suite
+:Source: https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git
+:Docs: https://github.com/tytso/xfstests-bld/blob/master/Documentation/what-is-xfstests.md
+
+As the name might imply, xfstests is a file system regression test suite which
+was originally developed by Silicon Graphics (SGI) for the XFS file system.
+Originally, xfstests, like XFS was only supported on the SGI's Irix operating
+system. When XFS was ported to Linux, so was xfstests, and now xfstests is
+only supported on Linux.
+
+Today, xfstests is used as a file system regression test suite for all of
+Linux's major file systems: xfs, ext2, ext4, cifs, btrfs, f2fs, reiserfs, gfs,
+jfs, udf, nfs, and tmpfs. Many file system maintainers will run a full set of
+xfstests before sending patches to Linus, and will require that any major
+changes be tested using xfstests before they are submitted for integration.
+
+The easiest way to start running xfstests is under KVM with xfstests-bld:
+https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
+
+kvm-xfstests smoke
+------------------
+
+:Summary: file system smoke test suite
+:Superset: xfstests
+:Docs: https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
+
+The "kvm-xfstests smoke" is a minimal subset of xfstests for testing all major
+file systems, running under KVM.
diff --git a/MAINTAINERS b/MAINTAINERS
index 3ed15d8327919..669b5ff571730 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7978,6 +7978,7 @@ L:	linux-ext4@vger.kernel.org
 S:	Maintained
 W:	http://ext4.wiki.kernel.org
 Q:	http://patchwork.ozlabs.org/project/linux-ext4/list/
+V:	*kvm-xfstests smoke
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git
 F:	Documentation/filesystems/ext4/
 F:	fs/ext4/
-- 
2.42.0

[RFC PATCH v2 08/10] docs: tests: Document kunit in general

[Nikolai Kondrashov]

Add an entry on the complete set of kunit tests to the
Documentation/process/tests.rst, so that it could be referenced in
MAINTAINERS, and is catalogued in general.

Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
---
 Documentation/process/tests.rst | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/Documentation/process/tests.rst b/Documentation/process/tests.rst
index cfaf937dc4d5f..0760229fc32b0 100644
--- a/Documentation/process/tests.rst
+++ b/Documentation/process/tests.rst
@@ -71,3 +71,26 @@ kvm-xfstests smoke
 
 The "kvm-xfstests smoke" is a minimal subset of xfstests for testing all major
 file systems, running under KVM.
+
+kunit
+-----
+
+:Summary: complete set of KUnit unit tests
+:Command: tools/testing/kunit/kunit.py run --alltests
+:Docs: https://docs.kernel.org/dev-tools/kunit/
+
+KUnit tests are part of the kernel, written in the C (programming) language,
+and test parts of the Kernel implementation (example: a C language function).
+Excluding build time, from invocation to completion, KUnit can run around 100
+tests in less than 10 seconds. KUnit can test any kernel component, for
+example: file system, system calls, memory management, device drivers and so
+on.
+
+KUnit follows the white-box testing approach. The test has access to internal
+system functionality. KUnit runs in kernel space and is not restricted to
+things exposed to user-space.
+
+In addition, KUnit has kunit_tool, a script (tools/testing/kunit/kunit.py)
+that configures the Linux kernel, runs KUnit tests under QEMU or UML (User
+Mode Linux), parses the test results and displays them in a user friendly
+manner.
-- 
2.42.0

[RFC PATCH v2 09/10] MAINTAINERS: Propose kunit tests for regmap

[Nikolai Kondrashov]

From: Mark Brown <broonie@kernel.org>

The regmap core and especially cache code have reasonable kunit
coverage, ask people to use that to test regmap changes.

Signed-off-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 669b5ff571730..84e90ec015090 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18367,6 +18367,7 @@ REGISTER MAP ABSTRACTION
 M:	Mark Brown <broonie@kernel.org>
 L:	linux-kernel@vger.kernel.org
 S:	Supported
+V:	*kunit
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git
 F:	Documentation/devicetree/bindings/regmap/
 F:	drivers/base/regmap/
-- 
2.42.0

[RFC PATCH v2 10/10] MAINTAINERS: Add proposal strength to V: entries

[Nikolai Kondrashov]

Require the MAINTAINERS V: entries to begin with a keyword, one of
SUGGESTED/RECOMMENDED/REQUIRED, signifying how strongly the test is
proposed for verifying the subsystem changes, prompting
scripts/checkpatch.pl to produce CHECK/WARNING/ERROR messages
respectively, whenever the commit message doesn't have the corresponding
Tested-with: tag.

Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
---
 Documentation/process/submitting-patches.rst | 11 ++-
 MAINTAINERS                                  | 20 +++--
 scripts/checkpatch.pl                        | 83 ++++++++++++--------
 3 files changed, 71 insertions(+), 43 deletions(-)

diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
index 45bd1a713ef33..199fadc50cf62 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -233,18 +233,21 @@ Test your changes
 
 Test the patch to the best of your ability. Check the MAINTAINERS file for the
 subsystem(s) you are changing to see if there are any **V:** entries
-proposing particular test suites, either directly as commands, or via
-documentation references.
+proposing particular test suites.
+
+The **V:** entries start with a proposal strength keyword
+(SUGGESTED/RECOMMENDED/REQUIRED), followed either by a command, or a
+documentation reference.
 
 Test suite references start with a ``*`` (similar to C pointer dereferencing),
 followed by the name of the test suite, which would be documented in the
 Documentation/process/tests.rst under the corresponding heading. E.g.::
 
-  V: *xfstests
+  V: SUGGESTED *xfstests
 
 Anything not starting with a ``*`` is considered a command. E.g.::
 
-  V: tools/testing/kunit/run_checks.py
+  V: RECOMMENDED tools/testing/kunit/run_checks.py
 
 Supplying the ``--test`` option to ``scripts/get_maintainer.pl`` adds **V:**
 entries to its output.
diff --git a/MAINTAINERS b/MAINTAINERS
index 84e90ec015090..3a35e320b5a5b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -59,15 +59,19 @@ Descriptions of section entries and preferred order
 	      matches patches or files that contain one or more of the words
 	      printk, pr_info or pr_err
 	   One regex pattern per line.  Multiple K: lines acceptable.
-	V: *Test suite* proposed for execution. The command that could be
-	   executed to verify changes to the maintained subsystem, or a reference
-	   to a test suite documented in Documentation/process/tests.txt.
+	V: *Test suite* proposed for execution for verifying changes to the
+	   maintained subsystem. Must start with a proposal strength keyword:
+	   (SUGGESTED/RECOMMENDED/REQUIRED), followed by the test suite command,
+	   or a reference to a test suite documented in
+	   Documentation/process/tests.txt.
+	   Proposal strengths correspond to checkpatch.pl message levels
+	   (CHECK/WARNING/ERROR respectively, whenever Tested-with: is missing).
 	   Commands must be executed from the root of the source tree.
 	   Commands must support the -h/--help option.
 	   References must be preceded with a '*'.
 	   Cannot contain '@' or '#' characters.
-	   V: tools/testing/kunit/run_checks.py
-	   V: *xfstests
+	   V: SUGGESTED tools/testing/kunit/run_checks.py
+	   V: RECOMMENDED *xfstests
 	   One test suite per line.
 
 Maintainers List
@@ -7978,7 +7982,7 @@ L:	linux-ext4@vger.kernel.org
 S:	Maintained
 W:	http://ext4.wiki.kernel.org
 Q:	http://patchwork.ozlabs.org/project/linux-ext4/list/
-V:	*kvm-xfstests smoke
+V:	RECOMMENDED *kvm-xfstests smoke
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git
 F:	Documentation/filesystems/ext4/
 F:	fs/ext4/
@@ -11628,7 +11632,7 @@ L:	linux-kselftest@vger.kernel.org
 L:	kunit-dev@googlegroups.com
 S:	Maintained
 W:	https://google.github.io/kunit-docs/third_party/kernel/docs/
-V:	tools/testing/kunit/run_checks.py
+V:	RECOMMENDED tools/testing/kunit/run_checks.py
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git kunit
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git kunit-fixes
 F:	Documentation/dev-tools/kunit/
@@ -18367,7 +18371,7 @@ REGISTER MAP ABSTRACTION
 M:	Mark Brown <broonie@kernel.org>
 L:	linux-kernel@vger.kernel.org
 S:	Supported
-V:	*kunit
+V:	RECOMMENDED *kunit
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git
 F:	Documentation/devicetree/bindings/regmap/
 F:	drivers/base/regmap/
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bfeb4c33b5424..9438e4f452a6c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1181,39 +1181,57 @@ sub is_maintained_obsolete {
 	return $maintained_status{$filename} =~ /obsolete/i;
 }
 
-# Test suites proposed per changed file
+# A list of test proposal strength keywords, weakest to strongest
+our @test_proposal_strengths = qw(suggested recommended required);
+# A regular expression string matching test proposal strength keywords
+my $test_proposal_strengths_res = join('|', @test_proposal_strengths);
+# A regular expression matching valid values of MAINTAINERS V: entries
+# Puts proposal strength into $1 and the test into $2
+my $test_proposal_entry_re = qr/^\s*($test_proposal_strengths_res)\s+([^@#]+?)\s*$/i;
+
+# A hashmap of changed files and references to hashmaps of test suites and the
+# strength (0-2) of their proposal
 our %files_proposed_tests = ();
 
-# Return a list of test suites proposed for execution for a particular file
+# Return a reference to a hashmap of test suites proposed for execution for a
+# particular file, and their proposal strengths - 0, 1, or 2 for
+# SUGGESTED/RECOMMENDED/REQUIRED respectively.
 sub get_file_proposed_tests {
 	my ($filename) = @_;
-	my $file_proposed_tests;
 
-	return () if (!$tree || !(-e "$root/scripts/get_maintainer.pl"));
+	return {} if (!$tree || !(-e "$root/scripts/get_maintainer.pl"));
 
 	if (!exists($files_proposed_tests{$filename})) {
+		# Retrieve and parse the entries
+		my %file_proposed_tests = ();
 		my $command = "perl $root/scripts/get_maintainer.pl --test --multiline --nogit --nogit-fallback -f $filename";
 		# Ignore warnings on stderr
 		my $output = `$command 2>/dev/null`;
 		# But regenerate stderr on failure
 		die "Failed retrieving tests proposed for changes to \"$filename\":\n" . `$command 2>&1 >/dev/null` if ($?);
-		$file_proposed_tests = [grep { !/@/ } split("\n", $output)];
-		# Validate and normalize all references
-		for my $index (0 .. scalar @$file_proposed_tests - 1) {
-			my $test = $file_proposed_tests->[$index];
-			if ($test =~ /^\*\s*(.*)$/) {
-				my $name = $1;
-				die "Test $name referenced in MAINTAINERS not found in $testsrelfile\n"
-					if (!exists $tests{$name});
-				$file_proposed_tests->[$index] = "*" . $name;
+		# For each non-email line (a V: entry)
+		foreach my $entry (grep { !/@/ } split("\n", $output)) {
+			# Extract the strength and the test
+			if ($entry =~ $test_proposal_entry_re) {
+				my $strength = grep { $test_proposal_strengths[$_] eq lc($1) }
+						0..$#test_proposal_strengths;
+				my $test = $2;
+				# Validate and normalize references
+				if ($test =~ /^\*\s*(.*)$/) {
+					my $name = $1;
+					die "Test $name referenced in MAINTAINERS not found in $testsrelfile\n"
+						if (!exists $tests{$name});
+					$test = "*" . $name;
+				}
+				$file_proposed_tests{$test} = $strength;
+			} else {
+				die "Invalid MAINTAINERS V: entry: $entry\n";
 			}
 		}
-		$files_proposed_tests{$filename} = $file_proposed_tests;
-	} else {
-		$file_proposed_tests = $files_proposed_tests{$filename};
+		$files_proposed_tests{$filename} = \%file_proposed_tests;
 	}
 
-	return @$file_proposed_tests;
+	return $files_proposed_tests{$filename};
 }
 
 sub is_SPDX_License_valid {
@@ -2761,7 +2779,7 @@ sub process {
 	my @setup_docs = ();
 	my $setup_docs = 0;
 
-	# Test suites which should not be proposed for execution
+	# Maximum strength (0-2) of test proposals to be ignored
 	my %dont_propose_tests = ();
 
 	my $camelcase_file_seeded = 0;
@@ -2983,8 +3001,10 @@ sub process {
 			}
 
 			# Check if tests are proposed for changes to the file
-			foreach my $test (get_file_proposed_tests($realfile)) {
-				next if exists $dont_propose_tests{$test};
+			my $file_proposed_tests = get_file_proposed_tests($realfile);
+			foreach my $test (keys %$file_proposed_tests) {
+				my $strength = $file_proposed_tests->{$test};
+				next if $strength <= ($dont_propose_tests{$test} // -1);
 				my $name;
 				my $title;
 				my $command;
@@ -3000,19 +3020,19 @@ sub process {
 					$command = $test;
 				}
 				if ($command) {
-					$message = "Execute the $title " .
-						"proposed for verifying changes to $realfile:\n" .
-						"$command\n";
+					$message = "Execute the $title $test_proposal_strengths[$strength] " .
+						"for verifying changes to $realfile:\n$command\n";
 				} else {
-					$message = "The $title is proposed for verifying changes to $realfile\n";
+					$message = "The $title is $test_proposal_strengths[$strength] " .
+						"for verifying changes to $realfile\n";
 				}
 				if ($name) {
 					$message .= "See instructions under \"$name\" in $testsrelfile\n";
 				}
 				$message .= "Add the following to the tested commit's message, " .
 					"IF IT PASSES:\nTested-with: $test\n";
-				CHK("TEST_PROPOSAL", $message);
-				$dont_propose_tests{$test} = 1;
+				(\&CHK, \&WARN, \&ERROR)[$strength]("TEST_PROPOSAL", $message);
+				$dont_propose_tests{$test} = $strength;
 			}
 
 			next;
@@ -3350,7 +3370,7 @@ sub process {
 				# the test and its subsets
 				local *dont_propose_test_name = sub {
 					my ($name) = @_;
-					$dont_propose_tests{"*" . $name} = 1;
+					$dont_propose_tests{"*" . $name} = 2;
 					foreach my $sub_name (keys %tests) {
 						my $sub_data = $tests{$sub_name};
 						my $superset = $sub_data->{"superset"};
@@ -3363,7 +3383,7 @@ sub process {
 			# Else it's a command
 			} else {
 				# Do not propose the test
-				$dont_propose_tests{$test} = 1;
+				$dont_propose_tests{$test} = 2;
 			}
 		}
 
@@ -3821,9 +3841,10 @@ sub process {
 			if ($rawline =~ /^\+V:\s*(.*)/) {
 				my $entry = $1;
 				# If this is a valid entry value
-				if ($entry =~ /^[^@#]*$/) {
+				if ($entry =~ $test_proposal_entry_re) {
+					my $test = $2;
 					# If the test in the entry is a reference
-					if ($entry =~ /^\*\s*(.*)$/) {
+					if ($test =~ /^\*\s*(.*)$/) {
 						my $name = $1;
 						ERROR("TEST_PROPOSAL_INVALID",
 						      "Test $name referenced in MAINTAINERS not found in $testsrelfile\n" .
@@ -3831,7 +3852,7 @@ sub process {
 					}
 				} else {
 					ERROR("TEST_PROPOSAL_INVALID",
-					      "Test proposal cannot contain '\@' or '#' characters\n" . $herecurr);
+					      "Invalid test proposal entry: $entry\n" . $herecurr);
 				}
 			}
 		}
-- 
2.42.0

Re: [RFC PATCH v2 01/10] get_maintainer: Survive querying missing files

[Joe Perches]

On Tue, 2023-12-05 at 20:02 +0200, Nikolai Kondrashov wrote:
> Do not die, but only warn when scripts/get_maintainer.pl is asked to
> retrieve information about a missing file.
> 
> This allows scripts/checkpatch.pl to query MAINTAINERS while processing
> patches which are removing files.

Why is this useful?

Give a for-instance example please.

Re: [RFC PATCH v2 02/10] MAINTAINERS: Introduce V: entry for tests

[Joe Perches]

On Tue, 2023-12-05 at 20:02 +0200, Nikolai Kondrashov wrote:
> Require the entry values to not contain the '@' character, so they could
> be distinguished from emails (always) output by get_maintainer.pl.

Why is this useful?
Why the need to distinguish?

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

[Jonathan Corbet]

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

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

[Joe Perches]

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.

Re: [RFC PATCH v2 06/10] MAINTAINERS: Support referencing test docs in V:

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 2675 bytes --]

On Wed, 6 Dec 2023 at 02:45, Nikolai Kondrashov
<Nikolai.Kondrashov@redhat.com> wrote:
>
> Support referencing test suite documentation in the V: entries of
> MAINTAINERS file. Use the '*<name>' syntax (like C pointer dereference),
> where '<name>' is a second-level heading in the new
> Documentation/process/tests.rst file, with the suite's description.
> This syntax allows distinguishing the references from test commands.
>
> Add a boiler-plate Documentation/process/tests.rst file, describing a
> way to add structured info to the test suites in the form of field
> lists. Apart from a "summary" and "command" fields, they can also
> contain a "superset" field specifying the superset of the test suite,
> helping reuse documentation and express both wider and narrower test
> sets.
>
> Make scripts/checkpatch.pl load the tests from the file, along with the
> structured data, validate the references in MAINTAINERS, dereference
> them, and output the test suite information in the CHECK messages
> whenever the corresponding subsystems are changed. But only if there was
> no corresponding Tested-with: tag in the commit message, certifying it
> was executed successfully already.
>
> This is supposed to help propose executing test suites which cannot be
> executed immediately, and need extra setup, as well as provide a place
> for extra documentation and information on directly-available suites.
>
> Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
> ---

I like the idea here, but wonder whether it makes sense to put all of
these tests into a single 'tests.rst' file. There's already lots of
existing documentation scattered around the tree, and while keeping
all of the testing information in one place does have advantages, I
think there's a lot to be said for keeping subsystem-specific test
docs alongside the rest of the documentation for the subsystem itself.
And it'd be less work, as the docs are already there.

So, could we just make this a path under Documentation/ (possibly with
an #anchor if we need to reference just one part of a file)?

e.g., something like these, all of which are existing docs:
V: *Documentation/dev-tools/kasan.rst#Tests
or
V: *Dcoumentation/RCU/torture.rst
or
V: *Documentation/gpu/automated_testing.rst
or
V: *Documentation/process/maintainer-kvm-x86.rst#Testing

(We could even get rid of the '*' and just use 'Documentation/' as a
prefix, or the executable bit on the file, or similar to distinguish
these from scripts.)

If we wanted to be very brave, we could extend this further to
arbitrary webpages, like:
V: https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/tree/README

Thoughts?

-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [RFC PATCH v2 02/10] MAINTAINERS: Introduce V: entry for tests

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 1417 bytes --]

On Wed, 6 Dec 2023 at 02:45, Nikolai Kondrashov
<Nikolai.Kondrashov@redhat.com> wrote:
>
> Introduce a new 'V:' ("Verify") entry to MAINTAINERS. The entry accepts
> a test suite command which is proposed to be executed for each
> contribution to the subsystem.
>
> Extend scripts/get_maintainer.pl to support retrieving the V: entries
> when '--test' option is specified.
>
> Require the entry values to not contain the '@' character, so they could
> be distinguished from emails (always) output by get_maintainer.pl. Make
> scripts/checkpatch.pl check that they don't.
>
> Update entry ordering in both scripts/checkpatch.pl and
> scripts/parse-maintainers.pl.
>
> Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
> ---

I'm pretty happy with this personally, though I definitely think we
need the support for tests which aren't just executable scripts (e.g.
the docs in patch 6).

The get_maintailer.pl bits, and hence the requirement to not include
'@', feel a little bit 'off': I'd rather get_maintainer.pl kept emails
and tests separate by some other means (either having --test _only_
print tests, not emails at all, or by giving them a prefix like
'TEST:' or something). But that is diverging more from the existing
behaviour of get_maintainer.pl, so I could go either way.

Otherwise, this looks pretty good. I'll give it a proper test tomorrow
alongside the other patches.

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

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

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC PATCH v2 01/10] get_maintainer: Survive querying missing files

[Nikolai Kondrashov]

Hi Joe,

On 12/5/23 20:55, Joe Perches wrote:
> On Tue, 2023-12-05 at 20:02 +0200, Nikolai Kondrashov wrote:
>> Do not die, but only warn when scripts/get_maintainer.pl is asked to
>> retrieve information about a missing file.
>>
>> This allows scripts/checkpatch.pl to query MAINTAINERS while processing
>> patches which are removing files.
> 
> Why is this useful?
> 
> Give a for-instance example please.

Sure! Take the ebb0130dad751e88c28ab94c71e46e8ee65427c9 commit for example.

It removes a file (and recreates it in another format, but that's besides the 
point) which belongs to four subsystems.

This will work OK:

scripts/get_maintainer.pl 
0001-dt-bindings-mailbox-convert-bcm2835-mbox-bindings-to.patch

But if we try to give the file being removed to get_maintainer.pl directly:

scripts/get_maintainer.pl -f 
Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt

It will abort with the following message:

scripts/get_maintainer.pl: file 
'Documentation/devicetree/bindings/mailbox/brcm,bcm2835-mbox.txt' not found

Even though the state of the source tree we're running this on is *exactly* 
the same.

The latter (using the -f option) is the way checkpatch.pl works to fetch the 
maintenance status (in is_maintained_obsolete()), and the way I implemented 
fetching the tests from MAINTAINERS (in get_file_proposed_tests()).

This way seems to work better for checkpatch.pl, I suppose, because it can 
link the error message to a specific file. But in principle, it might be 
possible to just call get_maintainer.pl on every patch, if we really have to.

However, I feel that conceptually it should be possible to query MAINTAINERS 
without actual *source* files being present in the tree (as opposed to patch 
files which it needs to read), or even the tree being there at all.

I saw that we are printing a warning if the queried file is not there in the 
git repo (I guess it's helpful?), and thought perhaps we can do the same 
without the git repo too.

Hope that helps!
Nick

Re: [RFC PATCH v2 02/10] MAINTAINERS: Introduce V: entry for tests

[Nikolai Kondrashov]

On 12/5/23 20:58, Joe Perches wrote:
> On Tue, 2023-12-05 at 20:02 +0200, Nikolai Kondrashov wrote:
>> Require the entry values to not contain the '@' character, so they could
>> be distinguished from emails (always) output by get_maintainer.pl.
> 
> Why is this useful?
> Why the need to distinguish?

This is because get_maintainer.pl seems to insist on *always* outputting 
emails. I guess that was its sole original purpose, and it stuck to it? It 
kinda makes sense to me, especially given the name of the script, but at the 
same time, as a consequence you can't query *only* the tests (or anything but 
emails, really).

Therefore we have to be able to somehow filter out the emails from the 
get_maintainer.pl output, to get only the tests. Email addresses *have* to 
have the '@' character (seems to be the only reliable thing about them :D), so 
naturally I chose that as the way to distinguish them from the tests.

It's ugly, but considering the get_maintainer.pl legacy, it's good enough.

I don't mind changing get_maintainer.pl, though, to stop it from outputting 
emails (e.g. given an option), if that works for everyone involved.

Nick

Re: [RFC PATCH v2 02/10] MAINTAINERS: Introduce V: entry for tests

[Nikolai Kondrashov]

On 12/6/23 10:12, David Gow wrote:
> I'm pretty happy with this personally, though I definitely think we
> need the support for tests which aren't just executable scripts (e.g.
> the docs in patch 6).
> 
> The get_maintailer.pl bits, and hence the requirement to not include
> '@', feel a little bit 'off': I'd rather get_maintainer.pl kept emails
> and tests separate by some other means (either having --test _only_
> print tests, not emails at all, or by giving them a prefix like
> 'TEST:' or something). But that is diverging more from the existing
> behaviour of get_maintainer.pl, so I could go either way.
> 
> Otherwise, this looks pretty good. I'll give it a proper test tomorrow
> alongside the other patches.

Thanks for the review, David!

Yeah, I don't like the '@' bit myself, but it seems to be the path of least 
resistance right now (not necessarily the best one, of course).

I'm up for adding an option to get_maintainer.pl that disables email output, 
if people like that, though.

Nick

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

[Nikolai Kondrashov]

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

Re: [RFC PATCH v2 02/10] MAINTAINERS: Introduce V: entry for tests

[Joe Perches]

On Wed, 2023-12-06 at 18:23 +0200, Nikolai Kondrashov wrote:
> On 12/6/23 10:12, David Gow wrote:
> > I'm pretty happy with this personally, though I definitely think we
> > need the support for tests which aren't just executable scripts (e.g.
> > the docs in patch 6).
> > 
> > The get_maintailer.pl bits, and hence the requirement to not include
> > '@', feel a little bit 'off': I'd rather get_maintainer.pl kept emails
> > and tests separate by some other means (either having --test _only_
> > print tests, not emails at all, or by giving them a prefix like
> > 'TEST:' or something). But that is diverging more from the existing
> > behaviour of get_maintainer.pl, so I could go either way.
> > 
> > Otherwise, this looks pretty good. I'll give it a proper test tomorrow
> > alongside the other patches.
> 
> Thanks for the review, David!
> 
> Yeah, I don't like the '@' bit myself, but it seems to be the path of least 
> resistance right now (not necessarily the best one, of course).
> 
> I'm up for adding an option to get_maintainer.pl that disables email output, 
> if people like that, though.

That already exists though I don't understand the
specific requirement here

--nom --nol --nor

from
$ ./scripts/get_maintainer.pl --help
[]
    --m => include maintainer(s) if any
    --r => include reviewer(s) if any
    --n => include name 'Full Name <addr@domain.tld>'
    --l => include list(s) if any
[]
   Most options have both positive and negative forms.
      The negative forms for --<foo> are --no<foo> and --no-<foo>.

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

[Nikolai Kondrashov]

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

Re: [RFC PATCH v2 06/10] MAINTAINERS: Support referencing test docs in V:

[Nikolai Kondrashov]

On 12/6/23 10:03, David Gow wrote:
> On Wed, 6 Dec 2023 at 02:45, Nikolai Kondrashov
> <Nikolai.Kondrashov@redhat.com> wrote:
>>
>> Support referencing test suite documentation in the V: entries of
>> MAINTAINERS file. Use the '*<name>' syntax (like C pointer dereference),
>> where '<name>' is a second-level heading in the new
>> Documentation/process/tests.rst file, with the suite's description.
>> This syntax allows distinguishing the references from test commands.
 >
> I like the idea here, but wonder whether it makes sense to put all of
> these tests into a single 'tests.rst' file. There's already lots of
> existing documentation scattered around the tree, and while keeping
> all of the testing information in one place does have advantages, I
> think there's a lot to be said for keeping subsystem-specific test
> docs alongside the rest of the documentation for the subsystem itself.
> And it'd be less work, as the docs are already there.
> 
> So, could we just make this a path under Documentation/ (possibly with
> an #anchor if we need to reference just one part of a file)?
> 
> e.g., something like these, all of which are existing docs:
> V: *Documentation/dev-tools/kasan.rst#Tests
> or
> V: *Dcoumentation/RCU/torture.rst
> or
> V: *Documentation/gpu/automated_testing.rst
> or
> V: *Documentation/process/maintainer-kvm-x86.rst#Testing
> 
> (We could even get rid of the '*' and just use 'Documentation/' as a
> prefix, or the executable bit on the file, or similar to distinguish
> these from scripts.)
> 
> If we wanted to be very brave, we could extend this further to
> arbitrary webpages, like:
> V: https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/tree/README

Sure, having a filename (in a specific directory) or a just piece of path in 
the source (sub)tree would work too. The idea of single file was mostly to 
make it easier to access a *catalog* of all tests in a single file with small 
bits of introductory documentation, pointing to the more detailed 
documentation (wherever you prefer it to be) from there.

URLs would work as well for pointing to the docs, but they become somewhat 
more cumbersome and error-prone for use in Tested-with: tags (if we would like 
them), just because of their length and complexity. If we won't care for that, 
it's not a problem.

However, overall, I would be cautious multiplying the ways tests can be 
specified in V: entries (and Tested-with: tags), as that could quickly become 
unwieldy and confusing for humans, who are expected to be interpreting and 
writing them. Especially if the syntax could potentially be ambiguous.

Nick

Re: [RFC PATCH v2 02/10] MAINTAINERS: Introduce V: entry for tests

[Nikolai Kondrashov]

On 12/6/23 18:38, Joe Perches wrote:
> On Wed, 2023-12-06 at 18:23 +0200, Nikolai Kondrashov wrote:
>> On 12/6/23 10:12, David Gow wrote:
>>> I'm pretty happy with this personally, though I definitely think we
>>> need the support for tests which aren't just executable scripts (e.g.
>>> the docs in patch 6).
>>>
>>> The get_maintailer.pl bits, and hence the requirement to not include
>>> '@', feel a little bit 'off': I'd rather get_maintainer.pl kept emails
>>> and tests separate by some other means (either having --test _only_
>>> print tests, not emails at all, or by giving them a prefix like
>>> 'TEST:' or something). But that is diverging more from the existing
>>> behaviour of get_maintainer.pl, so I could go either way.
>>>
>>> Otherwise, this looks pretty good. I'll give it a proper test tomorrow
>>> alongside the other patches.
>>
>> Thanks for the review, David!
>>
>> Yeah, I don't like the '@' bit myself, but it seems to be the path of least
>> resistance right now (not necessarily the best one, of course).
>>
>> I'm up for adding an option to get_maintainer.pl that disables email output,
>> if people like that, though.
> 
> That already exists though I don't understand the
> specific requirement here
> 
> --nom --nol --nor
> 
> from
> $ ./scripts/get_maintainer.pl --help
> []
>      --m => include maintainer(s) if any
>      --r => include reviewer(s) if any
>      --n => include name 'Full Name <addr@domain.tld>'
>      --l => include list(s) if any
> []
>     Most options have both positive and negative forms.
>        The negative forms for --<foo> are --no<foo> and --no-<foo>.
> 

Thanks, Joe!

Yeah, I already explored that way, but it seems to be explicitly forbidden:

$ scripts/get_maintainer.pl --nom --nol --nor 
0001-dt-bindings-mailbox-convert-bcm2835-mbox-bindings-to.patch
scripts/get_maintainer.pl: Please select at least 1 email option

So, I assumed there is a reason and an intention behind this behavior and went 
the other way.

Nick

Re: [RFC PATCH v2 00/10] MAINTAINERS: Introduce V: entry for tests

[Nikolai Kondrashov]

On 12/5/23 20:02, Nikolai Kondrashov wrote:
> Alright, here's a second version, attempting to address as many concerns as
> possible. It's likely I've missed something, though.

Happy New Year, everyone! Hope your holidays went smoothly and peacefully.

Is anyone still interested in this effort? Any more feedback you have in mind?
Something to change?

Thanks!
Nick

Re: [RFC PATCH v2 01/10] get_maintainer: Survive querying missing files

[Nikolai Kondrashov]

On 12/5/23 20:55, Joe Perches wrote:
> On Tue, 2023-12-05 at 20:02 +0200, Nikolai Kondrashov wrote:
>> Do not die, but only warn when scripts/get_maintainer.pl is asked to
>> retrieve information about a missing file.
>>
>> This allows scripts/checkpatch.pl to query MAINTAINERS while processing
>> patches which are removing files.
> 
> Why is this useful?
> 
> Give a for-instance example please.

Does the example I provided make sense, Joe?

Would my solution be adequate, or would you like to have another?

Thank you.
Nick