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

Daniel Latypov
2023-11-20T18:49:01Z

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