Re: [PATCH] ext: EXT4_KUNIT_TESTS should depend on EXT4_FS instead of selecting it

[David Gow]

On Fri, Oct 23, 2020 at 10:07 PM Theodore Y. Ts'o <tytso@xxxxxxx> wrote:
>
> On Thu, Oct 22, 2020 at 04:52:52PM -0700, Brendan Higgins wrote:
> > So you, me, Luis, David, and a whole bunch of other people have been
> > thinking about this problem for a while. What if we just put
> > kunitconfig fragments in directories along side the test files they
> > enable?
> >
> > For example, we could add a file to fs/ext4/kunitconfig which contains:
> >
> > CONFIG_EXT4_FS=y
> > CONFIG_EXT4_KUNIT_TESTS=y
> >
> > We could do something similar in fs/jdb2, etc.
> >
> > Obviously some logically separate KUnit tests (different maintainers,
> > different Kconfig symbols, etc) reside in the same directory, for
> > these we could name the kunitconfig file something like
> > lib/list-test.kunitconfig (not a great example because lists are
> > always built into Linux), but you get the idea.
> >
> > Then like Ted suggested, if you call kunit.py run foo/bar, then
> >
> > if bar is a directory, then kunit.py will look for foo/bar/kunitconfig
> >
> > if bar is a file ending with .kunitconfig like foo/bar.kunitconfig,
> > then it will use that kunitconfig
> >
> > if bar is '...' (foo/...) then kunit.py will look for all kunitconfigs
> > underneath foo.
> >
> > Once all the kunitconfigs have been resolved, they will be merged into
> > the .kunitconfig. If they can be successfully merged together, the new
> > .kunitconfig will then continue to function as it currently does.
>
> I was thinking along a similar set of lines this morning.  One thing
> I'd add in addition to your suggestion to that is to change how
> .kunitconfig is interpreted such that
>
> CONFIG_KUNIT=y
>
> is always implied, so it doesn't have to be specified explicitly, and
> that if a line like:
>
> fs/ext4
>
> or
>
> mm
>
> etc. occurs, that will cause a include of the Kunitconfig (I'd using a
> capitalized version of the filename like Kconfig, so that it's easier
> to see in a directory listing) in the named directory.
>
> That way, .kunitconfig is backwards compatible, but it also allows
> people to put a one-liner into .kunitconfig to enable the unit tests
> for that particular directory.
>
> What do folks think?
>

I quite like the idea of supporting includes, as it'd make it easier
to have test hierarchies as well: fs/Kunitconfig could include
ext4/Kunitconfig and fat/Kunitconfig, for instance. If we're adding
something more complicated to the Kunitconfig files than just raw
config entries, there are other things we could do, too (personally,
I'd quite like to be able to list KUnit test modules to be loaded
someday, though that's a bit outside the scope of this discussion).

There are some issues with exactly how we format these that'll need to
be resolved: there are cases where there are multiple distinct "areas"
that need testing which share a subdirectory (something like lib/), so
just using directory paths with one Kunitconfig file per directory has
some limitations. At the same time, it's definitely nicer to be able
to just specify a directory where that works.

Here's what I propose (noting that, realistically, it's unlikely we'll
have time to build the perfect system straight away):

1. We've agreed that 'select' isn't the solution we want, so accept
this patch to get rid of it, and avoid using it elsewhere (I've done
this for the fat test[1]). Do we know if changing this now will
seriously break anyone's workflow (particularly if there's something
automated that'd break?)

2. Add support to kunit.py for loading an arbitrary kunitconfig,
rather than using the default one in the root or build dir. (e.g.,
kunit.py run [path_to_kunitconfig]). This'd let us create
per-subsystem/feature/etc kunitconfigs and use them straight away.
This'd still require people to pass in the kunitconfig path each time
they run the tests, but'd at least give maintainers a single command
they can use and recommend — they'd just need to have a Kunitconfig
file somewhere in the tree with the tests they want people to run.
Maybe if you pass a directory path, it looks for "Kunitconfig" in that
directory, but otherwise it can be a file like
"lib/data-structres.Kunitconfig" or something which doesn't correspond
to a directory.

3. Add the include support, etc, to kunitconfig, so people working on
multiple subsystems can more easily run groups of tests. This may get
a little complicated if we care about handling things with conflicting
dependencies, so I don't want to hold up the first two on that.

How does that sound?

-- David

[1]: https://lore.kernel.org/linux-kselftest/20201024052047.2526780-1-davidgow@xxxxxxxxxx/