utils.vmimage.get: Should missing hash mark test as WARN?

[beraldoleal]

So, let's say that test writers are writing the following test:

class TransientDomain(Test):
    def setUp(self):
        super().setUp()
        self.image = vmimage.get(name='fedora',
                                 version='33',
                                 cache_dir='/tmp/libvirt-scratch')

    def test_lifecycle(self):
        self.assertTrue(1)

Because I'm not passing a hash, test will be marked as WARN, because No hash provided. Cannot check the asset file integrity:

15:20:16 INFO | INIT 1-TransientDomain.test_lifecycle
15:20:16 DEBUG| PARAMS (key=timeout, path=*, default=None) => None
15:20:16 DEBUG| Test metadata:
15:20:16 DEBUG|   filename: /home/local/src/beraldo/libvirt/tests/beraldo/tests/domain/test_transient.py
15:20:16 DEBUG|   teststmpdir: /var/tmp/avocado_hc2hfpv2
15:20:16 DEBUG|   workdir: /var/tmp/tmpg7u2d4ql/test-results/tmp_dirbakd7kn_/1-TransientDomain.test_lifecycle
15:20:16 INFO | START 1-TransientDomain.test_lifecycle
15:20:16 DEBUG| DATA (filename=output.expected) => NOT FOUND (data sources: variant, test, file)
15:20:17 WARNI| No hash provided. Cannot check the asset file integrity.
15:20:17 INFO | Running '/bin/qemu-img create -f qcow2 -b /tmp/libvirt-scratch/by_location/67e23165d25d7f9934a909a12bc14277c9fb0b0d/Fedora-Cloud-Base-33-1.2.x86_64.qcow2 -F qcow2 /tmp/libvirt-scratch/by_location/67e23165d25d7f9934a909a12bc14277c9fb0b0d/Fedora-Cloud-Base-33-1.2.x86_64-f2a9cc6d.qcow2'
15:20:17 DEBUG| [stdout] Formatting '/tmp/libvirt-scratch/by_location/67e23165d25d7f9934a909a12bc14277c9fb0b0d/Fedora-Cloud-Base-33-1.2.x86_64-f2a9cc6d.qcow2', fmt=qcow2 size=4294967296 backing_file=/tmp/libvirt-scratch/by_location/67e23165d25d7f9934a909a12bc14277c9fb0b0d/Fedora-Cloud-Base-33-1.2.x86_64.qcow2 backing_fmt=qcow2 cluster_size=65536 lazy_refcounts=off refcount_bits=16
15:20:17 INFO | Command '/bin/qemu-img create -f qcow2 -b /tmp/libvirt-scratch/by_location/67e23165d25d7f9934a909a12bc14277c9fb0b0d/Fedora-Cloud-Base-33-1.2.x86_64.qcow2 -F qcow2 /tmp/libvirt-scratch/by_location/67e23165d25d7f9934a909a12bc14277c9fb0b0d/Fedora-Cloud-Base-33-1.2.x86_64-f2a9cc6d.qcow2' finished with 0 after 0.006511348998174071s
15:20:17 DEBUG| Formatting '/tmp/libvirt-scratch/by_location/67e23165d25d7f9934a909a12bc14277c9fb0b0d/Fedora-Cloud-Base-33-1.2.x86_64-f2a9cc6d.qcow2', fmt=qcow2 size=4294967296 backing_file=/tmp/libvirt-scratch/by_location/67e23165d25d7f9934a909a12bc14277c9fb0b0d/Fedora-Cloud-Base-33-1.2.x86_64.qcow2 backing_fmt=qcow2 cluster_size=65536 lazy_refcounts=off refcount_bits=16

15:20:17 ERROR| WARN 1-TransientDomain.test_lifecycle -> TestWarn: Test passed but there were warnings during execution. Check the log for details.

I understand that ideally users should care about the HASH on an specific image before starting the tests to avoid issues, however on the other hand, setting a HASH will make test SKIP if any update happens on the image and the hash changes. To avoid updating tests every time an image is modified, unless I'm missing something, from the usability PoV, makes sense for some users to ignore the hash, and just say: "Give me a Fedora 33 image".

So I would like to know what you think about this? Is this acceptable? Makes sense?

[willianrampazzo]

Yes, I think that is the desired behavior as far as I remember of a discussion with @clebergnu in 2019. That is the main reason for the by_location feature. So, a test should just pass if no hash is available for an asset. If I recall correctly, this message about the hash was added some months ago, wasn't it?

[beraldoleal]

So, a test should just pass if no hash is available for an asset.

IIUC, this is different from what is happening today.

If I recall correctly, this message about the hash was added some months ago, wasn't it?

Yes:

commit 2f2ffa4
Author: Wainer dos Santos Moschetta [email protected]
Date: Mon Apr 19 19:05:13 2021 -0300

[beraldoleal]

But I think we should revisit this. Let's see what others think about it.

[wainersm]

As stated on commit 2f2ffa4 , the warn message is meant to help users to debug tests, by clearly spotting a fail may be caused by a corrupted asset.

I'm not sure if in this situation the framework should change the test status. The user may accept the risk of not passing a hash (i.e. the asset integrity won't be verified).

[beraldoleal]

I agree that we should send a WARN (or DEBUG) message to the log to help users.

I'm just not sure either about the test status itself. So the point is: should we force all users to pass a HASH? If so, are we ok with the fact that changes on the image will need to update this hash on tester's code?

[clebergnu]

I think WARN is a valid status here, because:

1. it doesn't imply a failure
2. it motivates users to eventually handle the warning (by providing a hash)
3. because of the previous point, users will either end up with a reproducible and trusted asset, which should be one of the basis of a true PASS status

IMO we're not very much used to the WARNING status, but I think we should get used to it.