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

Nikolai Kondrashov
2023-11-22T17:38:20Z

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