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

[Daniel P . Berrangé]

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.

Per comments on the previous patch though, I could push this logic down
into the lower method.

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