[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 08/16] qapi: Test use of 'number' within alte
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v6 08/16] qapi: Test use of 'number' within alternates |
Date: |
Thu, 01 Oct 2015 08:23:06 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> writes:
> On 09/29/2015 07:38 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> 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 <address@hidden>
>
>>> +++ 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.