Re: [RFC v1 0/6] kunit: Add test attributes API

[David Gow]

On Sat, 10 Jun 2023 at 08:52, Rae Moar <rmoar@xxxxxxxxxx> wrote:
>
> Hello everyone,
>
> This is an RFC patch series to propose the addition of a test attributes
> framework to KUnit.
>
> There has been interest in filtering out "slow" KUnit tests. Most notably,
> a new config, CONFIG_MEMCPY_SLOW_KUNIT_TEST, has been added to exclude
> particularly slow memcpy tests
> (https://lore.kernel.org/all/20230118200653.give.574-kees@xxxxxxxxxx/).

Awesome: this is a long overdue feature.

> This proposed attributes framework would be used to save and access test
> associated data, including whether a test is slow. These attributes would
> be reportable (via KTAP and command line output) and some will be
> filterable.

Why wouldn't they all be filterable? I guess I can imagine some where
filtering wouldn't be useful, but I can't think of a technical reason
why the filter shouldn't work.

Also, as I understand it, I think this could also work with data which
is not "saved" in this kunit_attributes struct you define. So we could
have attributes which are generated automatically from other
information about the test.
 I could definitely see value in being able to filter on things like
"is_parameterised" or "runs_at_init" or similar.

Finally, it'd be really great if these attributes could apply to
individual parameters in parameterised tests, in which case we could
have and filter on the parameter value. That seems like it could be
incredibly useful.

>
> This framework is designed to allow for the addition of other attributes in
> the future. These attributes could include whether the test is flaky,
> associated test files, etc.

A small part of me is hesitant to add this much framework code for
only one attribute, so it'd be nice to look into at least having an
RFC for some of these. Hopefully we don't actually have flaky tests,
but "is_deterministic" would be nice (alongside a future ability to
inject a random seed, or similar). Other properties like
"is_threadsafe", "is_reentrant", etc could be useful for future
features. And I'm sure there could be some more subsystem-specific
things which would be useful to filter on, too.

Some of these could probably replace the need for custom code to make
the test skip itself if dependencies aren't met, too, which would be
fun.

I'm not sure "associated test files" quite gels perfectly with this
system as-is (assuming I understand what that refers to). If it's the
ability to "attach" extra data (logs, etc) to the KTAP output, that's
possibly best injected at runtime, or added by the separate parser,
rather than hardcoded in the kernel.

>
> Note that this could intersect with the discussions on how to format
> test-associated data in KTAP v2 that I am also involved in
> (https://lore.kernel.org/all/20230420205734.1288498-1-rmoar@xxxxxxxxxx/).
>
I definitely need to re-read and respond to that. I'm not 100%
thrilled with the output format here, and I think the goal with KTAP
"test associated data" is, as you say, related, but not identical to
this. There'd definitely be data which doesn't make sense as a KUnit
attribute which we might want to add to the output (e.g., data only
calcuated while the test runs), and attributes which we might not want
to always print out with the results.

> If the overall idea seems good, I'll make sure to add tests/documentation,
> and more patches marking existing tests as slow to the patch series.
>

I think the series is good overall. If no-one else objects, let's move
forward with it.
I'd definitely prefer to see a few more tests and some documentation.
Having another attribute would be great, too, though I can certainly
live with that being a separate series.


> Thanks!
> Rae
>
> Rae Moar (6):
>   kunit: Add test attributes API structure
>   kunit: Add speed attribute
>   kunit: Add ability to filter attributes
>   kunit: tool: Add command line interface to filter and report
>     attributes
>   kunit: memcpy: Mark tests as slow using test attributes
>   kunit: time: Mark test as slow using test attributes
>
>  include/kunit/attributes.h             |  41 ++++
>  include/kunit/test.h                   |  62 ++++++
>  kernel/time/time_test.c                |   2 +-
>  lib/kunit/Makefile                     |   3 +-
>  lib/kunit/attributes.c                 | 280 +++++++++++++++++++++++++
>  lib/kunit/executor.c                   |  89 ++++++--
>  lib/kunit/executor_test.c              |   8 +-
>  lib/kunit/kunit-example-test.c         |   9 +
>  lib/kunit/test.c                       |  17 +-
>  lib/memcpy_kunit.c                     |   8 +-
>  tools/testing/kunit/kunit.py           |  34 ++-
>  tools/testing/kunit/kunit_kernel.py    |   6 +-
>  tools/testing/kunit/kunit_tool_test.py |  41 ++--
>  13 files changed, 536 insertions(+), 64 deletions(-)
>  create mode 100644 include/kunit/attributes.h
>  create mode 100644 lib/kunit/attributes.c
>
>
> base-commit: fefdb43943c1a0d87e1b43ae4d03e5f9a1d058f4
> --
> 2.41.0.162.gfafddb0af9-goog
>

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature