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

[Marco Elver]

On Fri, Sep 04, 2020 at 12:22PM +0800, David Gow wrote:
[...]
> 
> This is a good point -- renaming files is definitely a pain. It's
> obviously my hope that KUnit sticks around long enough that it's not
> being added/removed as a dependency too often, particularly for the
> unit tests, so "_kunit" as a name doesn't worry me that much
> otherwise.

Make sense. And I do also hope that once a test is a KUnit test, there
is no need to change it. :-)

[...]
> I'm not personally convinced that "kunit" isn't something people could
> associate with tests, particularly as it becomes more popular, but if
> people really dislike it, we could have"_unittest.c" or similar.
> There's a balancing act between being generic (and not distinguishing
> between unit/integration/etc tests) and being consistent or avoiding
> renames. Take the case where there's a set of unit tests in a
> "-test.c" file, and an integration test is written as well: it
> probably should go in a speparate file, so now you'd either have a
> "-test.c" and a separate "-integration-test.c" (or the other way
> around if the integration test was written first), or the "-test.c"
> file would be renamed.

Makes sense, too. Yeah, if we have both we'd need to distinguish one way
or another. What might be particularly annoying is the case if an
integration test exists first, and it had been named "_kunit.c",
followed by addition of a unit test. I think the only sane thing at that
point would be to do a rename of the integration test; whereas if it had
been named "_test.c", I could live with there simply being a
"_unit_test.c" (or similar) file for the new unit test.

[...]
> > 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.)
> >
> 
> I think that these are somewhat in conflict with each other, which is
> what makes this complicated. Particularly, it's going to be difficult
> to both avoid renames if the test framework changes and to distinguish
> between KUnit and non-KUnit tests by filename.
> 
> I personally think that of these requirements, 2 is probably the one
> that would cause people the most real-world pain. I'm not sure how
> often test type or scope changes enough to be worth the rename, and I
> hope KUnit survives long enough and is useful enough that test
> framework changes are kept to a minimum, but this has already
> irritated enough people porting tests to KUnit to be a noticeable
> issue. One possibility is to focus on module names, which are probably
> more important and can be renamed without changing the filename,
> though that's pretty ugly.
> 
> I actually think "_kunit.c" probably is descriptive/intuitive enough
> to meet (1) -- or at least will be once KUnit is more widely used --
> but it does conflict a bit with 2.
> 
> It'd be nice to have consistently named tests, but we're not there at
> the moment, so fixing it will require a lot of renaming things. It's
> looking increasingly unlikely that we'll be able to do that for
> everything, so making this a recommendation for new test suites is
> probably the best we're likely to get.
> 
> > 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.
> >
> 
> Kees also brings up that using hyphens instead of underscores causes
> some inconsistency with module names, which is a bit of a pain.
> 
> > 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.
> >
> 
> My concern with this is that we're introducing new jargon either way:
> does having "test" in the name outweigh the potential confusion from
> having "ktest" be in the filename only for "KUnit tests". So my
> feeling is that this would've been really useful if we'd named KUnit
> KTest (which, ironically, I think Brendan had considered) instead, but
> as-is is probably more confusing.

Make sense, too.

> At the risk of just chickening out at calling this "too hard", I'm
> leaning towards a variant of (A) here, and going for _test, but making
> it a weaker recommendation:
> - Specifying that the module name should end in _test, rather than the
> source filename. Module names are easier to change without causing
> merge conflicts (though they're a pain to change for the user).
> - Only applies to new test suites, and another suffix may be used if
> it conflicts with an existing non-kunit test (if it conflicts with a
> kunit test, they should be disambiguated in the suite name).
> - Test types (unit, integration, some subsystem-specific thing, etc)
> may be disambiguated in the suite name, at the discretion of the test
> author. (e.g., "driver_integration" as a suite name, with
> "driver_integration_test" as the module name, and either
> "driver_integration_test.c" or "integration_test.c" as recommended
> test filenames, depending on if "driver" is in its own directory.)
> 
> This should satisfy 1 & 2, and go some way towards satisfying 3. We
> can try to come up with some other technical solution to 4 if we need
> to.
> 
> Unless the objections are particularly earth-shattering, I'll do a new
> version of the patch that matches this next week. The other option is
> to drop the filename stuff from the document altogether, and sort it
> out in another patch, so we at least get some of the consistency in
> suite and Kconfig names.

Thanks for the detailed answer. Your plan sounds good to me. I'm fine
either way, as long as requirement (2) is somehow addressed, and we do
not end up with unnecessary renames.

Thanks,
-- Marco