Re: [PATCH-for-5.2? 3/5] tests/acceptance: Skip incomplete virtio_version tests using '@skip'

[Thomas Huth]

On 04/11/2020 12.27, Philippe Mathieu-Daudé wrote:
> On 11/4/20 12:13 PM, Thomas Huth wrote:
>> On 02/11/2020 15.42, Philippe Mathieu-Daudé wrote:
>>> Prefer skipping incomplete tests with the "@skip" keyword,
>>> rather than commenting the code.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>>  tests/acceptance/virtio_version.py | 11 +++++++----
>>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tests/acceptance/virtio_version.py 
>>> b/tests/acceptance/virtio_version.py
>>> index 33593c29dd0..187bbfa1f42 100644
>>> --- a/tests/acceptance/virtio_version.py
>>> +++ b/tests/acceptance/virtio_version.py
>>> @@ -140,17 +140,20 @@ def check_all_variants(self, qemu_devtype, 
>>> virtio_devid):
>>>          self.assertIn('conventional-pci-device', trans_ifaces)
>>>          self.assertNotIn('pci-express-device', trans_ifaces)
>>>  
>>> +    @skip("virtio-blk requires 'driver' parameter")
>>
>> Shouldn't that be 'drive' instead of 'driver' ?
> 
> No clue, this is the previous commented line.
> 
>>
>>> +    def test_conventional_devs_driver(self):
>>> +        self.check_all_variants('virtio-blk-pci', VIRTIO_BLOCK)
>>> +
>>> +    @skip("virtio-9p requires 'fsdev' parameter")
>>> +    def test_conventional_devs_fsdev(self):
>>> +        self.check_all_variants('virtio-9p-pci', VIRTIO_9P)
>>>  
>>>      def test_conventional_devs(self):
>>>          self.check_all_variants('virtio-net-pci', VIRTIO_NET)
>>> -        # virtio-blk requires 'driver' parameter
>>> -        #self.check_all_variants('virtio-blk-pci', VIRTIO_BLOCK)
>>>          self.check_all_variants('virtio-serial-pci', VIRTIO_CONSOLE)
>>>          self.check_all_variants('virtio-rng-pci', VIRTIO_RNG)
>>>          self.check_all_variants('virtio-balloon-pci', VIRTIO_BALLOON)
>>>          self.check_all_variants('virtio-scsi-pci', VIRTIO_SCSI)
>>> -        # virtio-9p requires 'fsdev' parameter
>>> -        #self.check_all_variants('virtio-9p-pci', VIRTIO_9P)
>>
>> I think I'd prefer to keep the stuff commented ... otherwise it will show up
>> in the logs, giving the impression that you could run the tests somehow if
>> you just provided the right environment, which is just not possible right 
>> now.
> 
> Well, we usually don't commit commented code like that.
> If it is committed, it is important, then it has to show up in
> the log. If you don't want it logged, why not remove it then?

Then I'd rather remove it. Or leave a comment saying "we cannot exercise
virtio-9p-pci and virtio-blk-pci here (yet) since they need additional
parameters to be set".

 Thomas