On 4/4/25 07:53, David Gow wrote:
On Fri, 4 Apr 2025 at 01:48, Mark Brown broonie@kernel.org wrote:
On Thu, Apr 03, 2025 at 05:57:51PM +0100, Richard Fitzgerald wrote:
On 3/4/25 14:26, Mark Brown wrote:
I've not heard anyone mention hitting the timeouts, though now I run interactively I do see that the Cirrus stuff is a good proportion of the runtime for --alltests. We could potentially add a config option, though it'd need to end up in the config file so it gets turned on and we'd still run into any timeout issues. There's a tension with people expecting --alltests to actually run all the tests as well...
I don't want to get into the situation where nobody outside of Cirrus is running the tests. One of the main points of converting our tests to kunit was the hope that more people would run them on more configurations.
Yeah, I definitely agree that this should be enabled by --alltests regardless: we possibly need to adjust the default timeout for --alltests compared to without --alltests.
Ultimately, this is about balancing a few different usecases:
- I want to check nothing relevant to my config is broken (enable
CONFIG_KUNIT_ALL_TESTS on top of the existing config)
- I want to test everything I can in my tree / CI system / etc (run
with --alltests, possibly several times across different architectures, or with KASAN, etc)
- I'm developing something specific, and want to run all of the tests
related to that (use a subsystem/driver/feature specific .kunitconfig)
- I want to quickly check if my source tree builds, boots, and is
vaguely sane (run the "default" set of tests)
I don't mind reorganizing the cs_dsp test into smoke and thorough.
Although the feedback I've seen (such as it is) is not about the time it takes but the fact it flagged an error. "This test fails - disable it" isn't helped if it's the quick smoke test which failed.
It's reasonable that people are annoyed if the test fails because of a bug in the test. It seems strange to disable the test because it fails on a real bug in the code it is testing (which it did).
- I want to run tests on my ancient/slow machine/emulator without
waiting all day (run whichever config makes sense, but filtering out "slow" tests).
Now, my gut feeling is that we definitely want to run all of these tests for the --alltests case, and also for the "developing the Cirrus Logic driver" case, but that only a subset of them need to run in the "does my tree look vaguely sane" case, and there's limited (but not zero) use in running them in the "I'm testing my config which doesn't have any of the Cirrus Logic drivers enabled" case.
If that's the case, I think the ideal solution is to:
- Make sure these tests don't automatically enable themselves if no
driver which depends on the firmware loader is enabled. (e.g. make it depend on the library, rather than selecting it. If there's an extra "dummy" option which force-enables it, that's fine, but that shouldn't be enabled if KUNIT_ALL_TESTS is)
I thought someone had already patched that but apparently not. I'm not the only person who thought "ALL_TESTS" meant "all tests", not "some tests". Given that ALL_TESTS is badly named and really means "All tests for modules that are already selected" then it should only build if cs_dsp is specifically selected.
However the users of cs_dsp can have a lot of dependencies that are entirely irrelevant to cs_dsp and so cs_dsp *could* be tested without those. (This also applies to other Cirrus KUnit tests.) I started on some patches to make the Kconfig for the tested libraries visible if CONFIG_KUNIT is enabled so that they can be selected for testing without the need to enable a large bunch of other frameworks just to be able to enable a user of the library. They are somewhere in my to-do pile.
Related to this I created a UM kunit.py config specifically to run all Cirrus KUnit tests with minimum other clutter. But does that mean nobody outside Cirrus will run it? Or is there a process somewhere that runs kunit.py on all configs in tools/testing/kunit/configs ?
- As a result, make them more explicitly enabled with --alltests, and
probably disabled -- or only run a subset -- in the default. Currently this is mostly decided by whether CONFIG_REGMAP is enabled, having a specific item to use for these tests would be less surprising.
- If any of the individual tests are particularly slow (more than a
~second or so on fast hardware), mark them as slow. Most people still enable slow tests, so they'll still get run most of the time, but they can be skipped on old m68k machines, or gated behind the quick tests passing in CI systems, etc)
I don't expect any individual test cases are slow (I'll check through). One of the advantages of parameterizing them was that it avoids one single test case taking a long time looping through a lot of testing. They should be a quick write-read-check-exit. "Slowness" is tricky to judge. The slowest hardware I have is an ARM7 clocked at a few hundred MHz, or use QEMU in full emulation.
- Bump up the default timeout ( at least for --alltests), as 5 minutes
clearly isn't enough for, e.g., qemu-based emulation anymore. (I'm happy to do this: I've got some timeout-related patches I'm working on anyway.)
Is this a default for each test case or the entire suite?
Does that seem like a sensible approach?
Cheers, -- David