Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210123001743.1379894-1-dlatypov@google.com>
In-Reply-To: <20210123001743.1379894-1-dlatypov@google.com>
From:   David Gow <davidgow@google.com>
Date:   Fri, 29 Jan 2021 14:33:12 +0800
Message-ID: <CABVgOSkksFz62F4fGeCd8hO_hHPDaQ5AB=VRo9tSWVdrE=nPow@mail.gmail.com>
Subject: Re: [PATCH] kunit: make kunit_tool accept optional path to
 .kunitconfig fragment
To:     Daniel Latypov <dlatypov@google.com>
Cc:     Brendan Higgins <brendanhiggins@google.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        Shuah Khan <skhan@linuxfoundation.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.

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, 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
>