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