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
+
def socket_dir(self):
if self.socketdir is None:
self.socketdir = tempfile.TemporaryDirectory(
--
2.46.0
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? Thomas
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 :|
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
On 02/12/2024 13.52, 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. FWIW, I think I just hit another reason for having the function in testcase.py: We want to skip tests if the download of an asset failed with a temporary server failure - for this we need a test object reference for calling skipTest(). Checking for HTTP errors within assets.py does not work since we cannot call skipTest() there without a reference to a test object. So please keep the function in testcase.py! Thomas
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 :|
© 2016 - 2024 Red Hat, Inc.