Re: [PATCH 2/3] Acceptance Tests: move definition of distro checksums to the framework

[Wainer dos Santos Moschetta <wainersm@redhat.com>]

Hi,

On 4/19/21 3:35 PM, Cleber Rosa wrote:
> On Mon, Apr 19, 2021 at 12:25:44PM -0300, Wainer dos Santos Moschetta wrote:
>> Hi,
>>
>> On 4/14/21 7:14 PM, Cleber Rosa wrote:
>>> Instead of having, by default, the checksum in the tests, and the
>>> definition of tests in the framework, let's keep them together.
>>>
>>> A central definition for distributions is available, and it should
>>> allow other known distros to be added more easily.
>>>
>>> No behavior change is expected here, and tests can still define
>>> a distro_checksum value if for some reason they want to override
>>> the known distribution information.
>>>
>>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>>> ---
>>>    tests/acceptance/avocado_qemu/__init__.py | 34 +++++++++++++++++++++--
>>>    tests/acceptance/boot_linux.py            |  8 ------
>>>    2 files changed, 32 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/tests/acceptance/avocado_qemu/__init__.py b/tests/acceptance/avocado_qemu/__init__.py
>>> index aae1e5bbc9..97093614d9 100644
>>> --- a/tests/acceptance/avocado_qemu/__init__.py
>>> +++ b/tests/acceptance/avocado_qemu/__init__.py
>>> @@ -299,6 +299,30 @@ def ssh_command(self, command):
>>>            return stdout_lines, stderr_lines
>>> +#: A collection of known distros and their respective image checksum
>>> +KNOWN_DISTROS = {
>> Do you plan to expand that mapping to record values other than checksums?
>> Otherwise it could be named KNOWN_DISTROS_CHECKSUMS.
>>
> Let's just say I had an intuition about it being used for other
> purposes.  Talking to Eric Auger earlier this morning, he will expand
> this mapping with default kernel args distros, so that he can *add*
> to the common args.
Ok
>
>>> +    'fedora': {
>>> +        '31': {
>>> +            'x86_64':
>>> +            {'checksum': 'e3c1b309d9203604922d6e255c2c5d098a309c2d46215d8fc026954f3c5c27a0'},
>>> +            'aarch64':
>>> +            {'checksum': '1e18d9c0cf734940c4b5d5ec592facaed2af0ad0329383d5639c997fdf16fe49'},
>>> +            'ppc64':
>>> +            {'checksum': '7c3528b85a3df4b2306e892199a9e1e43f991c506f2cc390dc4efa2026ad2f58'},
>>> +            's390x':
>>> +            {'checksum': '4caaab5a434fd4d1079149a072fdc7891e354f834d355069ca982fdcaf5a122d'},
>>> +            }
>>> +        }
>>> +    }
>>> +
>>> +
>>> +def get_known_distro_checksum(distro, distro_version, arch):
>>> +    try:
>>> +        return KNOWN_DISTROS.get(distro).get(distro_version).get(arch).get('checksum')
>>> +    except AttributeError:
>>> +        return None
>>> +
>>> +
>> Currently we have a few loose methods on avocado_qemu/__init__.py, and I'm
>> about to send a series to wrap them in a mixin class. This series will
>> introduce more loose code on the file; so would you consider moving
>> KNOWN_DISTROS and get_known_distro_checksum() to the LinuxTest class, and
>> possibly making the latest a class method?
>>
> Some of our experience in "avocado.Test" revealed that users would:
>
>    1) find it confusing to have so many methods in the class that are not useful
>       to them
>
>    2) would conflict with variables/attributes of their own
>
> About #2, we end up turning a lot of variables atttributes into
> properties so that errors would be explicit when users tried to
> overwrite them unknowingly.
>
> But, in the specific example of KNOWN_DISTROS and its expansion I
> mentioned before, it may indeed make sense to have a Test or LinuxTest
> method that test writers can use.  It'd probably need to be a bit more
> generic and evolved than this current version though.
>
> Maybe wait for Eric's input based on real world use case here?
Sure, we can wait on Eric's input.
>
>>>    class LinuxTest(Test, LinuxSSHMixIn):
>>>        """Facilitates having a cloud-image Linux based available.
>>> @@ -348,14 +372,20 @@ def download_boot(self):
>>>            vmimage.QEMU_IMG = qemu_img
>>>            self.log.info('Downloading/preparing boot image')
>>> +        distro = 'fedora'
>>> +        distro_version = '31'
>>> +        known_distro_checksum = get_known_distro_checksum(distro,
>>> +                                                          distro_version,
>>> +                                                          self.arch)
>>> +        distro_checksum = self.distro_checksum or known_distro_checksum
>>
>> distro_checksum may be None. In this case vmimage.get() will silently skip
>> the check? I suggest to log a warn message.
>>
>>
> Yes, good point.  But, I think adding that warning to Avocado's
> vmimage.get() method itself is even better.  What do you think?

I like the idea. IMHO avocado_qemu should be a thin layer upon Avocado, 
so such as kind of code should live on avocado's side.

I just opened the PR: https://github.com/avocado-framework/avocado/pull/4539

- Wainer

>
> Thanks for the review!
> - Cleber.