Hi Wainer, William, Cleber, On 7/8/21 3:17 AM, Cleber Rosa wrote: > > On 7/6/21 9:17 AM, Eric Auger wrote: >> From: Willian Rampazzo <willi...@redhat.com> >> >> As the KNOWN_DISTROS grows, more loosely methods will be created in >> the avocado_qemu/__init__.py file. >> >> Let's refactor the code so that KNOWN_DISTROS and related methods are >> packaged in a class >> >> Signed-off-by: Wainer dos Santos Moschetta <waine...@redhat.com> >> Signed-off-by: Eric Auger <eric.au...@redhat.com> >> --- >> tests/acceptance/avocado_qemu/__init__.py | 74 +++++++++++++---------- >> 1 file changed, 42 insertions(+), 32 deletions(-) >> >> diff --git a/tests/acceptance/avocado_qemu/__init__.py >> b/tests/acceptance/avocado_qemu/__init__.py >> index 81ac90bebb..af93cd63ea 100644 >> --- a/tests/acceptance/avocado_qemu/__init__.py >> +++ b/tests/acceptance/avocado_qemu/__init__.py >> @@ -299,29 +299,43 @@ def ssh_command(self, command): >> f'Guest command failed: {command}') >> return stdout_lines, stderr_lines >> +class LinuxDistro: >> + """Represents a Linux distribution >> > > > I definitely like the idea. > > >> -#: A collection of known distros and their respective image checksum >> -KNOWN_DISTROS = { >> - 'fedora': { >> - '31': { >> - 'x86_64': >> - {'checksum': >> 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'}, >> - 'aarch64': >> - {'checksum': >> '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'}, >> - 'ppc64': >> - {'checksum': >> '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'}, >> - 's390x': >> - {'checksum': >> '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'}, >> + Holds information of known distros. >> + """ >> + #: A collection of known distros and their respective image >> checksum >> + KNOWN_DISTROS = { >> + 'fedora': { >> + '31': { >> + 'x86_64': >> + {'checksum': >> 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'}, >> + 'ppc64': >> + {'checksum': >> '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'}, >> + 's390x': >> + {'checksum': >> '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'}, >> } >> } >> } >> + def __init__(self, name, version, arch): >> + self.name = name >> + self.version = version >> + self.arch = arch > > > This looks a lot like > https://github.com/avocado-framework/avocado/blob/f0996dafefa412c77c221c2d1a6fafdcba1c97b7/avocado/utils/distro.py#L34 > , although admittedly, their goals are very different. > > > As a next step, in the future, I'd consider separating the data from > the actual class and having it the LinuxDistro instances, helped by a > registry. Something like: > > > class LinuxDistroRegistry: > > def __init__(self): > self.distros = set() > > def register(self, linux_distro): > > self.distros.add(linux_distro) > > def query(self, **kwargs): > > ... > > > registry = LinuxDistroRegistry() > > registry.register(LinuxDistro('fedora', '31', 'x86_64', > 'deadbeefdeadbeef')) > > registry.register(LinuxDistro('fedora', '31', 'aarch64', > 'beefdeadbeefdead')) > > checksum = registry.query(name='fedora', version='31', > arch='x86_64').checksum > > >> + try: >> + self._info = >> self.KNOWN_DISTROS.get(name).get(version).get(arch) > > > The `AttributeError` that could be caught at the removed > `get_known_distro_checksum()` function, could come from any of the > `.get()`s returning `None`, which in turn would not have a `.get()` > attribute. > > But now, if there's a "name", then a "version", but no "arch" entry, > this line will set `self._info` to `None`. This is manifested if you > try to run a test that tries to find an aarch64 distro, such as: > > ./tests/venv/bin/avocado run > tests/acceptance/boot_linux.py:BootLinuxAarch64.test_virt_tcg_gicv2 > > > It will result in: > > > 20:38:18 ERROR| Reproduced traceback from: > /var/lib/users/cleber/build/qemu/tests/venv/lib64/python3.9/site-packages/avocado/core/test.py:756 > 20:38:18 ERROR| Traceback (most recent call last): > 20:38:18 ERROR| File > "/var/lib/users/cleber/build/qemu/tests/acceptance/avocado_qemu/__init__.py", > line 426, in download_boot > 20:38:18 ERROR| checksum=self.distro.checksum, > 20:38:18 ERROR| File > "/var/lib/users/cleber/build/qemu/tests/acceptance/avocado_qemu/__init__.py", > line 334, in checksum > 20:38:18 ERROR| return self._info.get('checksum', None) > 20:38:18 ERROR| AttributeError: 'NoneType' object has no attribute 'get' > 20:38:18 ERROR| > 20:38:18 ERROR| During handling of the above exception, another > exception occurred: > 20:38:18 ERROR| > 20:38:18 ERROR| Traceback (most recent call last): > 20:38:18 ERROR| File > "/var/lib/users/cleber/build/qemu/tests/acceptance/avocado_qemu/__init__.py", > line 387, in setUp > 20:38:18 ERROR| self.set_up_boot() > 20:38:18 ERROR| File > "/var/lib/users/cleber/build/qemu/tests/acceptance/avocado_qemu/__init__.py", > line 455, in set_up_boot > 20:38:18 ERROR| path = self.download_boot() > 20:38:18 ERROR| File > "/var/lib/users/cleber/build/qemu/tests/acceptance/avocado_qemu/__init__.py", > line 431, in download_boot > 20:38:18 ERROR| self.cancel('Failed to download/prepare boot image') > 20:38:18 ERROR| File > "/var/lib/users/cleber/build/qemu/tests/venv/lib64/python3.9/site-packages/avocado/core/test.py", > line 988, in cancel > 20:38:18 ERROR| raise exceptions.TestCancel(message) > 20:38:18 ERROR| avocado.core.exceptions.TestCancel: Failed to > download/prepare boot image
I am not sufficiently expert on the test infra and python to be really efficient fixing that. Can anyone help quickly to target the soft freeze? Otherwise, today I will drop that patch and restore the code I had in v4, just based on Cleber series. I think the refactoring can happen later... Thanks Eric > > > Cheers, > > - Cleber. >