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(-) ---
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));
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.
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?
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
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.
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
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
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*
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.
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
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
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.
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
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/)?
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
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.
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 :)
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
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
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
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.
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
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
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:
(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.
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
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%20...
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
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:
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.
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
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....
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
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:
- 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.
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....
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:
- 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.
- 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.
- 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
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/MAINTAINER... base: linus/master patch link: https://lore.kernel.org/r/20231115175146.9848-2-Nikolai.Kondrashov%40redhat.... patch subject: [PATCH 1/3] MAINTAINERS: Introduce V: field for required tests reproduce: (https://download.01.org/0day-ci/archive/20231122/202311220843.vh7WyxDF-lkp@i...)
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
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-xfst... + +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-quicksta... + +kvm-xfstests smoke +------------------ + +:Summary: File system smoke tests +:Superset: xfstests +:Docs: https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quicksta... + +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/
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-xfst...
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-quicksta...
+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-quicksta...
+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
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-xfst...
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:
Alright. Changed to https://git.kernel.org/pub/scm/fs/ext2/xfstests-bld.git/tree/Documentation/k...
And changed the kvm-xfstests docs link to https://git.kernel.org/pub/scm/fs/ext2/xfstests-bld.git/tree/Documentation/k...
+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-quicksta...
+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
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-xfst...
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-quicksta...
+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-quicksta...
+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
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-quicksta...
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-quicksta...
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
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-quicksta...
...
(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
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-quicksta...
...
(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
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
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-quicksta...
...
(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.
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/
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
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
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(-) ---
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))) {
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.
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
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
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));
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?
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
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
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
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>.
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
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/
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); } } }
Nikolai Kondrashov Nikolai.Kondrashov@redhat.com writes:
Introduce a new tag, 'Tested-with:', documented in the Documentation/process/submitting-patches.rst file.
The tag is expected to contain the test suite command which was executed for the commit, and to certify it passed. Additionally, it can contain a URL pointing to the execution results, after a '#' character.
Prohibit the V: field from containing the '#' character correspondingly.
Signed-off-by: Nikolai Kondrashov Nikolai.Kondrashov@redhat.com
Documentation/process/submitting-patches.rst | 10 ++++++++++ MAINTAINERS | 2 +- scripts/checkpatch.pl | 4 ++-- 3 files changed, 13 insertions(+), 3 deletions(-)
I have to ask whether we *really* need to introduce yet another tag for this. How are we going to use this information? Are we going to try to make a tag for every way in which somebody might test a patch?
Thanks,
jon
On Tue, 2023-12-05 at 11:59 -0700, Jonathan Corbet wrote:
Nikolai Kondrashov Nikolai.Kondrashov@redhat.com writes:
Introduce a new tag, 'Tested-with:', documented in the Documentation/process/submitting-patches.rst file.
[]
I have to ask whether we *really* need to introduce yet another tag for this. How are we going to use this information? Are we going to try to make a tag for every way in which somebody might test a patch?
In general, I think Link: <to some url test result> would be good enough.
And remember that all this goes stale after awhile and that includes old test suites.
On Tue, Dec 5, 2023 at 8:07 PM Joe Perches joe@perches.com wrote:
On Tue, 2023-12-05 at 11:59 -0700, Jonathan Corbet wrote:
Nikolai Kondrashov Nikolai.Kondrashov@redhat.com writes:
Introduce a new tag, 'Tested-with:', documented in the Documentation/process/submitting-patches.rst file.
[]
I have to ask whether we *really* need to introduce yet another tag for this. How are we going to use this information? Are we going to try to make a tag for every way in which somebody might test a patch?
In general, I think Link: <to some url test result> would be good enough.
Exactly. And if you put the test results (or a link) in your patch below the "---", or in your cover letter, the "Link:" tag pointing to lore (or something else, unfortunately) that most (but unfortunately not all) maintainers already add when committing patches allows anyone to find it.
And remember that all this goes stale after awhile and that includes old test suites.
Yeah...
Isn't the purpose of a "Tested-with:" tag just for the maintainer to know which patches have been tested with the test suite already, and which haven't? I expect reviewers/maintainers to scrutinize (extra) patches that lack such a tag (or lack the same under the "---"), and/or run the test suite theirselves. I.e. does this serve any purpose _after_ the patch has been applied?
Gr{oetje,eeting}s,
Geert
On 12/5/23 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
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
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) {
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); }
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
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
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-xfst... + +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-quicksta... + +kvm-xfstests smoke +------------------ + +:Summary: file system smoke test suite +:Superset: xfstests +:Docs: https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quicksta... + +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/
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.
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/
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); } } }
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
linux-kselftest-mirror@lists.linaro.org
-
Bagas Sanjaya -
Chandan Babu R -
Daniel Latypov -
Darrick J. Wong -
Dave Chinner -
David Gow -
Geert Uytterhoeven -
Greg Kroah-Hartman -
Gustavo Padovan -
Joe Perches -
Jonathan Corbet -
kernel test robot -
Konstantin Ryabitsev -
Mark Brown -
Nikolai Kondrashov -
Ricardo Cañuelo -
Theodore Ts'o