Re: [PATCH v5 3/4] avocado_qemu: Add SMMUv3 tests

[Philippe Mathieu-Daudé]

On 7/6/21 4:39 PM, Willian Rampazzo wrote:
> On Tue, Jul 6, 2021 at 11:25 AM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
>>
>> On 7/6/21 3:57 PM, Eric Auger wrote:
>>> Hi Philippe,
>>>
>>> On 7/6/21 3:34 PM, Philippe Mathieu-Daudé wrote:
>>>> On 7/6/21 3:17 PM, Eric Auger wrote:
>>>>> Add new tests checking the good behavior of the SMMUv3 protecting
>>>>> 2 virtio pci devices (block and net). We check the guest boots and
>>>>> we are able to install a package. Different guest configs are tested:
>>>>> standard, passthrough an strict=0. This is tested with both fedora 31 and
>>>>> 33. The former uses a 5.3 kernel without range invalidation whereas the
>>>>> latter uses a 5.8 kernel that features range invalidation.
>>>>>
>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>>> Reviewed-by: Willian Rampazzo <willianr@redhat.com>
>>>>> Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>>>>> Tested-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
>>>>>
>>>>> ---
>>>>>
>>>>> v4 -> v5:
>>>>> - Added the skipIf statement (William) and William's R-b
>>>>> - added Wainer's R-b and T-b
>>>>> ---
>>>>>  tests/acceptance/smmu.py | 133 +++++++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 133 insertions(+)
>>>>>  create mode 100644 tests/acceptance/smmu.py
>>>>> +    def run_and_check(self):
>>>>> +        if self.kernel_path:
>>>>> +            self.vm.add_args('-kernel', self.kernel_path,
>>>>> +                             '-append', self.kernel_params,
>>>>> +                             '-initrd', self.initrd_path)
>>>>> +        self.launch_and_wait()
>>>> IIUC above is the 'run' part and below is the 'check' part?
>>>>
>>>> So the check succeed if the VM booted, right?
>>>>
>>>>> +        self.ssh_command('cat /proc/cmdline')
>>>>> +        self.ssh_command('dnf -y install numactl-devel')
>>>
>>> For the test to succeed, the guests needs to boot, the ssh connection
>>> needs to be established and  the dnf command needs to complete.
>>> See launch_and_wait() in __init__.py
>>
>> OK. I see the Test class provide a .fail() method:
>>
>>   fail(msg=None)
>>
>>     Signals a test failure unconditionally, with msg or None
>>     for the error message.
>>
>> but no .succeed() one...
>>
>> Willian: Should we add a comment here mentioning the test
>> succeeded by that point? Would it be more explicit to add
>> .succeed() in avocado.Test or avocado_qemu.Test?
>>
> 
> If the test does not fail, it will succeed anyway.
> 
> I miss some assert statements in this test, telling what is really
> tested, but as it won't run on CI, I don't see a problem adding it now
> and extending it later.

Sure, but a one-line comment can be easily added by the maintainer
taking this series ;)