[libvirt] [PATCH 1/3] virjsontest: introduce DO_TEST_PARSE_FILE

Andrea Bolognani abologna at redhat.com
Wed Feb 13 13:18:32 UTC 2019


On Tue, 2019-02-12 at 16:57 +0100, Ján Tomko wrote:
[...]
> +{"return": [{"filename": \
> +"unix:/home/berrange/.libvirt/qemu/lib/tck.monitor,server",\
> +"label": "charmonitor"}, {"filename": "pty:/dev/pts/158",\
> +"label": "charserial0"}], "id": "libvirt-3"}

Are the backslashes at the end of lines necessary? I've tried
removing a bunch of them and the test didn't break. Are the files
even valid JSON with the backslashes included?

Additional question: can't we pretty-print at least the input
files now? Unless of course the point of these specific test cases
is to prove we can successfully parse certain unusual constructs.

[...]
> +    if (!injson) {
> +        if (info->pass) {
> +            VIR_TEST_VERBOSE("Fail to parse %s\n", info->name);
> +            return -1;
> +        } else {
> +            VIR_TEST_DEBUG("Fail to parse %s\n", info->name);
> +            return 0;
> +        }
> +    }

The second message should read something like

  Expected failure while parsing %s

or

  Failed to parse %s (expected)

> +
> +    if (!info->pass) {
> +        VIR_TEST_VERBOSE("Should not have parsed %s\n", info->name);
> +        return -1;
> +    }

Maybe

  Expected failure while parsing %s, got success instead

or something along those lines.

I think it would also look more legible if this entire if block was
inside the else branch of the previous if block, but if you feel
strongly about this version then just leave it as is.

Everything else looks good.

-- 
Andrea Bolognani / Red Hat / Virtualization




More information about the libvir-list mailing list