On Tue, 1 Sep 2020 at 07:31, David Gow [off-list ref] wrote:

On Tue, Sep 1, 2020 at 7:47 AM Kees Cook [off-list ref] wrote:

quoted

On Fri, Aug 28, 2020 at 12:17:05AM +0800, David Gow wrote:

quoted

On Thu, Aug 27, 2020 at 9:14 PM Marco Elver [off-list ref] 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.

[...]

quoted

quoted

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