| Message ID | 20231115175146.9848-4-Nikolai.Kondrashov@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | MAINTAINERS: Introduce V: field for required tests | expand |
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
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/
Signed-off-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com> --- Documentation/process/tests.rst | 13 +++++++++++++ MAINTAINERS | 1 + 2 files changed, 14 insertions(+)