[libvirt PATCH 04/20] tests: switch to compact empty JSON object formatting

Ján Tomko posted 20 patches 3 months, 1 week ago
There is a newer version of this series
[libvirt PATCH 04/20] tests: switch to compact empty JSON object formatting
Posted by Ján Tomko 3 months, 1 week ago
Some earlier versions of json-c format empty elements differently.
Run the tests who use the pretty formatting for readability and
diffability through a function that unifies the output.

Signed-off-by: Ján Tomko <jtomko@redhat.com>
---
 tests/qemublocktest.c                              | 5 ++++-
 tests/qemublocktestdata/backupmerge/empty-out.json | 4 +---
 tests/qemumigparamsdata/empty.json                 | 4 +---
 tests/qemumigparamstest.c                          | 5 ++++-
 tests/virmacmaptest.c                              | 5 ++++-
 tests/virmacmaptestdata/empty.json                 | 4 +---
 6 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c
index c581bd1748..6c4e735466 100644
--- a/tests/qemublocktest.c
+++ b/tests/qemublocktest.c
@@ -720,6 +720,7 @@ testQemuBackupIncrementalBitmapCalculate(const void *opaque)
     g_autofree char *expectpath = NULL;
     g_autoptr(virStorageSource) target = NULL;
     g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
+    g_autofree char *actual = NULL;
 
     expectpath = g_strdup_printf("%s/%s%s-out.json", abs_srcdir,
                                  backupDataPrefix, data->name);
@@ -748,7 +749,9 @@ testQemuBackupIncrementalBitmapCalculate(const void *opaque)
         virBufferAddLit(&buf, "NULL\n");
     }
 
-    return virTestCompareToFile(virBufferCurrentContent(&buf), expectpath);
+    actual = virJSONStringPrettifyBlanks(virBufferCurrentContent(&buf));
+
+    return virTestCompareToFile(actual, expectpath);
 }
 
 
diff --git a/tests/qemublocktestdata/backupmerge/empty-out.json b/tests/qemublocktestdata/backupmerge/empty-out.json
index 41b42e677b..fe51488c70 100644
--- a/tests/qemublocktestdata/backupmerge/empty-out.json
+++ b/tests/qemublocktestdata/backupmerge/empty-out.json
@@ -1,3 +1 @@
-[
-
-]
+[]
diff --git a/tests/qemumigparamsdata/empty.json b/tests/qemumigparamsdata/empty.json
index 0db3279e44..0967ef424b 100644
--- a/tests/qemumigparamsdata/empty.json
+++ b/tests/qemumigparamsdata/empty.json
@@ -1,3 +1 @@
-{
-
-}
+{}
diff --git a/tests/qemumigparamstest.c b/tests/qemumigparamstest.c
index 5d45a9dd58..67cc14d948 100644
--- a/tests/qemumigparamstest.c
+++ b/tests/qemumigparamstest.c
@@ -137,6 +137,7 @@ qemuMigParamsTestJSON(const void *opaque)
     g_autoptr(virJSONValue) paramsIn = NULL;
     g_autoptr(virJSONValue) paramsOut = NULL;
     g_autoptr(qemuMigrationParams) migParams = NULL;
+    g_autofree char *formattedJSON = NULL;
     g_autofree char *actualJSON = NULL;
     g_auto(virBuffer) debug = VIR_BUFFER_INITIALIZER;
 
@@ -156,9 +157,11 @@ qemuMigParamsTestJSON(const void *opaque)
         return -1;
 
     if (!(paramsOut = qemuMigrationParamsToJSON(migParams, false)) ||
-        !(actualJSON = virJSONValueToString(paramsOut, true)))
+        !(formattedJSON = virJSONValueToString(paramsOut, true)))
         return -1;
 
+    actualJSON = virJSONStringPrettifyBlanks(formattedJSON);
+
     if (testQEMUSchemaValidateCommand("migrate-set-parameters",
                                       paramsOut,
                                       data->qmpschema,
diff --git a/tests/virmacmaptest.c b/tests/virmacmaptest.c
index 9a28c1bed0..074bc8f659 100644
--- a/tests/virmacmaptest.c
+++ b/tests/virmacmaptest.c
@@ -118,13 +118,16 @@ testMACFlush(const void *opaque)
     const struct testData *data = opaque;
     g_autofree char *file = NULL;
     g_autofree char *str = NULL;
+    g_autofree char *actual = NULL;
 
     file = g_strdup_printf("%s/virmacmaptestdata/%s.json", abs_srcdir, data->file);
 
     if (virMacMapDumpStr(data->mgr, &str) < 0)
         return -1;
 
-    if (virTestCompareToFile(str, file) < 0)
+    actual = virJSONStringPrettifyBlanks(str);
+
+    if (virTestCompareToFile(actual, file) < 0)
         return -1;
 
     return 0;
diff --git a/tests/virmacmaptestdata/empty.json b/tests/virmacmaptestdata/empty.json
index 41b42e677b..fe51488c70 100644
--- a/tests/virmacmaptestdata/empty.json
+++ b/tests/virmacmaptestdata/empty.json
@@ -1,3 +1 @@
-[
-
-]
+[]
-- 
2.45.2
Re: [libvirt PATCH 04/20] tests: switch to compact empty JSON object formatting
Posted by Peter Krempa 3 months, 1 week ago
On Wed, Aug 14, 2024 at 23:40:19 +0200, Ján Tomko wrote:
> Some earlier versions of json-c format empty elements differently.
> Run the tests who use the pretty formatting for readability and
> diffability through a function that unifies the output.

Hmm so for test/output stability IMO it'd be better to fix the output
inside virJSONValueToString(..., true) (so when we're prettifying).

This would cover all existing instances in the tests, but also all
future XMLs. Additionally it'd unify the output of prettified JSON we
have e.g. in 'virsh qemu-monitor-command --pretty'.


Too bad that it basically requires duplicating the output string which
makes me think twice whether it's really worth doing.

What do you think?

Regardless, what's here works:

Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Re: [libvirt PATCH 04/20] tests: switch to compact empty JSON object formatting
Posted by Ján Tomko 3 months ago
On a Thursday in 2024, Peter Krempa wrote:
>On Wed, Aug 14, 2024 at 23:40:19 +0200, Ján Tomko wrote:
>> Some earlier versions of json-c format empty elements differently.
>> Run the tests who use the pretty formatting for readability and
>> diffability through a function that unifies the output.
>
>Hmm so for test/output stability IMO it'd be better to fix the output
>inside virJSONValueToString(..., true) (so when we're prettifying).
>
>This would cover all existing instances in the tests, but also all
>future XMLs. Additionally it'd unify the output of prettified JSON we
>have e.g. in 'virsh qemu-monitor-command --pretty'.
>
>
>Too bad that it basically requires duplicating the output string which
>makes me think twice whether it's really worth doing.
>
>What do you think?
>

I don't think the duplication is worth it. And we don't really guarantee
stability of the pretty formatting.

On the other hand, if the output really is intended for human
consumption, the overhead would be negligible.


The following systems from our CI need this workaround:

AlmaLinux 9: 0.14
CentOS Stream 9: 0.14
Ubuntu 22.04: 0.15
Debian 12: 0.16
OpenSUSE Leap 15.6: 0.16

So we would be able to drop it two years after a new major version of
all of these gets out, if I'm remembering our rules right.

Jano

>Regardless, what's here works:
>
>Reviewed-by: Peter Krempa <pkrempa@redhat.com>
>
Re: [libvirt PATCH 04/20] tests: switch to compact empty JSON object formatting
Posted by Peter Krempa 3 months ago
On Tue, Aug 20, 2024 at 17:04:19 +0200, Ján Tomko wrote:
> On a Thursday in 2024, Peter Krempa wrote:
> > On Wed, Aug 14, 2024 at 23:40:19 +0200, Ján Tomko wrote:
> > > Some earlier versions of json-c format empty elements differently.
> > > Run the tests who use the pretty formatting for readability and
> > > diffability through a function that unifies the output.
> > 
> > Hmm so for test/output stability IMO it'd be better to fix the output
> > inside virJSONValueToString(..., true) (so when we're prettifying).
> > 
> > This would cover all existing instances in the tests, but also all
> > future XMLs. Additionally it'd unify the output of prettified JSON we
> > have e.g. in 'virsh qemu-monitor-command --pretty'.
> > 
> > 
> > Too bad that it basically requires duplicating the output string which
> > makes me think twice whether it's really worth doing.
> > 
> > What do you think?
> > 
> 
> I don't think the duplication is worth it. And we don't really guarantee
> stability of the pretty formatting.

Hmm, yeah. This is mostly whether it makes sense to prevent potential
spurious CI failures in case we happen to add a JSON output file which
would break in any other output path.

Debugging that might not be obvious to people who didn't see the
function.

As said:

> > 
> > Reviewed-by: Peter Krempa <pkrempa@redhat.com>
> >