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

[Marco Elver <elver@google.com>]

On Tue, 1 Sep 2020 at 07:31, David Gow <davidgow@google.com> wrote:
> On Tue, Sep 1, 2020 at 7:47 AM Kees Cook <keescook@chromium.org> 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@google.com> wrote:
[...]
>
> 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).

I agree -- as you note, it's very hard to make this distinction. Since
we're still discussing the best convention to use, one point I want to
make is that encoding a dependency ("kunit") or type of test (unit,
integration, etc.) in the name hurts scalability of our workflows.
Because as soon as the dependency changes, or the type, any
rename/move is very destructive to our workflow, because it
immediately causes conflict with any in-flight patches. Whereas
encoding this either in a comment, or via Kconfig would be less
destructive.

> 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 a side-node, in the other very large codebase I have worked on, we
have such markers ("size = ..."):
https://docs.bazel.build/versions/master/be/common-definitions.html#common-attributes-tests
However, there is also incentive to get this distinction right,
because the test will be killed by the CI system if it exceeds the
specified size (ran too long, OOM). Not sure we have this incentive
yet.

[...]
> > > 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?

If possible, I'd still prefer generic filenames, because it's easy to
get wrong as we noted. Changes will cause conflicts.

> 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).

^ Agreed.

> 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.
[...]
>
> 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?

That could work, but it all still feels a little unsatisfying. Here's
what I think the requirements for all this are:

1. Clear, intuitive, descriptive filenames ("[...] something that says
more strongly that this is a test [...]").

2. Avoid renames if any of the following changes: test framework, test
type or scope. I worry the most about this point, because it affects
our workflows. We need to avoid unnecessary patch conflicts, keep
cherry-picks simple, etc.

3. Strive for consistently named tests, regardless of type (because
it's hard to get right).

4. Want to distinguish KUnit tests from non-KUnit tests. (Also
consider that tooling can assist with this.)

These are the 2 options under closer consideration:

A. Original choice of "*-test.c": Satisfies 1,2,3. It seems to fail 4,
per Kees's original concern.

B. "*_kunit.c": Satisfies 4, maybe 3.
  - Fails 1, because !strstr("_kunit.c", "test") and the resulting
indirection. It hints at "unit test", but this may be a problem for
(2).
  - Fails 2, because if the test for some reason decides to stop using
KUnit (or a unit test morphs into an integration test), the file needs
to be renamed.

And based on all this, why not:

C. "*-ktest.c" (or "*_ktest.c"):
  - Satisfies 1, because it's descriptive and clearly says it's a
test; the 'k' can suggest it's an "[in-]kernel test" vs. some other
hybrid test that requires a userspace component.
  - Satisfies 2, because neither test framework or test type need to
be encoded in the filename.
  - Satisfies 3, because every test (that wants to use KUnit) can just
use this without thinking too much about it.
  - Satisfies 4, because "git grep -- '[-_]ktest\.[co]'" returns nothing.

Thanks,
-- Marco