linux-kernel - Re: [PATCH 2/5] kunit: tool: fix unit test so it can run from non-root dir

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CABVgOSmX3foOr6XJhQ_goYabFEg8qNYoQ+5O7JPRW=gLmh=OQA@mail.gmail.com>
Date:   Tue, 1 Dec 2020 15:32:59 +0800
From:   David Gow <davidgow@...gle.com>
To:     Daniel Latypov <dlatypov@...gle.com>
Cc:     Brendan Higgins <brendanhiggins@...gle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH 2/5] kunit: tool: fix unit test so it can run from
 non-root dir

On Tue, Dec 1, 2020 at 7:33 AM Daniel Latypov <dlatypov@...gle.com> wrote:
>
> get_absolute_path() makes an attempt to allow for this.
> But that doesn't work as soon as os.chdir() gets called.

Can we explain why this doesn't work? It's because the test_data/
files are accessed with relative paths, so chdir breaks access to
them, right?

>
> So make it so that os.chdir() does nothing to avoid this.
>
> Note: mock.patch.object() doesn't seem to work in setUpModule(), hence
> the introduction of a new base class instead.
>
> Fixes: 5578d008d9e0 ("kunit: tool: fix running kunit_tool from outside kernel tree")
> Signed-off-by: Daniel Latypov <dlatypov@...gle.com>
> ---

I don't like this: disabling a real system call seems like overkill to
work around a path issue like this.

Wouldn't it be better to either:
(a) stop kunit_tool from needing to chdir entirely; or
(b) have get_absolute_path() / test_data_path() produce an absolute path.

The latter really seems like the most sensible approach: have the
script read its own path and read files relative to that.

Cheers,
-- David


>  tools/testing/kunit/kunit_tool_test.py | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
> index 3fbe1acd531a..9f1f1e1b772a 100755
> --- a/tools/testing/kunit/kunit_tool_test.py
> +++ b/tools/testing/kunit/kunit_tool_test.py
> @@ -32,7 +32,13 @@ def tearDownModule():
>  def get_absolute_path(path):
>         return os.path.join(os.path.dirname(__file__), path)
>
> -class KconfigTest(unittest.TestCase):
> +class KUnitTest(unittest.TestCase):
> +       """Contains common setup, like stopping main() from calling chdir."""
> +       def setUp(self):
> +               mock.patch.object(os, 'chdir').start()
> +               self.addCleanup(mock.patch.stopall)
> +
> +class KconfigTest(KUnitTest):
>
>         def test_is_subset_of(self):
>                 kconfig0 = kunit_config.Kconfig()
> @@ -88,7 +94,7 @@ class KconfigTest(unittest.TestCase):
>                 self.assertEqual(actual_kconfig.entries(),
>                                  expected_kconfig.entries())
>
> -class KUnitParserTest(unittest.TestCase):
> +class KUnitParserTest(KUnitTest):
>
>         def assertContains(self, needle, haystack):
>                 for line in haystack:
> @@ -250,7 +256,7 @@ class KUnitParserTest(unittest.TestCase):
>                                 result.status)
>                         self.assertEqual('kunit-resource-test', result.suites[0].name)
>
> -class KUnitJsonTest(unittest.TestCase):
> +class KUnitJsonTest(KUnitTest):
>
>         def _json_for(self, log_file):
>                 with(open(get_absolute_path(log_file))) as file:
> @@ -285,8 +291,9 @@ class StrContains(str):
>         def __eq__(self, other):
>                 return self in other
>
> -class KUnitMainTest(unittest.TestCase):
> +class KUnitMainTest(KUnitTest):
>         def setUp(self):
> +               super().setUp()
>                 path = get_absolute_path('test_data/test_is_test_passed-all_passed.log')
>                 with open(path) as file:
>                         all_passed_log = file.readlines()
> --
> 2.29.2.454.gaff20da3a2-goog
>