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

[Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>]

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