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

[Philippe Mathieu-Daudé]

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?