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

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

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)