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

[Cleber Rosa]

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.

> > +    '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?

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

Thanks for the review!
- Cleber.

signature.asc
Description: PGP signature