On Thu, Jan 28, 2021 at 10:33 PM David Gow <davidgow@xxxxxxxxxx> wrote:
>
> On Sat, Jan 23, 2021 at 8:17 AM Daniel Latypov <dlatypov@xxxxxxxxxx> 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 <dlatypov@xxxxxxxxxx>
> > ---
>
> 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__':
unittest.main()
>
> > 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):
> > + shutil.copyfile(DEFAULT_KUNITCONFIG_PATH, kunitconfig_path)
> >
> > self._kconfig = kunit_config.Kconfig()
> > self._kconfig.read_from_file(kunitconfig_path)
> > diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
> > index b593f4448e83..533fe41b5123 100755
> > --- a/tools/testing/kunit/kunit_tool_test.py
> > +++ b/tools/testing/kunit/kunit_tool_test.py
> > @@ -12,6 +12,7 @@ from unittest import mock
> > import tempfile, shutil # Handling test_tmpdir
> >
> > import json
> > +import signal
> > import os
> >
> > import kunit_config
> > @@ -250,6 +251,23 @@ class KUnitParserTest(unittest.TestCase):
> > result.status)
> > self.assertEqual('kunit-resource-test', result.suites[0].name)
> >
> > +class LinuxSourceTreeTest(unittest.TestCase):
> > +
> > + def setUp(self):
> > + mock.patch.object(signal, 'signal').start()
> > + self.addCleanup(mock.patch.stopall)
> > +
> > + def test_invalid_kunitconfig(self):
> > + with self.assertRaisesRegex(kunit_kernel.ConfigError, 'nonexistent.* does not exist'):
> > + kunit_kernel.LinuxSourceTree('', kunitconfig_path='/nonexistent_file')
> > +
> > + def test_valid_kunitconfig(self):
> > + with tempfile.NamedTemporaryFile('wt') as kunitconfig:
> > + tree = kunit_kernel.LinuxSourceTree('', kunitconfig_path=kunitconfig.name)
> > +
> > + # TODO: add more test cases.
> > +
> > +
> > class KUnitJsonTest(unittest.TestCase):
> >
> > def _json_for(self, log_file):
> > @@ -399,5 +417,12 @@ class KUnitMainTest(unittest.TestCase):
> > self.linux_source_mock.run_kernel.assert_called_once_with(build_dir=build_dir, timeout=300)
> > self.print_mock.assert_any_call(StrContains('Testing complete.'))
> >
> > + @mock.patch.object(kunit_kernel, 'LinuxSourceTree')
> > + def test_run_kunitconfig(self, mock_linux_init):
> > + mock_linux_init.return_value = self.linux_source_mock
> > + kunit.main(['run', '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__':
> > unittest.main()
> >
> > base-commit: 2b8fdbbf1c616300312f71fe5b21fe8f03129950
> > --
> > 2.30.0.280.ga3ce27912f-goog
> >
