Re: [PATCH v7 14/15] scripts: add python_qmp_updater.py

[Vladimir Sementsov-Ogievskiy]

On 06.10.23 20:01, Eric Blake wrote:
> On Fri, Oct 06, 2023 at 06:41:24PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > A script, to update the pattern
> >
> >      result = self.vm.qmp(...)
> >      self.assert_qmp(result, 'return', {})
> >
> > (and some similar ones) into
> >
> >      self.vm.cmd(...)
> >
> > Used in the next commit
> >      "python: use vm.cmd() instead of vm.qmp() where appropriate"
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > ---
> >   scripts/python_qmp_updater.py | 136 ++++++++++++++++++++++++++++++++++
> >   1 file changed, 136 insertions(+)
> >   create mode 100755 scripts/python_qmp_updater.py
> >
> > diff --git a/scripts/python_qmp_updater.py b/scripts/python_qmp_updater.py
> > new file mode 100755
> > index 0000000000..494a169812
> > --- /dev/null
> > +++ b/scripts/python_qmp_updater.py
> > @@ -0,0 +1,136 @@
> > +#!/usr/bin/env python3
> > +#
> > +# Intended usage:
> > +#
> > +# git grep -l '\.qmp(' | xargs ./scripts/python_qmp_updater.py
> > +#
> > +
> > +import re
> > +import sys
> > +from typing import Optional
> > +
> > +start_reg = re.compile(r'^(?P<padding> *)(?P<res>\w+) = (?P<vm>.*).qmp\(',
> > +                       flags=re.MULTILINE)
> > +
> > +success_reg_templ = re.sub('\n *', '', r"""
> > +    (\n*{padding}(?P<comment>\#.*$))?
> > +    \n*{padding}
> > +    (
> > +        self.assert_qmp\({res},\ 'return',\ {{}}\)
> > +    |
> > +        assert\ {res}\['return'\]\ ==\ {{}}
> > +    |
> > +        assert\ {res}\ ==\ {{'return':\ {{}}}}
> > +    |
> > +        self.assertEqual\({res}\['return'\],\ {{}}\)
> > +    )""")
>
> We may find other patterns, but this is a nice way to capture the most
> common ones and a simple place to update if we find another one.
>
> I did a quick grep for 'assert.*return' and noticed things like:
>
> tests/qemu-iotests/056:        self.assert_qmp(res, 'return', {})

this one is

        res = self.vm.qmp(cmd, **kwargs)
        if error:
            self.assert_qmp(res, 'error/desc', error)
            return False

so here we can't just use cmd()

> tests/qemu-iotests/056:        self.assert_qmp(res, 'return', [])

yes, that's a result of query- command, caller wants the exact result.
Actually that's a check for "no block-jobs".

> This script only simplifies the {} form, not the []; but that makes
> sense: when we are testing a command known to return an array rather
> than nothing, we still want to check if the array is empty, and not
> just that the command didn't crash.  We are only simplifying the
> commands that check for nothing in particular returned, on the grounds
> that not crashing was probably good enough, and explicitly checking
> that nothing extra was returned is not worth the effort.

[..]

> >
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks

--
Best regards,
Vladimir