[libvirt] [PATCH 4/6] tests: qemumonitorjsontest: Do some actual testing in qemuMonitorJSONTestAttachChardev

[Peter Krempa]

On Wed, Oct 05, 2016 at 09:38:16 -0400, John Ferlan wrote:
> On 09/27/2016 12:39 PM, Peter Krempa wrote:
> > Until now the test was rather useless since it didn't check the
> > arguments formatted and didn't use properly configured chardev objects.
> > 
> > Add the expected arguments and instrument the test to validate them.
> > Modify some test cases to actually add valid data.
> > 
> > Note that the UDP test data is currently wrong due to a bug.
> > ---
> >  tests/qemumonitorjsontest.c | 93 +++++++++++++++++++++++++++++++++------------
> >  1 file changed, 68 insertions(+), 25 deletions(-)
> > 
> > diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c
> > index 23877f8..61344b7 100644
> > --- a/tests/qemumonitorjsontest.c
> > +++ b/tests/qemumonitorjsontest.c
> > @@ -747,6 +747,7 @@ static int
> >  qemuMonitorJSONTestAttachOneChardev(virDomainXMLOptionPtr xmlopt,
> >                                      const char *label,
> >                                      virDomainChrSourceDefPtr chr,
> > +                                    const char *expectargs,
> >                                      const char *reply,
> >                                      const char *expectPty,
> >                                      bool fail)
> > @@ -772,7 +773,8 @@ qemuMonitorJSONTestAttachOneChardev(virDomainXMLOptionPtr xmlopt,
> >      if (!(data.test = qemuMonitorTestNewSimple(true, xmlopt)))
> >          goto cleanup;
> > 
> > -    if (qemuMonitorTestAddItem(data.test, "chardev-add", jsonreply) < 0)
> > +    if (qemuMonitorTestAddItemExpect(data.test, "chardev-add",
> > +                                     expectargs, true, jsonreply) < 0)
> 
> FWIW: The "knowledge" of whether the expectargs has an apostrophe would
> seem to lie in the caller. If in fact you still keep the apostrophe

That acutally means that the string uses apostrophes instead of quotes
for easier embedding into C.

In this exact case, all the strings tested use apostrophes instead of
quotes so this will always be true. I don't see a benefit of adding it
as an argument for every single test case in this test.

> check in patch 2, then I would think the caller would be able to supply
> true/false, but that's a minor thing.

This code is always using the shortcut, since I'm lazy to escape every
single quote.

> Also, something that just occurred to me... 'expectargs' had better not
> be NULL; otherwise, patch 2 will core at the while (*tmp != '\0')

There are other monitor testing APIs that you can use if you don't care
about the arguments.

Additionally the worker function that is verifying the data is not able
to expect a NULL string as these APIs are made to test the actual
arguments. For commands which don't have arguments you'll get an earlier
error. The expected string should thus never be NULL, since it would be
a programmer mistake.

And if it will be by accident, the testsuite will crash which is correct
in this regard IMO.

> ACK in principle - your call on the apostrophe thing - although perhaps
> a check in patch 2 should be added for (apostrophe && data->expectArgs)
> to protect from future mishaps...
> 
> John
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161006/7ec9d9c0/attachment-0001.sig>