Re: [PATCH v2 4/4] tests/functional/test_vmstate: Test whether the checker script works as expected

[Peter Xu <peterx@redhat.com>]

On Thu, May 22, 2025 at 08:12:49PM +0200, Thomas Huth wrote:
> On 22/05/2025 16.40, Peter Xu wrote:
> > On Thu, May 22, 2025 at 03:37:56PM +0200, Thomas Huth wrote:
> > > From: Thomas Huth <thuth@redhat.com>
> > > 
> > > We've got two vmstate dump files in the repository which are meant
> > > for verifying whether the vmstate-static-checker.py works as expected.
> > > Since running this manually is a cumbersome job, let's add an automated
> > > test for this instead that runs the script with the two dump files
> > > and checks for the expected output.
> > > 
> > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > ---
> > >   tests/functional/test_vmstate.py | 37 ++++++++++++++++++++++++++++++++
> > >   1 file changed, 37 insertions(+)
> > > 
> > > diff --git a/tests/functional/test_vmstate.py b/tests/functional/test_vmstate.py
> > > index cc988987481..19a62e8a17e 100755
> > > --- a/tests/functional/test_vmstate.py
> > > +++ b/tests/functional/test_vmstate.py
> > > @@ -9,6 +9,22 @@
> > >   from qemu_test import QemuSystemTest
> > > +expected_output='''Warning: checking incompatible machine types: "pc-i440fx-2.1", "pc-i440fx-2.2"
> > > +Section "fw_cfg" does not exist in dest
> > > +Section "fusbh200-ehci-usb" version error: 2 > 1
> > > +Section "fusbh200-ehci-usb", Description "ehci-core": expected field "usbsts", got "usbsts_pending"; skipping rest
> > > +Section "pci-serial-4x" Description "pci-serial-multi": Entry "Fields" missing
> > > +Section "intel-hda-generic", Description "intel-hda", Field "pci": missing description
> > > +Section "cfi.pflash01": Entry "Description" missing
> > > +Section "megasas", Description "PCIDevice": expected field "irq_state", while dest has no further fields
> > > +Section "PIIX3-xen" Description "PIIX3": minimum version error: 1 < 2
> > > +Section "PIIX3-xen" Description "PIIX3": Entry "Subsections" missing
> > > +Section "tpci200": Description "tpci200" missing, got "tpci2002" instead; skipping
> > > +Section "sun-fdtwo" Description "fdc": version error: 2 > 1
> > > +Section "sun-fdtwo", Description "fdrive": Subsection "fdrive/media_rate" not found
> > > +Section "usb-kbd" Description "usb-kbd" Field "kbd.keycodes" size mismatch: 4 , 2
> > > +'''
> > > +
> > >   class VmStateTest(QemuSystemTest):
> > >       def run_vmstate_checker(self, src_json, dst_json):
> > > @@ -19,6 +35,27 @@ def run_vmstate_checker(self, src_json, dst_json):
> > >                                 stderr=subprocess.STDOUT,
> > >                                 text=True)
> > > +    def test_checker(self):
> > > +        """
> > > +        Test whether the checker script correctly detects the changes
> > > +        between dump1.json and dump2.json.
> > > +        """
> > > +        if self.arch != 'x86_64':
> > > +            self.skipTest('for x86 only')
> > > +        src_json = self.data_file('..', 'data', 'vmstate-static-checker',
> > > +                                  'dump1.json')
> > > +        dst_json = self.data_file('..', 'data', 'vmstate-static-checker',
> > > +                                  'dump2.json')
> > > +        self.log.info(f'Comparing {src_json} with {dst_json}')
> > > +        cp = self.run_vmstate_checker(src_json, dst_json)
> > > +        if cp.returncode != 13:
> > > +            self.fail('Unexpected return code of vmstate-static-checker: ' +
> > > +                      cp.returncode)
> > > +        if cp.stdout != expected_output:
> > > +            self.log.info('vmstate-static-checker output:\n' + cp.stdout)
> > > +            self.log.info('expected output:\n' + expected_output)
> > > +            self.fail('Unexpected vmstate-static-checker output!')
> > > +
> > 
> > Would this run for every make check and every CI run?
> 
> Yes.
> 
> > Since the script almost never change, I wonder whether this test should be
> > triggered only if manually.
> 
> We could disable it by default with a @skip... decorator, but I think it's
> not really worth the effort: The runtime of test_checker() is about 100 ms,
> that's really nothing compared to the hunders of seconds that some other
> tests take.

Right, not a concern of time used but only CPU burns.

Meanwhile, this is not part of the QEMU function that the repo delivers,
but the infra to test QEMU, hence here we're always running this check
script for everyone testing yet another almost not-changing (and may not be
helpful, not anymore?) script to test QEMU..

I think we should save the cycles, but no strong opinions.

Thanks,

-- 
Peter Xu