Date:   Wed, 2 Dec 2020 12:41:35 +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 Wed, Dec 2, 2020 at 3:00 AM Daniel Latypov <dlatypov@...gle.com> wrote:
>
> On Mon, Nov 30, 2020 at 11:33 PM David Gow <davidgow@...gle.com> wrote:
> >
> > 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?
>
> Correct.
> Because it actually returns a relative path.
>
> (I forgot that I called out that get_absolute_path() gives relative
> paths in the last patch, and not this one. I can update the commit
> desc if we want a v2 of this)
>
> >
> > >
> > > 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
>
> a) is doable, but would require plumbing cwd=get_kernel_root_path() to
> all the subprocess calls to make, etc.
> I'm not sure fixing a internal test-only issue necessarily justifies
> taking that path instead of the easier "just add a chdir" we opted for
> in 5578d008d9e0 ("kunit: tool: fix running kunit_tool from outside
> kernel tree").
>
> > (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.
>
> b) is not that simple, sadly.
>
> Say I invoke
> $ python3 kunit_tool_test.py
> then __file__ = kunit_tool_test.py.
>
> So __file__ is a relative path, but the code assumed it was an
> absolute one and any change of directory breaks things.
> Working around that would require something like caching the result of
> os.path.abspath(__file__) somewhere.

So, to clarify, __file__ is a relative path based on the cwd when the
script is initially run, right?

In any case, caching the result of os.path.abspath(__file__) seems
like the most sensible solution to me. There's global state anyway
(the current directory), we might as well have it in an explicit
variable, IMHO.
>
> I can see the point about not mocking out something like os.chdir().
> But on the other hand, do we have any other legitimate reason to call
> it besides that one place in kunit.py?
> If we do have something that relies on doing an os.chdir(), it should
> ideally notice that it didn't work and manifest in a unit test failure
> somehow.

Certainly there doesn't seem to be any other need to chdir() in
kunit_tool at the moment, but I could see us doing so when adding
other features. More to the point, if both kunit.py and
kunit_tool_test.py rely on or manipulate the current directory as part
of their state, that seems like it's asking for some trouble down the
line.

If we use an absolute path for the test data, that's something that
seems unlikely to ever need further changes or cause issues.
>
> Alternatively, we could make get_kernel_root_path() return ""/None to
> avoid the chdir call.
> kunit.py:       if get_kernel_root_path():
> kunit.py:               os.chdir(get_kernel_root_path())
>
> There'd be no adverse affects atm for stubbing that out, and I don't forsee any.
> But if we want to be even safer, then perhaps we have
>
> def chdir_to_kernel_root():
>    ...
>
> and mock out just that func in the unit test?

I'd be okay with this, even if I'd prefer us to use an absolute path
for the test_data as well. Having something like this might even give
us the opportunity to verify that we're actually trying to change to
the kernel directory in cases where we need to, but that seems like
it's out-of-scope for a simple fix.

-- David

Powered by blists - more mailing lists