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

[Thomas Huth <thuth@redhat.com>]

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?

  Thomas


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