Re: [PATCH] kunit: make kunit_tool accept optional path to .kunitconfig fragment

On Thu, Jan 28, 2021 at 10:33 PM David Gow davidgow@google.com wrote:

On Sat, Jan 23, 2021 at 8:17 AM Daniel Latypov dlatypov@google.com 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@google.com

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