Re: [PATCH v3 1/5] tests/acceptance: Extract QemuBaseTest from Test

[Wainer dos Santos Moschetta]

On 3/17/21 10:26 AM, Philippe Mathieu-Daudé wrote:
> On 3/17/21 2:08 PM, Wainer dos Santos Moschetta wrote:> On 3/15/21 8:08
> PM, Philippe Mathieu-Daudé wrote:
> > > The Avocado Test::fetch_asset() is handy to download artifacts
> > > before running tests. The current class is named Test but only
> > > tests system emulation. As we want to test user emulation,
> > > refactor the common code as QemuBaseTest.
> > >
> > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > > ---
> > >    tests/acceptance/avocado_qemu/__init__.py | 23 ++++++++++++++++++++---
> > >    1 file changed, 20 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tests/acceptance/avocado_qemu/__init__.py
> > > b/tests/acceptance/avocado_qemu/__init__.py
> > > index df167b142cc..4f814047176 100644
> > > --- a/tests/acceptance/avocado_qemu/__init__.py
> > > +++ b/tests/acceptance/avocado_qemu/__init__.py
> > > @@ -155,7 +155,7 @@ def exec_command_and_wait_for_pattern(test, command,
> > >        """
> > >        _console_interaction(test, success_message, failure_message,
> > > command + '\r')
> > >    -class Test(avocado.Test):
> > > +class QemuBaseTest(avocado.Test):
> > The QemuBaseTest class still defines require_accelerator() which is only
> > used by qemu-system tests (thus, it should rather live on the Test
> > class). Same thing for self.machine, unless that property is used on
> > qemu-user tests.
> require_accelerator() has been added recently (efe30d5011b7, 2021-02-03)
> and this patch is 2 years old, so I missed that while rebasing.
>
> > >        def _get_unique_tag_val(self, tag_name):
> > >            """
> > >            Gets a tag value, if unique for a key
> > > @@ -188,8 +188,6 @@ def require_accelerator(self, accelerator):
> > >                            "available" % accelerator)
> > >          def setUp(self):
> > > -        self._vms = {}
> > > -
> > >            self.arch = self.params.get('arch',
> > >                                       
> > > default=self._get_unique_tag_val('arch'))
> > >    @@ -202,6 +200,25 @@ def setUp(self):
> > >            if self.qemu_bin is None:
> > >                self.cancel("No QEMU binary defined or found in the
> > > build tree")
> > >    +
> > > +    def fetch_asset(self, name,
> > > +                    asset_hash=None, algorithm=None,
> > > +                    locations=None, expire=None,
> > > +                    find_only=False, cancel_on_missing=True):
> > > +        return super(QemuBaseTest, self).fetch_asset(name,
> > > +                        asset_hash=asset_hash,
> > > +                        algorithm=algorithm,
> > > +                        locations=locations,
> > > +                        expire=expire,
> > > +                        find_only=find_only,
> > > +                        cancel_on_missing=cancel_on_missing)
> > Do you overwrite this fetch_asset() on class Test on purpose? I didn't
> > get why fetch_asset() is defined on the classes inherited from
> > QemuBaseTest.
> No idea. Maybe I had to do that back then, and now it is pointless.
> This is why peer-review is helpful :) I'll revisit.
>
> > > +
> > > +# a.k.a. QemuSystemTest for system emulation...
> > Above comment could become the class docstring.
> No, there should be no comment at all. We have to see if we are OK to
> rename or not. You suggested this change:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg659843.html
> But the there was more discussion and we never agreed on anything.
> I don't want to restart doing this change if it is ignored again,
> as it was a waste of time.

I stand my suggestion to rename the Test class. However, IMO this could 
be done on a separate series later after we get this merged, so to avoid 
more discussion. I can work on it myself. Sound as a good plan for you?

> > > +class Test(QemuBaseTest):
> > > +    def setUp(self):
> > > +        self._vms = {}
> > > +        super(Test, self).setUp()
> > > +
> > >        def _new_vm(self, *args):
> > >            self._sd = tempfile.TemporaryDirectory(prefix="avo_qemu_sock_")
> > >            vm = QEMUMachine(self.qemu_bin, sock_dir=self._sd.name)