Re: [PATCH] Documentation: kunit: Add naming guidelines

[David Gow]

On Tue, Sep 1, 2020 at 7:47 AM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> On Fri, Aug 28, 2020 at 12:17:05AM +0800, David Gow wrote:
> > On Thu, Aug 27, 2020 at 9:14 PM Marco Elver <elver@xxxxxxxxxx> wrote:
> > > Just an idea: Maybe the names are also an opportunity to distinguish
> > > real _unit_ style tests and then the rarer integration-style tests. I
> > > personally prefer using the more generic *-test.c, at least for the
> > > integration-style tests I've been working on (KUnit is still incredibly
> > > valuable for integration-style tests, because otherwise I'd have to roll
> > > my own poor-man's version of KUnit, so thank you!). Using *_kunit.c for
> > > such tests is unintuitive, because the word "unit" hints at "unit tests"
> > > -- and having descriptive (and not misleading) filenames is still
> > > important. So I hope you won't mind if *-test.c are still used where
> > > appropriate.
>
> This is a good point, and I guess not one that has really been examined.
> I'm not sure what to think of some of the lib/ tests. test_user_copy
> seems to be a "unit" test -- it's validating the function family vs
> all kinds of arguments and conditions. But test_overflow is less unit
> and more integration, which includes "do all of these allocators end up
> acting the same way?" etc
>
> I'm not really sure what to make of that -- I don't really have a formal
> testing background.
>

I'm not sure we need a super-precise definition here (maybe just
because I don't have one), and really just need to work out what
distinctions are actually useful. For example, I'm not sure there's
any real advantage to treating test_user_copy and test_overflow
differently. The KCSAN tests which spawn lots of threads and are both
slower and less deterministic seem more obviously different, though.

I guess there are two audiences to cater for:
1. Test authors, who may wish to have both unit-style and
integration-style tests, and want to distinguish them. They probably
have the best idea of where the line should be drawn for their
subsystems, and may have some existing style/nomenclature.
2. People running "all tests", who want to broadly understand how the
whole suite of tests will behave (e.g., how long they'll take to run,
are they possibly nondeterministic, are there weird hardware/software
dependencies). This is where some more standardisation probably makes
sense.

I'm not 100% the file/module name is the best place to make these
distinctions (given that it's the Kconfig entries that are at least
our current way of finding and running tests). An off-the-wall idea
would be to have a flags field in the test suite structure to note
things like "large/long-running test" or "nondeterministic", and have
either a KUnit option to bypass them, note them in the output, or even
something terrifying like parsing it out of a compiled module.

> > As Brendan alluded to in the talk, the popularity of KUnit for these
> > integration-style tests came as something of a surprise (more due to
> > our lack of imagination than anything else, I suspect). It's great
> > that it's working, though: I don't think anyone wants the world filled
> > with more single-use test "frameworks" than is necessary!
> >
> > I guess the interesting thing to note is that we've to date not really
> > made a distinction between KUnit the framework and the suite of all
> > KUnit tests. Maybe having a separate file/module naming scheme could
> > be a way of making that distinction, though it'd really only appear
> > when loading tests as modules -- there'd be no indication in e.g.,
> > suite names or test results. The more obvious solution to me (at
> > least, based on the current proposal) would be to have "integration"
> > or similar be part of the suite name (and hence the filename, so
> > _integration_kunit.c or similar), though even I admit that that's much
> > uglier. Maybe the idea of having the subsystem/suite distinction be
> > represented in the code could pave the way to having different suites
> > support different suffixes like that.
>
> Heh, yeah, let's not call them "_integration_kunit.c" ;) _behavior.c?
> _integration.c?
>

I think we'd really like something that says more strongly that this
is a test (which is I suspect one of the reasons why _kunit.c has its
detractors: it doesn't have the word "test" in it). The other thing to
consider is that if there are multiple tests for the same thing (e.g.,
a unit test suite and an integration test suite), they'd still need
separate, non-conflicting suite names if we wanted them to be able to
be present in the same kernel.

Maybe the right thing to do is to say that, for now, the _kunit.c
naming guideline only applies to "unit-style" tests.

> > Do you know of any cases where something has/would have both
> > unit-style tests and integration-style tests built with KUnit where
> > the distinction needs to be clear?
>
> This is probably the right question. :)
>
I had a quick look, and we don't appear to have anything quite like
this yet, at least with KUnit.


So, putting together the various bits of feedback, how about something
like this:
Test filenames/modules should end in _kunit.c, unless they are either
a) not unit-style tests -- i.e, test something other than correctness
(e.g., performance), are non-deterministic, take a long time to run (>
~1--2 seconds), or are testing across multiple subsystems -- OR
b) are ports of existing tests, which may keep their existing filename
(at least for now), so as not to break existing workflows.

This is a bit weaker than the existing guidelines, and will probably
need tightening up once we have a better idea of what non-unit tests
should be and/or the existing, inconsistently named tests are
sufficiently outnumbered by the _kunit ones that people are used to it
and the perceived ugliness fades away. What (if any) tooling we need
around enumerating tests may end up influencing/being influenced by
this a bit, too.

Thoughts?
-- David