[PATCH] kunit/kunit_kernel: Rebuild .config if .kunitconfig is modified

[PATCH] kunit/kunit_kernel: Rebuild .config if .kunitconfig is modified

[sj38.park]

From: SeongJae Park <sjpark@amazon.de>

Deletions of configs in the '.kunitconfig' is not applied because kunit
rebuilds '.config' only if the '.config' is not a subset of the
'.kunitconfig'.  To allow the deletions to applied, this commit modifies
the '.config' rebuild condition to addtionally check the modified times
of those files.

Signed-off-by: SeongJae Park <sjpark@amazon.de>
---
 tools/testing/kunit/kunit_kernel.py | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
index cc5d844ecca1..a3a5d6c7e66d 100644
--- a/tools/testing/kunit/kunit_kernel.py
+++ b/tools/testing/kunit/kunit_kernel.py
@@ -111,17 +111,22 @@ class LinuxSourceTree(object):
 		return True
 
 	def build_reconfig(self, build_dir):
-		"""Creates a new .config if it is not a subset of the .kunitconfig."""
+		"""Creates a new .config if it is not a subset of, or older than the .kunitconfig."""
 		kconfig_path = get_kconfig_path(build_dir)
 		if os.path.exists(kconfig_path):
 			existing_kconfig = kunit_config.Kconfig()
 			existing_kconfig.read_from_file(kconfig_path)
-			if not self._kconfig.is_subset_of(existing_kconfig):
-				print('Regenerating .config ...')
-				os.remove(kconfig_path)
-				return self.build_config(build_dir)
-			else:
+			subset = self._kconfig.is_subset_of(existing_kconfig)
+
+			kunitconfig_mtime = os.path.getmtime(kunitconfig_path)
+			kconfig_mtime = os.path.getmtime(kconfig_path)
+			older = kconfig_mtime < kunitconfig_mtime
+
+			if subset and not older:
 				return True
+			print('Regenerating .config ...')
+			os.remove(kconfig_path)
+			return self.build_config(build_dir)
 		else:
 			print('Generating .config ...')
 			return self.build_config(build_dir)
-- 
2.17.1

[PATCH] docs/kunit/start: Use '_KUNIT_TEST' config name suffix

[sjpark]

From: SeongJae Park <sjpark@amazon.de>

It is recommended to use '_KUNIT_TEST' config name suffix for kunit
tests but the example is using only '_TEST' suffix.  This commit fixes
it to also use '_KUNIT_TEST' suffix.

Signed-off-by: SeongJae Park <sjpark@amazon.de>
---
 Documentation/dev-tools/kunit/start.rst | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/dev-tools/kunit/start.rst b/Documentation/dev-tools/kunit/start.rst
index 4e1d24db6b13..2f8b4eda97eb 100644
--- a/Documentation/dev-tools/kunit/start.rst
+++ b/Documentation/dev-tools/kunit/start.rst
@@ -138,7 +138,7 @@ Now add the following to ``drivers/misc/Kconfig``:
 
 .. code-block:: kconfig
 
-	config MISC_EXAMPLE_TEST
+	config MISC_EXAMPLE_KUNIT_TEST
 		bool "Test for my example"
 		depends on MISC_EXAMPLE && KUNIT
 
@@ -146,14 +146,14 @@ and the following to ``drivers/misc/Makefile``:
 
 .. code-block:: make
 
-	obj-$(CONFIG_MISC_EXAMPLE_TEST) += example-test.o
+	obj-$(CONFIG_MISC_EXAMPLE_KUNIT_TEST) += example-test.o
 
 Now add it to your ``.kunitconfig``:
 
 .. code-block:: none
 
 	CONFIG_MISC_EXAMPLE=y
-	CONFIG_MISC_EXAMPLE_TEST=y
+	CONFIG_MISC_EXAMPLE_KUNIT_TEST=y
 
 Now you can run the test:
 
-- 
2.17.1

Re: [PATCH] kunit/kunit_kernel: Rebuild .config if .kunitconfig is modified

[Brendan Higgins]

On Sat, Jan 25, 2020 at 5:59 PM <sj38.park@gmail.com> wrote:
>
> From: SeongJae Park <sjpark@amazon.de>
>
> Deletions of configs in the '.kunitconfig' is not applied because kunit
> rebuilds '.config' only if the '.config' is not a subset of the
> '.kunitconfig'.  To allow the deletions to applied, this commit modifies
> the '.config' rebuild condition to addtionally check the modified times
> of those files.

The reason it only checks that .kunitconfig is a subset of .config is
because we don't want the .kunitconfig to remove options just because
it doesn't recognize them.

It runs `make ARCH=um olddefconfig` on the .config that it generates
from the .kunitconfig, and most of the time that means you will get a
.config with lots of things in it that aren't in the .kunitconfig.
Consequently, nothing should ever be deleted from the .config just
because it was deleted in the .kunitconfig (unless, of course, you
change a =y to a =n or # ... is not set), so I don't see what this
change would do.

Can you maybe provide an example?

> Signed-off-by: SeongJae Park <sjpark@amazon.de>
> ---
>  tools/testing/kunit/kunit_kernel.py | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
> index cc5d844ecca1..a3a5d6c7e66d 100644
> --- a/tools/testing/kunit/kunit_kernel.py
> +++ b/tools/testing/kunit/kunit_kernel.py
> @@ -111,17 +111,22 @@ class LinuxSourceTree(object):
>                 return True
>
>         def build_reconfig(self, build_dir):
> -               """Creates a new .config if it is not a subset of the .kunitconfig."""
> +               """Creates a new .config if it is not a subset of, or older than the .kunitconfig."""
>                 kconfig_path = get_kconfig_path(build_dir)
>                 if os.path.exists(kconfig_path):
>                         existing_kconfig = kunit_config.Kconfig()
>                         existing_kconfig.read_from_file(kconfig_path)
> -                       if not self._kconfig.is_subset_of(existing_kconfig):
> -                               print('Regenerating .config ...')
> -                               os.remove(kconfig_path)
> -                               return self.build_config(build_dir)
> -                       else:
> +                       subset = self._kconfig.is_subset_of(existing_kconfig)
> +
> +                       kunitconfig_mtime = os.path.getmtime(kunitconfig_path)
> +                       kconfig_mtime = os.path.getmtime(kconfig_path)
> +                       older = kconfig_mtime < kunitconfig_mtime
> +
> +                       if subset and not older:
>                                 return True
> +                       print('Regenerating .config ...')
> +                       os.remove(kconfig_path)
> +                       return self.build_config(build_dir)
>                 else:
>                         print('Generating .config ...')
>                         return self.build_config(build_dir)
> --
> 2.17.1
>

Re: Re: [PATCH] kunit/kunit_kernel: Rebuild .config if .kunitconfig is modified

[SeongJae Park]

On Mon, 27 Jan 2020 16:02:48 -0800 Brendan Higgins <brendanhiggins@google.com> wrote:

> On Sat, Jan 25, 2020 at 5:59 PM <sj38.park@gmail.com> wrote:
> >
> > From: SeongJae Park <sjpark@amazon.de>
> >
> > Deletions of configs in the '.kunitconfig' is not applied because kunit
> > rebuilds '.config' only if the '.config' is not a subset of the
> > '.kunitconfig'.  To allow the deletions to applied, this commit modifies
> > the '.config' rebuild condition to addtionally check the modified times
> > of those files.
> 
> The reason it only checks that .kunitconfig is a subset of .config is
> because we don't want the .kunitconfig to remove options just because
> it doesn't recognize them.
> 
> It runs `make ARCH=um olddefconfig` on the .config that it generates
> from the .kunitconfig, and most of the time that means you will get a
> .config with lots of things in it that aren't in the .kunitconfig.
> Consequently, nothing should ever be deleted from the .config just
> because it was deleted in the .kunitconfig (unless, of course, you
> change a =y to a =n or # ... is not set), so I don't see what this
> change would do.
> 
> Can you maybe provide an example?

Sorry for my insufficient explanation.  I added a kunit test
(SYSCTL_KUNIT_TEST) to '.kunitconfig', ran the added test, and then removed it
from the file.  However, '.config' is not generated again due to the condition
and therefore the test still runs.

For more detail:

    $ ./tools/testing/kunit/kunit.py run --defconfig --build_dir ../kunit.out/
    $ echo "CONFIG_SYSCTL_KUNIT_TEST=y" >> ../kunit.out/.kunitconfig
    $ ./tools/testing/kunit/kunit.py run --build_dir ../kunit.out/
    $ sed -i '4d' ../kunit.out/.kunitconfig
    $ ./tools/testing/kunit/kunit.py run --build_dir ../kunit.out/

The 2nd line command adds sysctl kunit test and the 3rd line shows it runs the
added test as expected.  Because the default kunit config contains only 3
lines, The 4th line command removes the sysctl kunit test from the
.kunitconfig.  However, the 5th line still run the test.

This patch is for such cases.  Of course, this might make more false positives
but I believe it would not be a big problem because .config generation takes no
long time.  If I missed something, please let me know.


Thanks,
SeongJae Park

> 
> > Signed-off-by: SeongJae Park <sjpark@amazon.de>
> > ---
> >  tools/testing/kunit/kunit_kernel.py | 17 +++++++++++------
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
> > index cc5d844ecca1..a3a5d6c7e66d 100644
> > --- a/tools/testing/kunit/kunit_kernel.py
> > +++ b/tools/testing/kunit/kunit_kernel.py
> > @@ -111,17 +111,22 @@ class LinuxSourceTree(object):
> >                 return True
> >
> >         def build_reconfig(self, build_dir):
> > -               """Creates a new .config if it is not a subset of the .kunitconfig."""
> > +               """Creates a new .config if it is not a subset of, or older than the .kunitconfig."""
> >                 kconfig_path = get_kconfig_path(build_dir)
> >                 if os.path.exists(kconfig_path):
> >                         existing_kconfig = kunit_config.Kconfig()
> >                         existing_kconfig.read_from_file(kconfig_path)
> > -                       if not self._kconfig.is_subset_of(existing_kconfig):
> > -                               print('Regenerating .config ...')
> > -                               os.remove(kconfig_path)
> > -                               return self.build_config(build_dir)
> > -                       else:
> > +                       subset = self._kconfig.is_subset_of(existing_kconfig)
> > +
> > +                       kunitconfig_mtime = os.path.getmtime(kunitconfig_path)
> > +                       kconfig_mtime = os.path.getmtime(kconfig_path)
> > +                       older = kconfig_mtime < kunitconfig_mtime
> > +
> > +                       if subset and not older:
> >                                 return True
> > +                       print('Regenerating .config ...')
> > +                       os.remove(kconfig_path)
> > +                       return self.build_config(build_dir)
> >                 else:
> >                         print('Generating .config ...')
> >                         return self.build_config(build_dir)
> > --
> > 2.17.1
> >

Re: [PATCH] kunit/kunit_kernel: Rebuild .config if .kunitconfig is modified

[SeongJae Park]

Ping?


Thanks,
SeongJae Park

On Sun, 26 Jan 2020 01:59:24 +0000 sj38.park@gmail.com wrote:

> From: SeongJae Park <sjpark@amazon.de>
> 
> Deletions of configs in the '.kunitconfig' is not applied because kunit
> rebuilds '.config' only if the '.config' is not a subset of the
> '.kunitconfig'.  To allow the deletions to applied, this commit modifies
> the '.config' rebuild condition to addtionally check the modified times
> of those files.
> 
> Signed-off-by: SeongJae Park <sjpark@amazon.de>
> ---
>  tools/testing/kunit/kunit_kernel.py | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
> index cc5d844ecca1..a3a5d6c7e66d 100644
> --- a/tools/testing/kunit/kunit_kernel.py
> +++ b/tools/testing/kunit/kunit_kernel.py
> @@ -111,17 +111,22 @@ class LinuxSourceTree(object):
>  		return True
>  
>  	def build_reconfig(self, build_dir):
> -		"""Creates a new .config if it is not a subset of the .kunitconfig."""
> +		"""Creates a new .config if it is not a subset of, or older than the .kunitconfig."""
>  		kconfig_path = get_kconfig_path(build_dir)
>  		if os.path.exists(kconfig_path):
>  			existing_kconfig = kunit_config.Kconfig()
>  			existing_kconfig.read_from_file(kconfig_path)
> -			if not self._kconfig.is_subset_of(existing_kconfig):
> -				print('Regenerating .config ...')
> -				os.remove(kconfig_path)
> -				return self.build_config(build_dir)
> -			else:
> +			subset = self._kconfig.is_subset_of(existing_kconfig)
> +
> +			kunitconfig_mtime = os.path.getmtime(kunitconfig_path)
> +			kconfig_mtime = os.path.getmtime(kconfig_path)
> +			older = kconfig_mtime < kunitconfig_mtime
> +
> +			if subset and not older:
>  				return True
> +			print('Regenerating .config ...')
> +			os.remove(kconfig_path)
> +			return self.build_config(build_dir)
>  		else:
>  			print('Generating .config ...')
>  			return self.build_config(build_dir)
> -- 
> 2.17.1

Re: Re: [PATCH] kunit/kunit_kernel: Rebuild .config if .kunitconfig is modified

[Brendan Higgins]

Sorry for the delay.

On Mon, Jan 27, 2020 at 10:03 PM SeongJae Park <sj38.park@gmail.com> wrote:
>
> On Mon, 27 Jan 2020 16:02:48 -0800 Brendan Higgins <brendanhiggins@google.com> wrote:
>
> > On Sat, Jan 25, 2020 at 5:59 PM <sj38.park@gmail.com> wrote:
> > >
> > > From: SeongJae Park <sjpark@amazon.de>
> > >
> > > Deletions of configs in the '.kunitconfig' is not applied because kunit
> > > rebuilds '.config' only if the '.config' is not a subset of the
> > > '.kunitconfig'.  To allow the deletions to applied, this commit modifies
> > > the '.config' rebuild condition to addtionally check the modified times
> > > of those files.
> >
> > The reason it only checks that .kunitconfig is a subset of .config is
> > because we don't want the .kunitconfig to remove options just because
> > it doesn't recognize them.
> >
> > It runs `make ARCH=um olddefconfig` on the .config that it generates
> > from the .kunitconfig, and most of the time that means you will get a
> > .config with lots of things in it that aren't in the .kunitconfig.
> > Consequently, nothing should ever be deleted from the .config just
> > because it was deleted in the .kunitconfig (unless, of course, you
> > change a =y to a =n or # ... is not set), so I don't see what this
> > change would do.
> >
> > Can you maybe provide an example?
>
> Sorry for my insufficient explanation.  I added a kunit test
> (SYSCTL_KUNIT_TEST) to '.kunitconfig', ran the added test, and then removed it
> from the file.  However, '.config' is not generated again due to the condition
> and therefore the test still runs.
>
> For more detail:
>
>     $ ./tools/testing/kunit/kunit.py run --defconfig --build_dir ../kunit.out/
>     $ echo "CONFIG_SYSCTL_KUNIT_TEST=y" >> ../kunit.out/.kunitconfig
>     $ ./tools/testing/kunit/kunit.py run --build_dir ../kunit.out/
>     $ sed -i '4d' ../kunit.out/.kunitconfig
>     $ ./tools/testing/kunit/kunit.py run --build_dir ../kunit.out/
>
> The 2nd line command adds sysctl kunit test and the 3rd line shows it runs the
> added test as expected.  Because the default kunit config contains only 3
> lines, The 4th line command removes the sysctl kunit test from the
> .kunitconfig.  However, the 5th line still run the test.
>
> This patch is for such cases.  Of course, this might make more false positives
> but I believe it would not be a big problem because .config generation takes no
> long time.  If I missed something, please let me know.

I think I understand.

It is intentional - currently - that KUnit doesn't generate a new
.config with every invocation. The reason is basically to support
interaction with other methods of generating .configs. Consider that
you might want to use make menuconfig to turn something on. It is a
pretty handy interface if you work on vastly different parts of the
kernel. Or maybe you have a defconfig that you always use for some
platform, I think it is easier to run

make foo_config; tools/testing/kunit/kunit.py run

Then having to maintain both your defconfig and a .kunitconfig which
is a superset of the defconfig.

Your change would make it so that you have to have a .kunitconfig for
every test environment that you care about, and you could not as
easily take advantage of menuconfig.

I think what we do now is a bit janky, and the use cases I mentioned
are not super well supported. So I am sympathetic to what you are
trying to do, maybe we could have a config option for it?

I think Ted and Bjorn might have opinions on this; they had some
related opinions in the past.

Re: Re: Re: [PATCH] kunit/kunit_kernel: Rebuild .config if .kunitconfig is modified

[SeongJae Park]

On Tue, 4 Feb 2020 16:46:06 -0800 Brendan Higgins <brendanhiggins@google.com> wrote:

> Sorry for the delay.
> 
> On Mon, Jan 27, 2020 at 10:03 PM SeongJae Park <sj38.park@gmail.com> wrote:
> >
> > On Mon, 27 Jan 2020 16:02:48 -0800 Brendan Higgins <brendanhiggins@google.com> wrote:
> >
> > > On Sat, Jan 25, 2020 at 5:59 PM <sj38.park@gmail.com> wrote:
> > > >
> > > > From: SeongJae Park <sjpark@amazon.de>
> > > >
> > > > Deletions of configs in the '.kunitconfig' is not applied because kunit
> > > > rebuilds '.config' only if the '.config' is not a subset of the
> > > > '.kunitconfig'.  To allow the deletions to applied, this commit modifies
> > > > the '.config' rebuild condition to addtionally check the modified times
> > > > of those files.
> > >
> > > The reason it only checks that .kunitconfig is a subset of .config is
> > > because we don't want the .kunitconfig to remove options just because
> > > it doesn't recognize them.
> > >
> > > It runs `make ARCH=um olddefconfig` on the .config that it generates
> > > from the .kunitconfig, and most of the time that means you will get a
> > > .config with lots of things in it that aren't in the .kunitconfig.
> > > Consequently, nothing should ever be deleted from the .config just
> > > because it was deleted in the .kunitconfig (unless, of course, you
> > > change a =y to a =n or # ... is not set), so I don't see what this
> > > change would do.
> > >
> > > Can you maybe provide an example?
> >
> > Sorry for my insufficient explanation.  I added a kunit test
> > (SYSCTL_KUNIT_TEST) to '.kunitconfig', ran the added test, and then removed it
> > from the file.  However, '.config' is not generated again due to the condition
> > and therefore the test still runs.
> >
> > For more detail:
> >
> >     $ ./tools/testing/kunit/kunit.py run --defconfig --build_dir ../kunit.out/
> >     $ echo "CONFIG_SYSCTL_KUNIT_TEST=y" >> ../kunit.out/.kunitconfig
> >     $ ./tools/testing/kunit/kunit.py run --build_dir ../kunit.out/
> >     $ sed -i '4d' ../kunit.out/.kunitconfig
> >     $ ./tools/testing/kunit/kunit.py run --build_dir ../kunit.out/
> >
> > The 2nd line command adds sysctl kunit test and the 3rd line shows it runs the
> > added test as expected.  Because the default kunit config contains only 3
> > lines, The 4th line command removes the sysctl kunit test from the
> > .kunitconfig.  However, the 5th line still run the test.
> >
> > This patch is for such cases.  Of course, this might make more false positives
> > but I believe it would not be a big problem because .config generation takes no
> > long time.  If I missed something, please let me know.
> 
> I think I understand.
> 
> It is intentional - currently - that KUnit doesn't generate a new
> .config with every invocation. The reason is basically to support
> interaction with other methods of generating .configs. Consider that
> you might want to use make menuconfig to turn something on. It is a
> pretty handy interface if you work on vastly different parts of the
> kernel. Or maybe you have a defconfig that you always use for some
> platform, I think it is easier to run
> 
> make foo_config; tools/testing/kunit/kunit.py run
> 
> Then having to maintain both your defconfig and a .kunitconfig which
> is a superset of the defconfig.
> 
> Your change would make it so that you have to have a .kunitconfig for
> every test environment that you care about, and you could not as
> easily take advantage of menuconfig.

Thank you for this kind answer.  Now I understood the intention and agree with
that. :)

> 
> I think what we do now is a bit janky, and the use cases I mentioned
> are not super well supported. So I am sympathetic to what you are
> trying to do, maybe we could have a config option for it?
> 
> I think Ted and Bjorn might have opinions on this; they had some
> related opinions in the past.

I'm ok with current state, but if related discussions continue and my opinion
is required, I will join in.


Thanks,
SeongJae Park

> 

Re: Re: Re: [PATCH] kunit/kunit_kernel: Rebuild .config if .kunitconfig is modified

[David Gow]

One thing we'd like to do with kunit_tool is to make its functionality
a bit more independent: in particular, allowing the configuration,
running the kernel, and parsing the results to be done independently.

If that's the case, it may make sense for "kunit.py run" or similar to
not do anything with the .config, and to relegate that to a separate
"configuration" step, which would allow someone to modify the
configuration themselves (e.g., using make menuconfig) and re-run the
tests, but also allow the config to be explicitly regenerated when
helpful.

Exactly what that'd end up looking like (and to what extent we'd still
want to support a single command that'd do both) are still up in the
air: but I think a general "separation of concerns" like this is
probably the right path forward for kunit_tool.

Cheers,
-- David


On Tue, Feb 4, 2020 at 6:14 PM SeongJae Park <sj38.park@gmail.com> wrote:
>
> On Tue, 4 Feb 2020 16:46:06 -0800 Brendan Higgins <brendanhiggins@google.com> wrote:
>
> > Sorry for the delay.
> >
> > On Mon, Jan 27, 2020 at 10:03 PM SeongJae Park <sj38.park@gmail.com> wrote:
> > >
> > > On Mon, 27 Jan 2020 16:02:48 -0800 Brendan Higgins <brendanhiggins@google.com> wrote:
> > >
> > > > On Sat, Jan 25, 2020 at 5:59 PM <sj38.park@gmail.com> wrote:
> > > > >
> > > > > From: SeongJae Park <sjpark@amazon.de>
> > > > >
> > > > > Deletions of configs in the '.kunitconfig' is not applied because kunit
> > > > > rebuilds '.config' only if the '.config' is not a subset of the
> > > > > '.kunitconfig'.  To allow the deletions to applied, this commit modifies
> > > > > the '.config' rebuild condition to addtionally check the modified times
> > > > > of those files.
> > > >
> > > > The reason it only checks that .kunitconfig is a subset of .config is
> > > > because we don't want the .kunitconfig to remove options just because
> > > > it doesn't recognize them.
> > > >
> > > > It runs `make ARCH=um olddefconfig` on the .config that it generates
> > > > from the .kunitconfig, and most of the time that means you will get a
> > > > .config with lots of things in it that aren't in the .kunitconfig.
> > > > Consequently, nothing should ever be deleted from the .config just
> > > > because it was deleted in the .kunitconfig (unless, of course, you
> > > > change a =y to a =n or # ... is not set), so I don't see what this
> > > > change would do.
> > > >
> > > > Can you maybe provide an example?
> > >
> > > Sorry for my insufficient explanation.  I added a kunit test
> > > (SYSCTL_KUNIT_TEST) to '.kunitconfig', ran the added test, and then removed it
> > > from the file.  However, '.config' is not generated again due to the condition
> > > and therefore the test still runs.
> > >
> > > For more detail:
> > >
> > >     $ ./tools/testing/kunit/kunit.py run --defconfig --build_dir ../kunit.out/
> > >     $ echo "CONFIG_SYSCTL_KUNIT_TEST=y" >> ../kunit.out/.kunitconfig
> > >     $ ./tools/testing/kunit/kunit.py run --build_dir ../kunit.out/
> > >     $ sed -i '4d' ../kunit.out/.kunitconfig
> > >     $ ./tools/testing/kunit/kunit.py run --build_dir ../kunit.out/
> > >
> > > The 2nd line command adds sysctl kunit test and the 3rd line shows it runs the
> > > added test as expected.  Because the default kunit config contains only 3
> > > lines, The 4th line command removes the sysctl kunit test from the
> > > .kunitconfig.  However, the 5th line still run the test.
> > >
> > > This patch is for such cases.  Of course, this might make more false positives
> > > but I believe it would not be a big problem because .config generation takes no
> > > long time.  If I missed something, please let me know.
> >
> > I think I understand.
> >
> > It is intentional - currently - that KUnit doesn't generate a new
> > .config with every invocation. The reason is basically to support
> > interaction with other methods of generating .configs. Consider that
> > you might want to use make menuconfig to turn something on. It is a
> > pretty handy interface if you work on vastly different parts of the
> > kernel. Or maybe you have a defconfig that you always use for some
> > platform, I think it is easier to run
> >
> > make foo_config; tools/testing/kunit/kunit.py run
> >
> > Then having to maintain both your defconfig and a .kunitconfig which
> > is a superset of the defconfig.
> >
> > Your change would make it so that you have to have a .kunitconfig for
> > every test environment that you care about, and you could not as
> > easily take advantage of menuconfig.
>
> Thank you for this kind answer.  Now I understood the intention and agree with
> that. :)
>
> >
> > I think what we do now is a bit janky, and the use cases I mentioned
> > are not super well supported. So I am sympathetic to what you are
> > trying to do, maybe we could have a config option for it?
> >
> > I think Ted and Bjorn might have opinions on this; they had some
> > related opinions in the past.
>
> I'm ok with current state, but if related discussions continue and my opinion
> is required, I will join in.
>
>
> Thanks,
> SeongJae Park
>
> >
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20200205021428.8007-1-sj38.park%40gmail.com.

Re: Re: Re: [PATCH] kunit/kunit_kernel: Rebuild .config if .kunitconfig is modified

[Brendan Higgins]

On Wed, Feb 5, 2020 at 9:58 AM David Gow <davidgow@google.com> wrote:
>
> One thing we'd like to do with kunit_tool is to make its functionality
> a bit more independent: in particular, allowing the configuration,
> running the kernel, and parsing the results to be done independently.
>
> If that's the case, it may make sense for "kunit.py run" or similar to
> not do anything with the .config, and to relegate that to a separate
> "configuration" step, which would allow someone to modify the
> configuration themselves (e.g., using make menuconfig) and re-run the
> tests, but also allow the config to be explicitly regenerated when
> helpful.
>
> Exactly what that'd end up looking like (and to what extent we'd still
> want to support a single command that'd do both) are still up in the
> air: but I think a general "separation of concerns" like this is
> probably the right path forward for kunit_tool.

You and I have talked about splitting up kunit_tool's functionality
before. I agree with the idea.

I imagine it that we would have

- configuration
- running tests
- dmesg/TAP parsing

as separate runnable scripts. I think that would make it a lot easier
for people with various test bed setups to reuse our code in their
test harness.

Nevertheless, I think it would also be nice to have, as Ted has
previously suggested, a short easy to remember one line command that
just works; it is easily said, and much harder to do, but I think it
is at odds with the separation of functionality. I guess one solution
might just be to have these three separate tools, and then the classic
kunit.py script that combines the functionalities in a single step, or
as Ted suggested we could have some sort of default "make kunit"
command or something like that. I am not really sure what is best
here.

It doesn't address the problem of separation of functionality in
anyway, but one way we could achieve the idea of having a command that
just works, is by putting a line in MAINTAINERS file entries that have
a command that a maintainer expects a submitter to run before sending
a patch to LKML. That might at least make it possible to hack together
a single line KUnit command for every relevant MAINTAINERS entry.
(Obviously there is no reason we have to do this particular idea just
for KUnit. We could do this for other tests as well.) Russel, I think
this was your idea at LCA?

> On Tue, Feb 4, 2020 at 6:14 PM SeongJae Park <sj38.park@gmail.com> wrote:
> >
> > On Tue, 4 Feb 2020 16:46:06 -0800 Brendan Higgins <brendanhiggins@google.com> wrote:
> >
> > > Sorry for the delay.
> > >
> > > On Mon, Jan 27, 2020 at 10:03 PM SeongJae Park <sj38.park@gmail.com> wrote:
> > > >
> > > > On Mon, 27 Jan 2020 16:02:48 -0800 Brendan Higgins <brendanhiggins@google.com> wrote:
> > > >
> > > > > On Sat, Jan 25, 2020 at 5:59 PM <sj38.park@gmail.com> wrote:
> > > > > >
> > > > > > From: SeongJae Park <sjpark@amazon.de>
> > > > > >
> > > > > > Deletions of configs in the '.kunitconfig' is not applied because kunit
> > > > > > rebuilds '.config' only if the '.config' is not a subset of the
> > > > > > '.kunitconfig'.  To allow the deletions to applied, this commit modifies
> > > > > > the '.config' rebuild condition to addtionally check the modified times
> > > > > > of those files.
> > > > >
> > > > > The reason it only checks that .kunitconfig is a subset of .config is
> > > > > because we don't want the .kunitconfig to remove options just because
> > > > > it doesn't recognize them.
> > > > >
> > > > > It runs `make ARCH=um olddefconfig` on the .config that it generates
> > > > > from the .kunitconfig, and most of the time that means you will get a
> > > > > .config with lots of things in it that aren't in the .kunitconfig.
> > > > > Consequently, nothing should ever be deleted from the .config just
> > > > > because it was deleted in the .kunitconfig (unless, of course, you
> > > > > change a =y to a =n or # ... is not set), so I don't see what this
> > > > > change would do.
> > > > >
> > > > > Can you maybe provide an example?
> > > >
> > > > Sorry for my insufficient explanation.  I added a kunit test
> > > > (SYSCTL_KUNIT_TEST) to '.kunitconfig', ran the added test, and then removed it
> > > > from the file.  However, '.config' is not generated again due to the condition
> > > > and therefore the test still runs.
> > > >
> > > > For more detail:
> > > >
> > > >     $ ./tools/testing/kunit/kunit.py run --defconfig --build_dir ../kunit.out/
> > > >     $ echo "CONFIG_SYSCTL_KUNIT_TEST=y" >> ../kunit.out/.kunitconfig
> > > >     $ ./tools/testing/kunit/kunit.py run --build_dir ../kunit.out/
> > > >     $ sed -i '4d' ../kunit.out/.kunitconfig
> > > >     $ ./tools/testing/kunit/kunit.py run --build_dir ../kunit.out/
> > > >
> > > > The 2nd line command adds sysctl kunit test and the 3rd line shows it runs the
> > > > added test as expected.  Because the default kunit config contains only 3
> > > > lines, The 4th line command removes the sysctl kunit test from the
> > > > .kunitconfig.  However, the 5th line still run the test.
> > > >
> > > > This patch is for such cases.  Of course, this might make more false positives
> > > > but I believe it would not be a big problem because .config generation takes no
> > > > long time.  If I missed something, please let me know.
> > >
> > > I think I understand.
> > >
> > > It is intentional - currently - that KUnit doesn't generate a new
> > > .config with every invocation. The reason is basically to support
> > > interaction with other methods of generating .configs. Consider that
> > > you might want to use make menuconfig to turn something on. It is a
> > > pretty handy interface if you work on vastly different parts of the
> > > kernel. Or maybe you have a defconfig that you always use for some
> > > platform, I think it is easier to run
> > >
> > > make foo_config; tools/testing/kunit/kunit.py run
> > >
> > > Then having to maintain both your defconfig and a .kunitconfig which
> > > is a superset of the defconfig.
> > >
> > > Your change would make it so that you have to have a .kunitconfig for
> > > every test environment that you care about, and you could not as
> > > easily take advantage of menuconfig.
> >
> > Thank you for this kind answer.  Now I understood the intention and agree with
> > that. :)
> >
> > >
> > > I think what we do now is a bit janky, and the use cases I mentioned
> > > are not super well supported. So I am sympathetic to what you are
> > > trying to do, maybe we could have a config option for it?
> > >
> > > I think Ted and Bjorn might have opinions on this; they had some
> > > related opinions in the past.
> >
> > I'm ok with current state, but if related discussions continue and my opinion
> > is required, I will join in.
> >
> >
> > Thanks,
> > SeongJae Park
> >
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20200205021428.8007-1-sj38.park%40gmail.com.

Re: [PATCH] kunit/kunit_kernel: Rebuild .config if .kunitconfig is modified

[Russell Currey]

On Thu, Feb 6, 2020, at 7:00 AM, Brendan Higgins wrote:
> On Wed, Feb 5, 2020 at 9:58 AM David Gow <davidgow@google.com> wrote:
> >
> > One thing we'd like to do with kunit_tool is to make its functionality
> > a bit more independent: in particular, allowing the configuration,
> > running the kernel, and parsing the results to be done independently.
> >
> > If that's the case, it may make sense for "kunit.py run" or similar to
> > not do anything with the .config, and to relegate that to a separate
> > "configuration" step, which would allow someone to modify the
> > configuration themselves (e.g., using make menuconfig) and re-run the
> > tests, but also allow the config to be explicitly regenerated when
> > helpful.
> >
> > Exactly what that'd end up looking like (and to what extent we'd still
> > want to support a single command that'd do both) are still up in the
> > air: but I think a general "separation of concerns" like this is
> > probably the right path forward for kunit_tool.
> 
> You and I have talked about splitting up kunit_tool's functionality
> before. I agree with the idea.
> 
> I imagine it that we would have
> 
> - configuration
> - running tests
> - dmesg/TAP parsing
> 
> as separate runnable scripts. I think that would make it a lot easier
> for people with various test bed setups to reuse our code in their
> test harness.
> 
> Nevertheless, I think it would also be nice to have, as Ted has
> previously suggested, a short easy to remember one line command that
> just works; it is easily said, and much harder to do, but I think it
> is at odds with the separation of functionality. I guess one solution
> might just be to have these three separate tools, and then the classic
> kunit.py script that combines the functionalities in a single step, or
> as Ted suggested we could have some sort of default "make kunit"
> command or something like that. I am not really sure what is best
> here.
> 
> It doesn't address the problem of separation of functionality in
> anyway, but one way we could achieve the idea of having a command that
> just works, is by putting a line in MAINTAINERS file entries that have
> a command that a maintainer expects a submitter to run before sending
> a patch to LKML. That might at least make it possible to hack together
> a single line KUnit command for every relevant MAINTAINERS entry.
> (Obviously there is no reason we have to do this particular idea just
> for KUnit. We could do this for other tests as well.) Russel, I think
> this was your idea at LCA?

Hi Brendan, it wasn't me, it was someone in the audience during questions in my
testing talk.  I don't recall who.

They were suggesting a script like get_maintainers - i.e. get_tests - that for a
given file/patch/commit it gives you a suggested set of tests, whether that's
KUnit you can run there and then, or selftests you can run once it's booted,
or maybe external test suites that are relevant.

A single line in MAINTAINERS would probably sell that specific idea short,
but it's possibly the easiest and quickest way to get something going that
people would use.

- Russell

> 
> > On Tue, Feb 4, 2020 at 6:14 PM SeongJae Park <sj38.park@gmail.com> wrote:
> > >
> > > On Tue, 4 Feb 2020 16:46:06 -0800 Brendan Higgins <brendanhiggins@google.com> wrote:
> > >
> > > > Sorry for the delay.
> > > >
> > > > On Mon, Jan 27, 2020 at 10:03 PM SeongJae Park <sj38.park@gmail.com> wrote:
> > > > >
> > > > > On Mon, 27 Jan 2020 16:02:48 -0800 Brendan Higgins <brendanhiggins@google.com> wrote:
> > > > >
> > > > > > On Sat, Jan 25, 2020 at 5:59 PM <sj38.park@gmail.com> wrote:
> > > > > > >
> > > > > > > From: SeongJae Park <sjpark@amazon.de>
> > > > > > >
> > > > > > > Deletions of configs in the '.kunitconfig' is not applied because kunit
> > > > > > > rebuilds '.config' only if the '.config' is not a subset of the
> > > > > > > '.kunitconfig'. To allow the deletions to applied, this commit modifies
> > > > > > > the '.config' rebuild condition to addtionally check the modified times
> > > > > > > of those files.
> > > > > >
> > > > > > The reason it only checks that .kunitconfig is a subset of .config is
> > > > > > because we don't want the .kunitconfig to remove options just because
> > > > > > it doesn't recognize them.
> > > > > >
> > > > > > It runs `make ARCH=um olddefconfig` on the .config that it generates
> > > > > > from the .kunitconfig, and most of the time that means you will get a
> > > > > > .config with lots of things in it that aren't in the .kunitconfig.
> > > > > > Consequently, nothing should ever be deleted from the .config just
> > > > > > because it was deleted in the .kunitconfig (unless, of course, you
> > > > > > change a =y to a =n or # ... is not set), so I don't see what this
> > > > > > change would do.
> > > > > >
> > > > > > Can you maybe provide an example?
> > > > >
> > > > > Sorry for my insufficient explanation. I added a kunit test
> > > > > (SYSCTL_KUNIT_TEST) to '.kunitconfig', ran the added test, and then removed it
> > > > > from the file. However, '.config' is not generated again due to the condition
> > > > > and therefore the test still runs.
> > > > >
> > > > > For more detail:
> > > > >
> > > > > $ ./tools/testing/kunit/kunit.py run --defconfig --build_dir ../kunit.out/
> > > > > $ echo "CONFIG_SYSCTL_KUNIT_TEST=y" >> ../kunit.out/.kunitconfig
> > > > > $ ./tools/testing/kunit/kunit.py run --build_dir ../kunit.out/
> > > > > $ sed -i '4d' ../kunit.out/.kunitconfig
> > > > > $ ./tools/testing/kunit/kunit.py run --build_dir ../kunit.out/
> > > > >
> > > > > The 2nd line command adds sysctl kunit test and the 3rd line shows it runs the
> > > > > added test as expected. Because the default kunit config contains only 3
> > > > > lines, The 4th line command removes the sysctl kunit test from the
> > > > > .kunitconfig. However, the 5th line still run the test.
> > > > >
> > > > > This patch is for such cases. Of course, this might make more false positives
> > > > > but I believe it would not be a big problem because .config generation takes no
> > > > > long time. If I missed something, please let me know.
> > > >
> > > > I think I understand.
> > > >
> > > > It is intentional - currently - that KUnit doesn't generate a new
> > > > .config with every invocation. The reason is basically to support
> > > > interaction with other methods of generating .configs. Consider that
> > > > you might want to use make menuconfig to turn something on. It is a
> > > > pretty handy interface if you work on vastly different parts of the
> > > > kernel. Or maybe you have a defconfig that you always use for some
> > > > platform, I think it is easier to run
> > > >
> > > > make foo_config; tools/testing/kunit/kunit.py run
> > > >
> > > > Then having to maintain both your defconfig and a .kunitconfig which
> > > > is a superset of the defconfig.
> > > >
> > > > Your change would make it so that you have to have a .kunitconfig for
> > > > every test environment that you care about, and you could not as
> > > > easily take advantage of menuconfig.
> > >
> > > Thank you for this kind answer. Now I understood the intention and agree with
> > > that. :)
> > >
> > > >
> > > > I think what we do now is a bit janky, and the use cases I mentioned
> > > > are not super well supported. So I am sympathetic to what you are
> > > > trying to do, maybe we could have a config option for it?
> > > >
> > > > I think Ted and Bjorn might have opinions on this; they had some
> > > > related opinions in the past.
> > >
> > > I'm ok with current state, but if related discussions continue and my opinion
> > > is required, I will join in.
> > >
> > >
> > > Thanks,
> > > SeongJae Park
> > >
> > > >
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com.
> > > To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20200205021428.8007-1-sj38.park%40gmail.com.
>

Re: [PATCH] docs/kunit/start: Use '_KUNIT_TEST' config name suffix

[Brendan Higgins]

Sorry, I didn't see this until now.

On Mon, Jan 27, 2020 at 7:32 AM <sjpark@amazon.com> wrote:
>
> From: SeongJae Park <sjpark@amazon.de>
>
> It is recommended to use '_KUNIT_TEST' config name suffix for kunit
> tests but the example is using only '_TEST' suffix.  This commit fixes
> it to also use '_KUNIT_TEST' suffix.
>
> Signed-off-by: SeongJae Park <sjpark@amazon.de>

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

Thanks!

Re: Re: [PATCH] docs/kunit/start: Use '_KUNIT_TEST' config name suffix

[SeongJae Park]

On Wed, 19 Feb 2020 14:16:03 -0800 Brendan Higgins <brendanhiggins@google.com> wrote:

> Sorry, I didn't see this until now.

Even I also almost forgot this.

> 
> On Mon, Jan 27, 2020 at 7:32 AM <sjpark@amazon.com> wrote:
> >
> > From: SeongJae Park <sjpark@amazon.de>
> >
> > It is recommended to use '_KUNIT_TEST' config name suffix for kunit
> > tests but the example is using only '_TEST' suffix.  This commit fixes
> > it to also use '_KUNIT_TEST' suffix.
> >
> > Signed-off-by: SeongJae Park <sjpark@amazon.de>
> 
> Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

Thanks for finding this! :D


Thanks,
SeongJae Park

> 
> Thanks!
> 

Re: [PATCH] kunit/kunit_kernel: Rebuild .config if .kunitconfig is modified

[shuah]

On 2/5/20 3:09 PM, Russell Currey wrote:
> On Thu, Feb 6, 2020, at 7:00 AM, Brendan Higgins wrote:
>> On Wed, Feb 5, 2020 at 9:58 AM David Gow <davidgow@google.com> wrote:
>>>
>>> One thing we'd like to do with kunit_tool is to make its functionality
>>> a bit more independent: in particular, allowing the configuration,
>>> running the kernel, and parsing the results to be done independently.
>>>
>>> If that's the case, it may make sense for "kunit.py run" or similar to
>>> not do anything with the .config, and to relegate that to a separate
>>> "configuration" step, which would allow someone to modify the
>>> configuration themselves (e.g., using make menuconfig) and re-run the
>>> tests, but also allow the config to be explicitly regenerated when
>>> helpful.
>>>
>>> Exactly what that'd end up looking like (and to what extent we'd still
>>> want to support a single command that'd do both) are still up in the
>>> air: but I think a general "separation of concerns" like this is
>>> probably the right path forward for kunit_tool.
>>
>> You and I have talked about splitting up kunit_tool's functionality
>> before. I agree with the idea.
>>
>> I imagine it that we would have
>>
>> - configuration
>> - running tests
>> - dmesg/TAP parsing
>>
>> as separate runnable scripts. I think that would make it a lot easier
>> for people with various test bed setups to reuse our code in their
>> test harness.
>>
>> Nevertheless, I think it would also be nice to have, as Ted has
>> previously suggested, a short easy to remember one line command that
>> just works; it is easily said, and much harder to do, but I think it
>> is at odds with the separation of functionality. I guess one solution
>> might just be to have these three separate tools, and then the classic
>> kunit.py script that combines the functionalities in a single step, or
>> as Ted suggested we could have some sort of default "make kunit"
>> command or something like that. I am not really sure what is best
>> here.
>>
>> It doesn't address the problem of separation of functionality in
>> anyway, but one way we could achieve the idea of having a command that
>> just works, is by putting a line in MAINTAINERS file entries that have
>> a command that a maintainer expects a submitter to run before sending
>> a patch to LKML. That might at least make it possible to hack together
>> a single line KUnit command for every relevant MAINTAINERS entry.
>> (Obviously there is no reason we have to do this particular idea just
>> for KUnit. We could do this for other tests as well.) Russel, I think
>> this was your idea at LCA?
> 
> Hi Brendan, it wasn't me, it was someone in the audience during questions in my
> testing talk.  I don't recall who.
> 
> They were suggesting a script like get_maintainers - i.e. get_tests - that for a
> given file/patch/commit it gives you a suggested set of tests, whether that's
> KUnit you can run there and then, or selftests you can run once it's booted,
> or maybe external test suites that are relevant.
> 

I like this idea of get_tests type script that could be run separately
as well as part of check_patch or get_maintainers will serve as a
reminder or hint to patch submitter.

We have some pieces in the MAINTAINERS file now. Selftest files are
usually listed under subsystem entries. get_tests could leverage
that and we will definitely more information to for a complete set
of tests for a subsystem.

thanks,
-- Shuah