Re: [Qemu-devel] [PATCH v6 08/16] qapi: Test use of 'number' within alternates

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 09/29/2015 07:38 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> Add some testsuite exposure for use of a 'number' as part of
>>> an alternate.  The current state of the tree has a few bugs
>>> exposed by this: our input parser depends on the ordering of
>>> how the qapi schema declared the alternate, and the parser
>>> does not accept integers for a 'number' in an alternate even
>>> though it does for numbers outside of an alternate.
>>>
>>> Mixing 'int' and 'number' in the same alternate is unusual,
>>> since both are supplied by json-numbers, but there does not
>>> seem to be a technical reason to forbid it given that our
>>> json lexer distinguishes between json-numbers that can be
>>> represented as an int vs. those that cannot.
>>>
>>> Improve the existing test_visitor_in_alternate() to match the
>>> style of the new test_visitor_in_alternate_number(), and to
>>> ensure full coverage of all possible qtype parsing.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>
>>> +++ b/tests/test-qmp-input-visitor.c
>>> @@ -371,12 +371,133 @@ static void test_visitor_in_alternate(TestInputVisitorData *data,
>>>      UserDefAlternate *tmp;
>>>
>>>      v = visitor_input_test_init(data, "42");
>>> -
>>> -    visit_type_UserDefAlternate(v, &tmp, NULL, &err);
>>> -    g_assert(err == NULL);
>>> +    visit_type_UserDefAlternate(v, &tmp, NULL, &error_abort);
>>>      g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_I);
>>>      g_assert_cmpint(tmp->i, ==, 42);
>>>      qapi_free_UserDefAlternate(tmp);
>>> +    visitor_input_teardown(data, NULL);
>> 
>> Ugly in this test: visitor_input_test_init() is to be paired with
>> visitor_input_teardown(), but each test's last visitor_input_teardown()
>> can be omitted, because we also pass visitor_input_teardown to
>> g_test_add().  Not your patch's fault.  Let's ignore it for now.
>
> v5 25/46 claims otherwise (valgrind was complaining about leaks that
> additional calls to visitor_input_teardown() resolved; maybe the test
> engine is not automatically doing the expected teardown?).  And maybe it
> would be smarter to rework the tests to allow reusing an existing
> visitor on a new string, instead of completely allocating a new visitor
> framework for every new string to parse.  But indeed, any further
> cleanups are better done in that later patch.

Let me rephrase.

visitor_input_test_init() creates what visitor_input_teardown()
destroys.  The two need to be paired.

The pairing is done in an ugly way: the tests call
visitor_input_teardown() for every visitor_input_test_init() *except*
the last one.  That one's teardown is set up by
input_visitor_test_add(), which passes visitor_input_teardown() to
g_test_add().

When a function calls visitor_input_test_init() just once, this ugliness
isn't very visible.  When it calls it multiple times, it's quite visible
and actually confusing.