[v3] tracetool: add test suite to improve reviewability

[PATCH v3 8/8] qapi: switch to use QEMU_TEST_REGENERATE env var

Posted by Daniel P. Berrangé 4 months, 1 week ago

The QAPI_TEST_UPDATE env var can be set when running the QAPI
schema tests to regenerate the reference output. For consistent
naming with the tracetool test, change the env var name to
QEMU_TEST_REGENERATE.

The test is modified to provide a hint about use of the new
env var and it is also added to the developer documentation.document its usage.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 docs/devel/testing/main.rst    | 12 ++++++++++++
 tests/qapi-schema/test-qapi.py |  7 +++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/docs/devel/testing/main.rst b/docs/devel/testing/main.rst
index 11f05c0006..fe233fcf7a 100644
--- a/docs/devel/testing/main.rst
+++ b/docs/devel/testing/main.rst
@@ -178,6 +178,18 @@ parser (either fixing a bug or extending/modifying the syntax). To do this:
 
   ``qapi-schema += foo.json``
 
+The reference output can be automatically updated to match the latest QAPI
+code generator by running the tests with the QEMU_TEST_REGENERATE environment
+variable set.
+
+.. code::
+
+   QEMU_TEST_REGENERATE=1 make check-qapi-schema
+
+The resulting changes must be reviewed by the author to ensure they match
+the intended results, before adding the updated reference output to the
+same commit that alters the generator code.
+
 .. _tracetool-tests:
 
 Tracetool tests
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index 4be930228c..cf7fb8a6df 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -165,7 +165,7 @@ def test_and_diff(test_name, dir_name, update):
     if actual_out == expected_out and actual_err == expected_err:
         return 0
 
-    print("%s %s" % (test_name, 'UPDATE' if update else 'FAIL'),
+    print("%s: %s" % (test_name, 'UPDATE' if update else 'FAIL'),
           file=sys.stderr)
     out_diff = difflib.unified_diff(expected_out, actual_out, outfp.name)
     err_diff = difflib.unified_diff(expected_err, actual_err, errfp.name)
@@ -173,6 +173,9 @@ def test_and_diff(test_name, dir_name, update):
     sys.stdout.writelines(err_diff)
 
     if not update:
+        print(("\n%s: set QEMU_TEST_REGENERATE=1 to recreate reference output" +
+               "if the QAPI schema generator was intentionally changed") % test_name,
+              file=sys.stderr)
         return 1
 
     try:
@@ -197,7 +200,7 @@ def main(argv):
     parser.add_argument('-d', '--dir', action='store', default='',
                         help="directory containing tests")
     parser.add_argument('-u', '--update', action='store_true',
-                        default='QAPI_TEST_UPDATE' in os.environ,
+                        default='QEMU_TEST_REGENERATE' in os.environ,
                         help="update expected test results")
     parser.add_argument('tests', nargs='*', metavar='TEST', action='store')
     args = parser.parse_args()
-- 
2.50.1

Re: [PATCH v3 8/8] qapi: switch to use QEMU_TEST_REGENERATE env var

Posted by Markus Armbruster 4 months, 1 week ago

Daniel P. Berrangé <berrange@redhat.com> writes:

> The QAPI_TEST_UPDATE env var can be set when running the QAPI
> schema tests to regenerate the reference output. For consistent
> naming with the tracetool test, change the env var name to
> QEMU_TEST_REGENERATE.
>
> The test is modified to provide a hint about use of the new
> env var and it is also added to the developer documentation.document its usage.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  docs/devel/testing/main.rst    | 12 ++++++++++++
>  tests/qapi-schema/test-qapi.py |  7 +++++--
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/docs/devel/testing/main.rst b/docs/devel/testing/main.rst
> index 11f05c0006..fe233fcf7a 100644
> --- a/docs/devel/testing/main.rst
> +++ b/docs/devel/testing/main.rst
> @@ -178,6 +178,18 @@ parser (either fixing a bug or extending/modifying the syntax). To do this:
>  
>    ``qapi-schema += foo.json``
>  
> +The reference output can be automatically updated to match the latest QAPI
> +code generator by running the tests with the QEMU_TEST_REGENERATE environment
> +variable set.
> +
> +.. code::
> +
> +   QEMU_TEST_REGENERATE=1 make check-qapi-schema
> +
> +The resulting changes must be reviewed by the author to ensure they match
> +the intended results, before adding the updated reference output to the

Not sure about the comma.  I'm not a native speaker, though.

> +same commit that alters the generator code.
> +
>  .. _tracetool-tests:
>  
>  Tracetool tests
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index 4be930228c..cf7fb8a6df 100755
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -165,7 +165,7 @@ def test_and_diff(test_name, dir_name, update):
>      if actual_out == expected_out and actual_err == expected_err:
>          return 0
>  
> -    print("%s %s" % (test_name, 'UPDATE' if update else 'FAIL'),
> +    print("%s: %s" % (test_name, 'UPDATE' if update else 'FAIL'),

Is there a need for this, or is it just drive-by polishing?

>            file=sys.stderr)
>      out_diff = difflib.unified_diff(expected_out, actual_out, outfp.name)
>      err_diff = difflib.unified_diff(expected_err, actual_err, errfp.name)
> @@ -173,6 +173,9 @@ def test_and_diff(test_name, dir_name, update):
>      sys.stdout.writelines(err_diff)
>  
>      if not update:
> +        print(("\n%s: set QEMU_TEST_REGENERATE=1 to recreate reference output" +
> +               "if the QAPI schema generator was intentionally changed") % test_name,
> +              file=sys.stderr)
>          return 1
>  
>      try:
> @@ -197,7 +200,7 @@ def main(argv):
>      parser.add_argument('-d', '--dir', action='store', default='',
>                          help="directory containing tests")
>      parser.add_argument('-u', '--update', action='store_true',
> -                        default='QAPI_TEST_UPDATE' in os.environ,
> +                        default='QEMU_TEST_REGENERATE' in os.environ,
>                          help="update expected test results")
>      parser.add_argument('tests', nargs='*', metavar='TEST', action='store')
>      args = parser.parse_args()

Re: [PATCH v3 8/8] qapi: switch to use QEMU_TEST_REGENERATE env var

Posted by Daniel P. Berrangé 3 months, 3 weeks ago

On Fri, Aug 08, 2025 at 08:46:10AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > The QAPI_TEST_UPDATE env var can be set when running the QAPI
> > schema tests to regenerate the reference output. For consistent
> > naming with the tracetool test, change the env var name to
> > QEMU_TEST_REGENERATE.
> >
> > The test is modified to provide a hint about use of the new
> > env var and it is also added to the developer documentation.document its usage.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  docs/devel/testing/main.rst    | 12 ++++++++++++
> >  tests/qapi-schema/test-qapi.py |  7 +++++--
> >  2 files changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/docs/devel/testing/main.rst b/docs/devel/testing/main.rst
> > index 11f05c0006..fe233fcf7a 100644
> > --- a/docs/devel/testing/main.rst
> > +++ b/docs/devel/testing/main.rst
> > @@ -178,6 +178,18 @@ parser (either fixing a bug or extending/modifying the syntax). To do this:
> >  
> >    ``qapi-schema += foo.json``
> >  
> > +The reference output can be automatically updated to match the latest QAPI
> > +code generator by running the tests with the QEMU_TEST_REGENERATE environment
> > +variable set.
> > +
> > +.. code::
> > +
> > +   QEMU_TEST_REGENERATE=1 make check-qapi-schema
> > +
> > +The resulting changes must be reviewed by the author to ensure they match
> > +the intended results, before adding the updated reference output to the
> 
> Not sure about the comma.  I'm not a native speaker, though.

Yeah, the comma is redundant

> > +same commit that alters the generator code.
> > +
> >  .. _tracetool-tests:
> >  
> >  Tracetool tests
> > diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> > index 4be930228c..cf7fb8a6df 100755
> > --- a/tests/qapi-schema/test-qapi.py
> > +++ b/tests/qapi-schema/test-qapi.py
> > @@ -165,7 +165,7 @@ def test_and_diff(test_name, dir_name, update):
> >      if actual_out == expected_out and actual_err == expected_err:
> >          return 0
> >  
> > -    print("%s %s" % (test_name, 'UPDATE' if update else 'FAIL'),
> > +    print("%s: %s" % (test_name, 'UPDATE' if update else 'FAIL'),
> 
> Is there a need for this, or is it just drive-by polishing?

Just making it more consistent in style with other error print()
statements we have in the file, as well as this new one I'm
adding.


> 
> >            file=sys.stderr)
> >      out_diff = difflib.unified_diff(expected_out, actual_out, outfp.name)
> >      err_diff = difflib.unified_diff(expected_err, actual_err, errfp.name)
> > @@ -173,6 +173,9 @@ def test_and_diff(test_name, dir_name, update):
> >      sys.stdout.writelines(err_diff)
> >  
> >      if not update:
> > +        print(("\n%s: set QEMU_TEST_REGENERATE=1 to recreate reference output" +
> > +               "if the QAPI schema generator was intentionally changed") % test_name,
> > +              file=sys.stderr)
> >          return 1
> >  
> >      try:
> > @@ -197,7 +200,7 @@ def main(argv):
> >      parser.add_argument('-d', '--dir', action='store', default='',
> >                          help="directory containing tests")
> >      parser.add_argument('-u', '--update', action='store_true',
> > -                        default='QAPI_TEST_UPDATE' in os.environ,
> > +                        default='QEMU_TEST_REGENERATE' in os.environ,
> >                          help="update expected test results")
> >      parser.add_argument('tests', nargs='*', metavar='TEST', action='store')
> >      args = parser.parse_args()
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 8/8] qapi: switch to use QEMU_TEST_REGENERATE env var

Posted by Markus Armbruster 3 months, 2 weeks ago

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Aug 08, 2025 at 08:46:10AM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > The QAPI_TEST_UPDATE env var can be set when running the QAPI
>> > schema tests to regenerate the reference output. For consistent
>> > naming with the tracetool test, change the env var name to
>> > QEMU_TEST_REGENERATE.
>> >
>> > The test is modified to provide a hint about use of the new
>> > env var and it is also added to the developer documentation.document its usage.
>> >
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

[...]

>> > index 4be930228c..cf7fb8a6df 100755
>> > --- a/tests/qapi-schema/test-qapi.py
>> > +++ b/tests/qapi-schema/test-qapi.py
>> > @@ -165,7 +165,7 @@ def test_and_diff(test_name, dir_name, update):
>> >      if actual_out == expected_out and actual_err == expected_err:
>> >          return 0
>> >  
>> > -    print("%s %s" % (test_name, 'UPDATE' if update else 'FAIL'),
>> > +    print("%s: %s" % (test_name, 'UPDATE' if update else 'FAIL'),
>> 
>> Is there a need for this, or is it just drive-by polishing?
>
> Just making it more consistent in style with other error print()
> statements we have in the file, as well as this new one I'm
> adding.

Which existing print()s do you mean?

>
>> 
>> >            file=sys.stderr)
>> >      out_diff = difflib.unified_diff(expected_out, actual_out, outfp.name)
>> >      err_diff = difflib.unified_diff(expected_err, actual_err, errfp.name)
>> > @@ -173,6 +173,9 @@ def test_and_diff(test_name, dir_name, update):
>> >      sys.stdout.writelines(err_diff)
>> >  
>> >      if not update:
>> > +        print(("\n%s: set QEMU_TEST_REGENERATE=1 to recreate reference output" +
>> > +               "if the QAPI schema generator was intentionally changed") % test_name,
>> > +              file=sys.stderr)
>> >          return 1
>> >  
>> >      try:
>> > @@ -197,7 +200,7 @@ def main(argv):
>> >      parser.add_argument('-d', '--dir', action='store', default='',
>> >                          help="directory containing tests")
>> >      parser.add_argument('-u', '--update', action='store_true',
>> > -                        default='QAPI_TEST_UPDATE' in os.environ,
>> > +                        default='QEMU_TEST_REGENERATE' in os.environ,
>> >                          help="update expected test results")
>> >      parser.add_argument('tests', nargs='*', metavar='TEST', action='store')
>> >      args = parser.parse_args()

Re: [PATCH v3 8/8] qapi: switch to use QEMU_TEST_REGENERATE env var

Posted by Daniel P. Berrangé 3 months, 2 weeks ago

On Mon, Aug 25, 2025 at 02:01:35PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Fri, Aug 08, 2025 at 08:46:10AM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > The QAPI_TEST_UPDATE env var can be set when running the QAPI
> >> > schema tests to regenerate the reference output. For consistent
> >> > naming with the tracetool test, change the env var name to
> >> > QEMU_TEST_REGENERATE.
> >> >
> >> > The test is modified to provide a hint about use of the new
> >> > env var and it is also added to the developer documentation.document its usage.
> >> >
> >> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> [...]
> 
> >> > index 4be930228c..cf7fb8a6df 100755
> >> > --- a/tests/qapi-schema/test-qapi.py
> >> > +++ b/tests/qapi-schema/test-qapi.py
> >> > @@ -165,7 +165,7 @@ def test_and_diff(test_name, dir_name, update):
> >> >      if actual_out == expected_out and actual_err == expected_err:
> >> >          return 0
> >> >  
> >> > -    print("%s %s" % (test_name, 'UPDATE' if update else 'FAIL'),
> >> > +    print("%s: %s" % (test_name, 'UPDATE' if update else 'FAIL'),
> >> 
> >> Is there a need for this, or is it just drive-by polishing?
> >
> > Just making it more consistent in style with other error print()
> > statements we have in the file, as well as this new one I'm
> > adding.
> 
> Which existing print()s do you mean?

I was referring to the ones in this method

    except OSError as err:
        print("%s: can't open '%s': %s"
              % (sys.argv[0], err.filename, err.strerror),
              file=sys.stderr)
        return 2

..

    except OSError as err:
        print("%s: can't write '%s': %s"
              % (sys.argv[0], err.filename, err.strerror),
              file=sys.stderr)


though I notice now they are using 'sys.argv[0]' which is not the
same as 'test_name', as well as using stderr. So the consistency
isn't actually helped

> 
> >
> >> 
> >> >            file=sys.stderr)
> >> >      out_diff = difflib.unified_diff(expected_out, actual_out, outfp.name)
> >> >      err_diff = difflib.unified_diff(expected_err, actual_err, errfp.name)
> >> > @@ -173,6 +173,9 @@ def test_and_diff(test_name, dir_name, update):
> >> >      sys.stdout.writelines(err_diff)
> >> >  
> >> >      if not update:
> >> > +        print(("\n%s: set QEMU_TEST_REGENERATE=1 to recreate reference output" +
> >> > +               "if the QAPI schema generator was intentionally changed") % test_name,
> >> > +              file=sys.stderr)
> >> >          return 1
> >> >  
> >> >      try:
> >> > @@ -197,7 +200,7 @@ def main(argv):
> >> >      parser.add_argument('-d', '--dir', action='store', default='',
> >> >                          help="directory containing tests")
> >> >      parser.add_argument('-u', '--update', action='store_true',
> >> > -                        default='QAPI_TEST_UPDATE' in os.environ,
> >> > +                        default='QEMU_TEST_REGENERATE' in os.environ,
> >> >                          help="update expected test results")
> >> >      parser.add_argument('tests', nargs='*', metavar='TEST', action='store')
> >> >      args = parser.parse_args()
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|