Re: [PATCH 18/22] tests/functional: add 'archive_extract' to QemuBaseTest

[Daniel P . Berrangé]

On Mon, Dec 02, 2024 at 01:52:46PM +0100, Thomas Huth wrote:
> On 02/12/2024 13.13, Daniel P. Berrangé wrote:
> > On Mon, Dec 02, 2024 at 11:30:28AM +0100, Thomas Huth wrote:
> > > On 29/11/2024 18.31, Daniel P. Berrangé wrote:
> > > > This helper wrappers utils.archive_extract, forcing the use of the
> > > > scratch directory, to ensure any extracted files are cleaned at test
> > > > termination. If a specific member is requested, then the path to the
> > > > extracted file is also returned.
> > > > 
> > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > > ---
> > > >    tests/functional/qemu_test/testcase.py | 36 
> > > > ++++++++++++++++++++++++++
> > > >    1 file changed, 36 insertions(+)
> > > > 
> > > > diff --git a/tests/functional/qemu_test/testcase.py 
> > > > b/tests/functional/qemu_test/testcase.py
> > > > index 2f32742387..31d06f0172 100644
> > > > --- a/tests/functional/qemu_test/testcase.py
> > > > +++ b/tests/functional/qemu_test/testcase.py
> > > > @@ -28,6 +28,8 @@
> > > >    from .asset import Asset
> > > >    from .cmd import run_cmd
> > > >    from .config import BUILD_DIR
> > > > +from .utils import (archive_extract as utils_archive_extract,
> > > > +                    guess_archive_format)
> > > >    class QemuBaseTest(unittest.TestCase):
> > > > @@ -39,6 +41,40 @@ class QemuBaseTest(unittest.TestCase):
> > > >        log = None
> > > >        logdir = None
> > > > +    '''
> > > > +    @params archive: filename, Asset, or file-like object to extract
> > > > +    @params sub_dir: optional sub-directory to extract into
> > > > +    @params member: optional member file to limit extraction to
> > > > +
> > > > +    Extracts @archive into the scratch directory, or a
> > > > +    directory beneath named by @sub_dir. All files are
> > > > +    extracted unless @member specifies a limit.
> > > > +
> > > > +    If @member is non-None, returns the fully qualified
> > > > +    path to @member
> > > > +    '''
> > > > +    def archive_extract(self, archive, format=None, sub_dir=None, 
> > > > member=None):
> > > > +        if type(archive) == Asset:
> > > > +            if format is None:
> > > > +                format = guess_archive_format(archive.url)
> > > > +            archive = archive.fetch()
> > > > +        elif format is None:
> > > > +            format = guess_archive_format(archive)
> > > > +
> > > > +        if member is not None:
> > > > +            if os.path.isabs(member):
> > > > +                member = os.path.relpath(member, '/')
> > > > +
> > > > +        if sub_dir is None:
> > > > +            utils_archive_extract(archive, self.scratch_file(), 
> > > > format, member)
> > > > +        else:
> > > > +            utils_archive_extract(archive, self.scratch_file(sub_dir),
> > > > +                                  format, member)
> > > > +
> > > > +        if member is not None:
> > > > +            return self.scratch_file(member)
> > > > +        return None
> > > 
> > > Ah, ok, so the guessing is done here ...
> > > 
> > > But somehow it feels wrong to have a "archive_extract" function in the
> > > QemuBaseTest class that also does asset fetching under the hood.
> > > 
> > > Could you maybe rather move this into the asset.py file and rename the
> > > function to "fetch_and_extract()" to make it clearer what it does?
> > 
> > We can't move it into asset.py because not all callers are passing in an
> > Asset object - there are some cases where we've just got a local file.
> > eg when the asset we extracted contains other archives that need to be
> > extracted.
> 
> Couldn't those spots rather use the function from utils.py directly for the
> second extraction?
> 
> Anyway, I guess now it's likely better to keep it here since you want to
> have access to self.scratch_file() in this function, too.
> But maybe you could still at least rename it to fetch_and_extract() to make
> it more obvious what it all about?

I wanted to de-emphasize the 'fetch' concept, to make use of assets look
and work no different from local files.

In practice they are all local files, with no fetching taking place, as
the test suite arranges for everything to be pre-fetched into the local
cache. THe only time a "fetch" would ever happen during execution is if
you had directly invoked a python file, with an empt ycache.

IOW, from the POV of the executing test case, an Asset object is no more
than a wrapper around a local filename.

Hmm, in fact I wonder if we shouldn't implement the 'str' method to
directly return the local filename, so we can pass Asset object instances
to any method that expects a filename.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|