Re: [PATCH] kunit: make kunit_tool accept optional path to .kunitconfig fragment
On Thu, Jan 28, 2021 at 10:33 PM David Gow wrote:
>
> On Sat, Jan 23, 2021 at 8:17 AM Daniel Latypov wrote:
> >
> > Currently running tests via KUnit tool means tweaking a .kunitconfig
> > file, which you'd keep around locally and never commit.
> > This changes makes it so users can pass in a path to a kunitconfig.
> >
> > One of the imagined use cases is having kunitconfig fragments in-tree
> > to formalize interesting sets of tests for features/subsystems, e.g.
> > $ ./tools/testing/kunit/kunit.py run fs/ext4/kunitconfig
> >
> > For now, this hypothetical fs/ext4/kunitconfig would contain
> > CONFIG_KUNIT=y
> > CONFIG_EXT4_FS=y
> > CONFIG_EXT4_KUNIT_TESTS=y
> >
> > At the moment, it's not hard to manually whip up this file, but as more
> > and more tests get added, this will get tedious.
> >
> > It also opens the door to documenting how to run all the tests relevant
> > to a specific subsystem or feature as a simple one-liner.
> >
> > This can be seen as an analogue to tools/testing/selftests/*/config
> > But in the case of KUnit, the tests live in the same directory as the
> > code-under-test, so it feels more natural to allow the kunitconfig
> > fragments to live anywhere. (Though, people could create a separate
> > directory if wanted; this patch imposes no restrictions on the path).
> >
> > Signed-off-by: Daniel Latypov
> > ---
>
> Really glad this is finally happening. I tried it out, and it seemed
> to work pretty well.
>
> I was wondering whether a positional argument like this was best, or
> whether it'd be better to have an explicitly named argument
> (--kunitconfig=path). Thinking about it though, I'm quite happy with
> having this as-is: the only real other contender for a coveted
> positional argument spot would've been the name of a test or test
> suite (e.g., kunit.py run ext4_inode_test), and that's not really
> possible with the kunit_tool architecture as-is.
Same, I was on the fence about this for a good while.
I had originally intended to make it a flag, but
* as you noted, that would require bigger changes to kunit_tool, but
also KUnit (the C code) itself to handle this.
* I felt that the kunitconfig fragment essentially takes the place of it.
* E.g. If I want to run a specific test, I can manually create a
fragment for just that.
* It's sadly more work than just specifying a single test for
bisection, but it would work.
That said, adding in support for running specific tests/suites by name
seems like it would be a somewhat smaller change than I thought.
So I might lean more towards making this a flag now.
Will wait to get Brendan's opinion and then send out a v2.
>
> One other comment below (should this work for kunit.py config?),
> otherwise it looks good.
>
> -- David
>
> > tools/testing/kunit/kunit.py | 9 ++---
> > tools/testing/kunit/kunit_kernel.py| 12
> > tools/testing/kunit/kunit_tool_test.py | 25 +
> > 3 files changed, 39 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> > index e808a47c839b..3204a23bd16e 100755
> > --- a/tools/testing/kunit/kunit.py
> > +++ b/tools/testing/kunit/kunit.py
> > @@ -188,6 +188,9 @@ def add_build_opts(parser) -> None:
> > help='As in the make command, "Specifies the
> > number of '
> > 'jobs (commands) to run simultaneously."',
> > type=int, default=8, metavar='jobs')
> > + parser.add_argument('kunitconfig',
> > +help='Path to Kconfig fragment that enables
> > KUnit tests',
> > +type=str, nargs='?', metavar='kunitconfig')
> >
>
> Should this maybe be in add_common_opts()? I'd assume that we want
> kunit.py config to accept this custom kunitconfig path as well.
Good point.
Moved it over into add_common_opts() and including this minimal test
case as well since `kunit.py config` is probably a very key command
that should work with custom fragments.
diff --git a/tools/testing/kunit/kunit_tool_test.py
b/tools/testing/kunit/kunit_tool_test.py
index 533fe41b5123..a74877ee2c90 100755
--- a/tools/testing/kunit/kunit_tool_test.py
+++ b/tools/testing/kunit/kunit_tool_test.py
@@ -424,5 +424,12 @@ class KUnitMainTest(unittest.TestCase):
# Just verify that we parsed and initialized it correctly here.
mock_linux_init.assert_called_once_with('.kunit',
kunitconfig_path='mykunitconfig')
+ @mock.patch.object(kunit_kernel, 'LinuxSourceTree')
+ def test_config_kunitconfig(self, mock_linux_init):
+ mock_linux_init.return_value = self.linux_source_mock
+ kunit.main(['config', 'mykunitconfig'])
+ # Just verify that we parsed and initialized it correctly here.
+ mock_linux_init.assert_called_once_with('.kunit',
kunitconfig_path='mykunitconfig')
+
if __name__ == '__main__':
Re: [PATCH] kunit: make kunit_tool accept optional path to .kunitconfig fragment
On Sat, Jan 23, 2021 at 8:17 AM Daniel Latypov wrote:
>
> Currently running tests via KUnit tool means tweaking a .kunitconfig
> file, which you'd keep around locally and never commit.
> This changes makes it so users can pass in a path to a kunitconfig.
>
> One of the imagined use cases is having kunitconfig fragments in-tree
> to formalize interesting sets of tests for features/subsystems, e.g.
> $ ./tools/testing/kunit/kunit.py run fs/ext4/kunitconfig
>
> For now, this hypothetical fs/ext4/kunitconfig would contain
> CONFIG_KUNIT=y
> CONFIG_EXT4_FS=y
> CONFIG_EXT4_KUNIT_TESTS=y
>
> At the moment, it's not hard to manually whip up this file, but as more
> and more tests get added, this will get tedious.
>
> It also opens the door to documenting how to run all the tests relevant
> to a specific subsystem or feature as a simple one-liner.
>
> This can be seen as an analogue to tools/testing/selftests/*/config
> But in the case of KUnit, the tests live in the same directory as the
> code-under-test, so it feels more natural to allow the kunitconfig
> fragments to live anywhere. (Though, people could create a separate
> directory if wanted; this patch imposes no restrictions on the path).
>
> Signed-off-by: Daniel Latypov
> ---
Really glad this is finally happening. I tried it out, and it seemed
to work pretty well.
I was wondering whether a positional argument like this was best, or
whether it'd be better to have an explicitly named argument
(--kunitconfig=path). Thinking about it though, I'm quite happy with
having this as-is: the only real other contender for a coveted
positional argument spot would've been the name of a test or test
suite (e.g., kunit.py run ext4_inode_test), and that's not really
possible with the kunit_tool architecture as-is.
One other comment below (should this work for kunit.py config?),
otherwise it looks good.
-- David
> tools/testing/kunit/kunit.py | 9 ++---
> tools/testing/kunit/kunit_kernel.py| 12
> tools/testing/kunit/kunit_tool_test.py | 25 +
> 3 files changed, 39 insertions(+), 7 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> index e808a47c839b..3204a23bd16e 100755
> --- a/tools/testing/kunit/kunit.py
> +++ b/tools/testing/kunit/kunit.py
> @@ -188,6 +188,9 @@ def add_build_opts(parser) -> None:
> help='As in the make command, "Specifies the
> number of '
> 'jobs (commands) to run simultaneously."',
> type=int, default=8, metavar='jobs')
> + parser.add_argument('kunitconfig',
> +help='Path to Kconfig fragment that enables
> KUnit tests',
> +type=str, nargs='?', metavar='kunitconfig')
>
Should this maybe be in add_common_opts()? I'd assume that we want
kunit.py config to accept this custom kunitconfig path as well.
> def add_exec_opts(parser) -> None:
> parser.add_argument('--timeout',
> @@ -256,7 +259,7 @@ def main(argv, linux=None):
> os.mkdir(cli_args.build_dir)
>
> if not linux:
> - linux =
> kunit_kernel.LinuxSourceTree(cli_args.build_dir)
> + linux =
> kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> kunitconfig_path=cli_args.kunitconfig)
>
> request = KunitRequest(cli_args.raw_output,
>cli_args.timeout,
> @@ -274,7 +277,7 @@ def main(argv, linux=None):
> os.mkdir(cli_args.build_dir)
>
> if not linux:
> - linux =
> kunit_kernel.LinuxSourceTree(cli_args.build_dir)
> + linux =
> kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> kunitconfig_path=cli_args.kunitconfig)
>
> request = KunitConfigRequest(cli_args.build_dir,
> cli_args.make_options)
> @@ -286,7 +289,7 @@ def main(argv, linux=None):
> sys.exit(1)
> elif cli_args.subcommand == 'build':
> if not linux:
> - linux =
> kunit_kernel.LinuxSourceTree(cli_args.build_dir)
> + linux =
> kunit_kernel.LinuxSourceTree(cli_args.build_dir,
> kunitconfig_path=cli_args.kunitconfig)
>
> request = KunitBuildRequest(cli_args.jobs,
> cli_args.build_dir,
> diff --git a/tools/testing/kunit/kunit_kernel.py
> b/tools/testing/kunit/kunit_kernel.py
> index 2076a5a2d060..0b461663e7d9 100644
> --- a/tools/testing/kunit/kunit_kernel.py
> +++ b/tools/testing/kunit/kunit_kernel.py
> @@ -123,7 +123,7 @@ def get_outfile_path(build_dir) -> str:
> class LinuxSourceTree(object):
> """Represents a Linux kernel source tree with KUnit tests."""
>
> - def __init__(self, build_dir: str, load_config=True,
>
[PATCH] kunit: make kunit_tool accept optional path to .kunitconfig fragment
Currently running tests via KUnit tool means tweaking a .kunitconfig
file, which you'd keep around locally and never commit.
This changes makes it so users can pass in a path to a kunitconfig.
One of the imagined use cases is having kunitconfig fragments in-tree
to formalize interesting sets of tests for features/subsystems, e.g.
$ ./tools/testing/kunit/kunit.py run fs/ext4/kunitconfig
For now, this hypothetical fs/ext4/kunitconfig would contain
CONFIG_KUNIT=y
CONFIG_EXT4_FS=y
CONFIG_EXT4_KUNIT_TESTS=y
At the moment, it's not hard to manually whip up this file, but as more
and more tests get added, this will get tedious.
It also opens the door to documenting how to run all the tests relevant
to a specific subsystem or feature as a simple one-liner.
This can be seen as an analogue to tools/testing/selftests/*/config
But in the case of KUnit, the tests live in the same directory as the
code-under-test, so it feels more natural to allow the kunitconfig
fragments to live anywhere. (Though, people could create a separate
directory if wanted; this patch imposes no restrictions on the path).
Signed-off-by: Daniel Latypov
---
tools/testing/kunit/kunit.py | 9 ++---
tools/testing/kunit/kunit_kernel.py| 12
tools/testing/kunit/kunit_tool_test.py | 25 +
3 files changed, 39 insertions(+), 7 deletions(-)
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
index e808a47c839b..3204a23bd16e 100755
--- a/tools/testing/kunit/kunit.py
+++ b/tools/testing/kunit/kunit.py
@@ -188,6 +188,9 @@ def add_build_opts(parser) -> None:
help='As in the make command, "Specifies the
number of '
'jobs (commands) to run simultaneously."',
type=int, default=8, metavar='jobs')
+ parser.add_argument('kunitconfig',
+help='Path to Kconfig fragment that enables KUnit
tests',
+type=str, nargs='?', metavar='kunitconfig')
def add_exec_opts(parser) -> None:
parser.add_argument('--timeout',
@@ -256,7 +259,7 @@ def main(argv, linux=None):
os.mkdir(cli_args.build_dir)
if not linux:
- linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir)
+ linux =
kunit_kernel.LinuxSourceTree(cli_args.build_dir,
kunitconfig_path=cli_args.kunitconfig)
request = KunitRequest(cli_args.raw_output,
cli_args.timeout,
@@ -274,7 +277,7 @@ def main(argv, linux=None):
os.mkdir(cli_args.build_dir)
if not linux:
- linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir)
+ linux =
kunit_kernel.LinuxSourceTree(cli_args.build_dir,
kunitconfig_path=cli_args.kunitconfig)
request = KunitConfigRequest(cli_args.build_dir,
cli_args.make_options)
@@ -286,7 +289,7 @@ def main(argv, linux=None):
sys.exit(1)
elif cli_args.subcommand == 'build':
if not linux:
- linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir)
+ linux =
kunit_kernel.LinuxSourceTree(cli_args.build_dir,
kunitconfig_path=cli_args.kunitconfig)
request = KunitBuildRequest(cli_args.jobs,
cli_args.build_dir,
diff --git a/tools/testing/kunit/kunit_kernel.py
b/tools/testing/kunit/kunit_kernel.py
index 2076a5a2d060..0b461663e7d9 100644
--- a/tools/testing/kunit/kunit_kernel.py
+++ b/tools/testing/kunit/kunit_kernel.py
@@ -123,7 +123,7 @@ def get_outfile_path(build_dir) -> str:
class LinuxSourceTree(object):
"""Represents a Linux kernel source tree with KUnit tests."""
- def __init__(self, build_dir: str, load_config=True,
defconfig=DEFAULT_KUNITCONFIG_PATH) -> None:
+ def __init__(self, build_dir: str, load_config=True,
kunitconfig_path='') -> None:
signal.signal(signal.SIGINT, self.signal_handler)
self._ops = LinuxSourceTreeOperations()
@@ -131,9 +131,13 @@ class LinuxSourceTree(object):
if not load_config:
return
- kunitconfig_path = get_kunitconfig_path(build_dir)
- if not os.path.exists(kunitconfig_path):
- shutil.copyfile(defconfig, kunitconfig_path)
+ if kunitconfig_path:
+ if not os.path.exists(kunitconfig_path):
+ raise ConfigError(f'Specified kunitconfig
({kunitconfig_path}) does not exist')
+ else:
+ kunitconfig_path = get_kunitconfig_path(build_dir)
+ if not os.path.exists(kunitconfig_path):
+
