On Wed, Feb 13, 2019 at 02:18:32PM +0100, Andrea Bolognani wrote:
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?
In this patch? Yes. The aim is to preserve the test coverage done before and after.
I've tried removing a bunch of them and the test didn't break. Are the files even valid JSON with the backslashes included?
No, they get stripped by virTestLoadFile
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.
The whole point of separating these was to allow easier changes and make it more-readable, so introducing more whitespace by removing the backslashes and prettifying it is possible.
[...]+ 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.
All the error messages match the original test. Guess it would make more sense to alter them before copying them.
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.
Like this?
if (!injson) {
if (info->pass) {
return 0;
} else {
return -1;
}
} else {
if (!info->pass)
return -1;
}
Jano
Attachment:
signature.asc
Description: PGP signature
