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

[Philippe Mathieu-Daudé]

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.

>> +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)
>