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

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

[Geert Uytterhoeven]

EXT4_KUNIT_TESTS selects EXT4_FS, thus enabling an optional feature the
user may not want to enable.  Fix this by making the test depend on
EXT4_FS instead.

Fixes: 1cbeab1b242d16fd ("ext4: add kunit test for decoding extended timestamps")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
See also "[PATCH] mptcp: MPTCP_KUNIT_TESTS should depend on MPTCP
instead of selecting it".
https://lore.kernel.org/lkml/20201019113240.11516-1-geert@linux-m68k.org/
---
 fs/ext4/Kconfig | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/ext4/Kconfig b/fs/ext4/Kconfig
index 619dd35ddd48a973..86699c8cab281cbc 100644
--- a/fs/ext4/Kconfig
+++ b/fs/ext4/Kconfig
@@ -103,8 +103,7 @@ config EXT4_DEBUG
 
 config EXT4_KUNIT_TESTS
 	tristate "KUnit tests for ext4" if !KUNIT_ALL_TESTS
-	select EXT4_FS
-	depends on KUNIT
+	depends on EXT4_FS && KUNIT
 	default KUNIT_ALL_TESTS
 	help
 	  This builds the ext4 KUnit tests.
-- 
2.17.1

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

[Brendan Higgins]

On Tue, Oct 20, 2020 at 12:37 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> EXT4_KUNIT_TESTS selects EXT4_FS, thus enabling an optional feature the
> user may not want to enable.  Fix this by making the test depend on
> EXT4_FS instead.
>
> Fixes: 1cbeab1b242d16fd ("ext4: add kunit test for decoding extended timestamps")
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

If I remember correctly, having EXT4_KUNIT_TESTS select EXT4_FS was
something that Ted specifically requested, but I don't have any strong
feelings on it either way.

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

[Randy Dunlap]

On 10/21/20 2:15 PM, Brendan Higgins wrote:
> On Tue, Oct 20, 2020 at 12:37 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>>
>> EXT4_KUNIT_TESTS selects EXT4_FS, thus enabling an optional feature the
>> user may not want to enable.  Fix this by making the test depend on
>> EXT4_FS instead.
>>
>> Fixes: 1cbeab1b242d16fd ("ext4: add kunit test for decoding extended timestamps")
>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> 
> If I remember correctly, having EXT4_KUNIT_TESTS select EXT4_FS was
> something that Ted specifically requested, but I don't have any strong
> feelings on it either way.

omg, please No. depends on is the right fix here.

-- 
~Randy

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

[David Gow]

On Thu, Oct 22, 2020 at 5:15 AM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> On Tue, Oct 20, 2020 at 12:37 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> >
> > EXT4_KUNIT_TESTS selects EXT4_FS, thus enabling an optional feature the
> > user may not want to enable.  Fix this by making the test depend on
> > EXT4_FS instead.
> >
> > Fixes: 1cbeab1b242d16fd ("ext4: add kunit test for decoding extended timestamps")
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
> If I remember correctly, having EXT4_KUNIT_TESTS select EXT4_FS was
> something that Ted specifically requested, but I don't have any strong
> feelings on it either way.

For what it's worth, the upcoming FAT filesystem tests[1] are also
select-ing FAT_FS at the moment, so if this changes here, I'll likely
update it there as well.

-- David

[1]: https://lore.kernel.org/linux-kselftest/20201021061713.1545931-1-davidgow@google.com/T/#u

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

[Randy Dunlap]

On 10/21/20 2:29 PM, David Gow wrote:
> On Thu, Oct 22, 2020 at 5:15 AM Brendan Higgins
> <brendanhiggins@google.com> wrote:
>>
>> On Tue, Oct 20, 2020 at 12:37 AM Geert Uytterhoeven
>> <geert@linux-m68k.org> wrote:
>>>
>>> EXT4_KUNIT_TESTS selects EXT4_FS, thus enabling an optional feature the
>>> user may not want to enable.  Fix this by making the test depend on
>>> EXT4_FS instead.
>>>
>>> Fixes: 1cbeab1b242d16fd ("ext4: add kunit test for decoding extended timestamps")
>>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>>
>> If I remember correctly, having EXT4_KUNIT_TESTS select EXT4_FS was
>> something that Ted specifically requested, but I don't have any strong
>> feelings on it either way.
> 
> For what it's worth, the upcoming FAT filesystem tests[1] are also
> select-ing FAT_FS at the moment, so if this changes here, I'll likely
> update it there as well.
> 
> -- David
> 
> [1]: https://lore.kernel.org/linux-kselftest/20201021061713.1545931-1-davidgow@google.com/T/#u
> 

then hopefully both get updated. This is just backwards.

-- 
~Randy

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

[Theodore Y. Ts'o]

On Wed, Oct 21, 2020 at 02:16:56PM -0700, Randy Dunlap wrote:
> On 10/21/20 2:15 PM, Brendan Higgins wrote:
> > On Tue, Oct 20, 2020 at 12:37 AM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> >>
> >> EXT4_KUNIT_TESTS selects EXT4_FS, thus enabling an optional feature the
> >> user may not want to enable.  Fix this by making the test depend on
> >> EXT4_FS instead.
> >>
> >> Fixes: 1cbeab1b242d16fd ("ext4: add kunit test for decoding extended timestamps")
> >> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > 
> > If I remember correctly, having EXT4_KUNIT_TESTS select EXT4_FS was
> > something that Ted specifically requested, but I don't have any strong
> > feelings on it either way.
> 
> omg, please No. depends on is the right fix here.

So my requirement which led to that particular request is to keep what
needs to be placed in .kunitconfig to a small and reasonable set.

Per Documentation/dev-tools/kunit, we start by:

    cd $PATH_TO_LINUX_REPO
    cp arch/um/configs/kunit_defconfig .kunitconfig

we're then supposed to add whatever Kunit tests we want to enable, to wit:

CONFIG_EXT4_KUNIT_TESTS=y

so that .kunitconfig would look like this:

CONFIG_KUNIT=y
CONFIG_KUNIT_TEST=y
CONFIG_KUNIT_EXAMPLE_TEST=y
CONFIG_EXT4_KUNIT_TESTS=y

... and then you should be able to run:

./tools/testing/kunit/kunit.py run

... and have the kunit tests run.  I would *not* like to have to put a
huge long list of CONFIG_* dependencies into the .kunitconfig file.

I'm don't particularly care how this gets achieved, but please think
about how to make it easy for a kernel developer to run a specific set
of subsystem unit tests.  (In fact, being able to do something like
"kunit.py run fs/ext4 fs/jbd2" or maybe "kunit.py run fs/..." would be
*great*.  No need to fuss with hand editing the .kunitconfig file at
all would be **wonderful**.

Cheers,

					- Ted

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

[Randy Dunlap]

On 10/21/20 3:36 PM, Theodore Y. Ts'o wrote:
> On Wed, Oct 21, 2020 at 02:16:56PM -0700, Randy Dunlap wrote:
>> On 10/21/20 2:15 PM, Brendan Higgins wrote:
>>> On Tue, Oct 20, 2020 at 12:37 AM Geert Uytterhoeven
>>> <geert@linux-m68k.org> wrote:
>>>>
>>>> EXT4_KUNIT_TESTS selects EXT4_FS, thus enabling an optional feature the
>>>> user may not want to enable.  Fix this by making the test depend on
>>>> EXT4_FS instead.
>>>>
>>>> Fixes: 1cbeab1b242d16fd ("ext4: add kunit test for decoding extended timestamps")
>>>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>>>
>>> If I remember correctly, having EXT4_KUNIT_TESTS select EXT4_FS was
>>> something that Ted specifically requested, but I don't have any strong
>>> feelings on it either way.
>>
>> omg, please No. depends on is the right fix here.
> 
> So my requirement which led to that particular request is to keep what
> needs to be placed in .kunitconfig to a small and reasonable set.
> 
> Per Documentation/dev-tools/kunit, we start by:
> 
>     cd $PATH_TO_LINUX_REPO
>     cp arch/um/configs/kunit_defconfig .kunitconfig
> 
> we're then supposed to add whatever Kunit tests we want to enable, to wit:
> 
> CONFIG_EXT4_KUNIT_TESTS=y
> 
> so that .kunitconfig would look like this:
> 
> CONFIG_KUNIT=y
> CONFIG_KUNIT_TEST=y
> CONFIG_KUNIT_EXAMPLE_TEST=y
> CONFIG_EXT4_KUNIT_TESTS=y
> 
> ... and then you should be able to run:
> 
> ./tools/testing/kunit/kunit.py run
> 
> ... and have the kunit tests run.  I would *not* like to have to put a
> huge long list of CONFIG_* dependencies into the .kunitconfig file.
> 
> I'm don't particularly care how this gets achieved, but please think
> about how to make it easy for a kernel developer to run a specific set
> of subsystem unit tests.  (In fact, being able to do something like
> "kunit.py run fs/ext4 fs/jbd2" or maybe "kunit.py run fs/..." would be
> *great*.  No need to fuss with hand editing the .kunitconfig file at
> all would be **wonderful**.

I understand the wish for ease of use, but this is still the tail
wagging the dog.

The primary documentation for 'select' is
Documentation/kbuild/kconfig-language.rst, which says:

  Note:
	select should be used with care. select will force
	a symbol to a value without visiting the dependencies.
	By abusing select you are able to select a symbol FOO even
	if FOO depends on BAR that is not set.
	In general use select only for non-visible symbols
	(no prompts anywhere) and for symbols with no dependencies.
	That will limit the usefulness but on the other hand avoid
	the illegal configurations all over.


-- 
~Randy

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

[Theodore Y. Ts'o]

On Wed, Oct 21, 2020 at 04:07:15PM -0700, Randy Dunlap wrote:
> > I'm don't particularly care how this gets achieved, but please think
> > about how to make it easy for a kernel developer to run a specific set
> > of subsystem unit tests.  (In fact, being able to do something like
> > "kunit.py run fs/ext4 fs/jbd2" or maybe "kunit.py run fs/..." would be
> > *great*.  No need to fuss with hand editing the .kunitconfig file at
> > all would be **wonderful**.
> 
> I understand the wish for ease of use, but this is still the tail
> wagging the dog.
> 
> The primary documentation for 'select' is
> Documentation/kbuild/kconfig-language.rst, which says:
> 
>   Note:
> 	select should be used with care. select will force
> 	a symbol to a value without visiting the dependencies.
> 	By abusing select you are able to select a symbol FOO even
> 	if FOO depends on BAR that is not set.
> 	In general use select only for non-visible symbols
> 	(no prompts anywhere) and for symbols with no dependencies.
> 	That will limit the usefulness but on the other hand avoid
> 	the illegal configurations all over.
> 

Well, the KUNIT configs are kinda of a special case, since normally
they don't have a lot of huge number of dependencies, since unit tests
in general are not integration tests.  So ideally, dependencies will
mostly be replaced with mocking functions.  And if there are *real*
dependencies that the Kunit Unit tests need, they can be explicitly
pulled in with selects.

That being said, as I said, I'm not picky about *how* this gets
achieved.  But ease of use is a key part of making people more likely
to run the unit tests.  So another way of solving the problem might be
to put some kind of automated dependency solver into kunit.py, or some
way of manually adding the necessary dependencies in some kind of
Kunitconfig file that are in directories where their are Unit tests,
or maybe some kind of extenstion to the Kconfig file.  My main
requirement is that the only thing that should be necessary for
enabling the ext4 Kunit tests should be adding a single line to the
.kunitconfig file.  It's not fair to make the human developer manually
have to figure out the dependency chains.

As far as I'm concerned, ease of use is important enough to justfy
special casing and/or bending the rules as far as "select" is concered
for Kunit-related CONFIG items.  But if someone else want to suggest a
better approach, I'm all ears.

Cheers,

							- Ted

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

[Randy Dunlap]

On 10/21/20 8:43 PM, Theodore Y. Ts'o wrote:
> On Wed, Oct 21, 2020 at 04:07:15PM -0700, Randy Dunlap wrote:
>>> I'm don't particularly care how this gets achieved, but please think
>>> about how to make it easy for a kernel developer to run a specific set
>>> of subsystem unit tests.  (In fact, being able to do something like
>>> "kunit.py run fs/ext4 fs/jbd2" or maybe "kunit.py run fs/..." would be
>>> *great*.  No need to fuss with hand editing the .kunitconfig file at
>>> all would be **wonderful**.
>>
>> I understand the wish for ease of use, but this is still the tail
>> wagging the dog.
>>
>> The primary documentation for 'select' is
>> Documentation/kbuild/kconfig-language.rst, which says:
>>
>>   Note:
>> 	select should be used with care. select will force
>> 	a symbol to a value without visiting the dependencies.
>> 	By abusing select you are able to select a symbol FOO even
>> 	if FOO depends on BAR that is not set.
>> 	In general use select only for non-visible symbols
>> 	(no prompts anywhere) and for symbols with no dependencies.
>> 	That will limit the usefulness but on the other hand avoid
>> 	the illegal configurations all over.
>>
> 
> Well, the KUNIT configs are kinda of a special case, since normally
> they don't have a lot of huge number of dependencies, since unit tests
> in general are not integration tests.  So ideally, dependencies will
> mostly be replaced with mocking functions.  And if there are *real*
> dependencies that the Kunit Unit tests need, they can be explicitly
> pulled in with selects.
> 
> That being said, as I said, I'm not picky about *how* this gets
> achieved.  But ease of use is a key part of making people more likely
> to run the unit tests.  So another way of solving the problem might be
> to put some kind of automated dependency solver into kunit.py, or some
> way of manually adding the necessary dependencies in some kind of
> Kunitconfig file that are in directories where their are Unit tests,
> or maybe some kind of extenstion to the Kconfig file.  My main
> requirement is that the only thing that should be necessary for
> enabling the ext4 Kunit tests should be adding a single line to the
> .kunitconfig file.  It's not fair to make the human developer manually
> have to figure out the dependency chains.
> 
> As far as I'm concerned, ease of use is important enough to justfy
> special casing and/or bending the rules as far as "select" is concered
> for Kunit-related CONFIG items.  But if someone else want to suggest a
> better approach, I'm all ears.
> 
> Cheers,

Indeed.  For the record, I support testing and have for a long time.
I just don't care for this big fscking hammer approach.
But I doubt that I can change your mind.

g'day.
-- 
~Randy

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

[Geert Uytterhoeven]

Hi Ted,

On Thu, Oct 22, 2020 at 5:43 AM Theodore Y. Ts'o <tytso@mit.edu> wrote:
> On Wed, Oct 21, 2020 at 04:07:15PM -0700, Randy Dunlap wrote:
> > > I'm don't particularly care how this gets achieved, but please think
> > > about how to make it easy for a kernel developer to run a specific set
> > > of subsystem unit tests.  (In fact, being able to do something like
> > > "kunit.py run fs/ext4 fs/jbd2" or maybe "kunit.py run fs/..." would be
> > > *great*.  No need to fuss with hand editing the .kunitconfig file at
> > > all would be **wonderful**.
> >
> > I understand the wish for ease of use, but this is still the tail
> > wagging the dog.
> >
> > The primary documentation for 'select' is
> > Documentation/kbuild/kconfig-language.rst, which says:
> >
> >   Note:
> >       select should be used with care. select will force
> >       a symbol to a value without visiting the dependencies.
> >       By abusing select you are able to select a symbol FOO even
> >       if FOO depends on BAR that is not set.
> >       In general use select only for non-visible symbols
> >       (no prompts anywhere) and for symbols with no dependencies.
> >       That will limit the usefulness but on the other hand avoid
> >       the illegal configurations all over.
> >
>
> Well, the KUNIT configs are kinda of a special case, since normally

Please read "Why my CONFIG symbol is (not) special?"...

> they don't have a lot of huge number of dependencies, since unit tests

They still depend on the feature under test, which may not be enabled.

> in general are not integration tests.  So ideally, dependencies will
> mostly be replaced with mocking functions.  And if there are *real*
> dependencies that the Kunit Unit tests need, they can be explicitly
> pulled in with selects.

While I'm a strong supporter of testing, this select is what I object
to, as it forces the enablement of features that were not enabled in
your kernel in the first place.

> That being said, as I said, I'm not picky about *how* this gets
> achieved.  But ease of use is a key part of making people more likely
> to run the unit tests.  So another way of solving the problem might be
> to put some kind of automated dependency solver into kunit.py, or some
> way of manually adding the necessary dependencies in some kind of
> Kunitconfig file that are in directories where their are Unit tests,
> or maybe some kind of extenstion to the Kconfig file.  My main
> requirement is that the only thing that should be necessary for
> enabling the ext4 Kunit tests should be adding a single line to the
> .kunitconfig file.  It's not fair to make the human developer manually
> have to figure out the dependency chains.

If you want to test e.g. EXT4, sure you know you should enable EXT4?
If you don't enable feature FOO in your kernel. why would you be
interested in testing feature FOO? Unless you want to test everything,
and you might as well run an allmodconfig kernel.

> As far as I'm concerned, ease of use is important enough to justfy
> special casing and/or bending the rules as far as "select" is concered
> for Kunit-related CONFIG items.  But if someone else want to suggest a
> better approach, I'm all ears.

Providing .config snippets containing both the CONFIG_FOO_KUNIT_TESTS
and CONFIG_FOO symbols set (and other dependencies, if needed) could
provide this, and would also be useful in documenting CONFIG_FOO has a
corresponding tests.

As kunit can be modular, and most tests can be modular too, I can build
all tests relevant for my system as modules.  Hence they are available
when the need to run them ever arises, while there is no further impact
on my system. This use case also seems to be supported by
CONFIG_KUNIT_ALL_TESTS:

    config KUNIT_ALL_TESTS
            tristate "All KUnit tests with satisfied dependencies"
            help
              Enables all KUnit tests, if they can be enabled.

However, this no longer works if this suddenly starts enabling random
features in my kernel, which was the case for EXT4_KUNIT_TESTS
and MPTCP_KUNIT_TESTS (enables MPTCP, and IPV6!).

While ext4 is ubiquitous (I did have it enabled ;-) and may be
considered common on all systems (it is not: e.g. embedded systems with
JFFS do not need EXT4 enabled), this is surely not the case for other
features with tests, and won't be the case for all features that will
receive kunit tests in the future.

Thanks for your understanding!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Brendan Higgins]

On Wed, Oct 21, 2020 at 3:36 PM Theodore Y. Ts'o <tytso@mit.edu> wrote:
>
> On Wed, Oct 21, 2020 at 02:16:56PM -0700, Randy Dunlap wrote:
> > On 10/21/20 2:15 PM, Brendan Higgins wrote:
> > > On Tue, Oct 20, 2020 at 12:37 AM Geert Uytterhoeven
> > > <geert@linux-m68k.org> wrote:
> > >>
> > >> EXT4_KUNIT_TESTS selects EXT4_FS, thus enabling an optional feature the
> > >> user may not want to enable.  Fix this by making the test depend on
> > >> EXT4_FS instead.
> > >>
> > >> Fixes: 1cbeab1b242d16fd ("ext4: add kunit test for decoding extended timestamps")
> > >> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > >
> > > If I remember correctly, having EXT4_KUNIT_TESTS select EXT4_FS was
> > > something that Ted specifically requested, but I don't have any strong
> > > feelings on it either way.
> >
> > omg, please No. depends on is the right fix here.
>
> So my requirement which led to that particular request is to keep what
> needs to be placed in .kunitconfig to a small and reasonable set.
>
> Per Documentation/dev-tools/kunit, we start by:
>
>     cd $PATH_TO_LINUX_REPO
>     cp arch/um/configs/kunit_defconfig .kunitconfig
>
> we're then supposed to add whatever Kunit tests we want to enable, to wit:
>
> CONFIG_EXT4_KUNIT_TESTS=y
>
> so that .kunitconfig would look like this:
>
> CONFIG_KUNIT=y
> CONFIG_KUNIT_TEST=y
> CONFIG_KUNIT_EXAMPLE_TEST=y
> CONFIG_EXT4_KUNIT_TESTS=y
>
> ... and then you should be able to run:
>
> ./tools/testing/kunit/kunit.py run
>
> ... and have the kunit tests run.  I would *not* like to have to put a
> huge long list of CONFIG_* dependencies into the .kunitconfig file.
>
> I'm don't particularly care how this gets achieved, but please think
> about how to make it easy for a kernel developer to run a specific set
> of subsystem unit tests.  (In fact, being able to do something like
> "kunit.py run fs/ext4 fs/jbd2" or maybe "kunit.py run fs/..." would be
> *great*.  No need to fuss with hand editing the .kunitconfig file at
> all would be **wonderful**.

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.

What do people think about this?

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

[Theodore Y. Ts'o]

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?

Cheers,

						- Ted

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@mit.edu> 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@google.com/