On 09/24/2015 08:36 AM, Markus Armbruster wrote: > Eric Blake <ebl...@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. >> >> Signed-off-by: Eric Blake <ebl...@redhat.com> >> --- >> tests/qapi-schema/qapi-schema-test.json | 8 ++ >> tests/qapi-schema/qapi-schema-test.out | 24 ++++++ >> tests/test-qmp-input-visitor.c | 129 >> +++++++++++++++++++++++++++++++- >> 3 files changed, 158 insertions(+), 3 deletions(-) >>
>> +++ b/tests/test-qmp-input-visitor.c
>> @@ -368,15 +368,136 @@ static void
>> test_visitor_in_alternate(TestInputVisitorData *data,
>> {
>> Visitor *v;
>> Error *err = NULL;
>> - UserDefAlternate *tmp;
>> + UserDefAlternate *tmp = NULL;
>
> Any particular reason for adding the initializer?
>
>>
>> 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);
Hmm - I don't know if we have a clear contract for what happens if you
call visit_type_FOO on an uninitialized pointer. It may have been
succeeding by mere luck.
>
> The pattern
>
> foo(..., &err);
> g_assert(err == NULL);
>
> is pretty common in tests. Can't see what it buys us over straight
> &error_abort. Perhaps I'll spatch it away.
>
>> g_assert_cmpint(tmp->kind, ==, USER_DEF_ALTERNATE_KIND_I);
>> g_assert_cmpint(tmp->i, ==, 42);
>> qapi_free_UserDefAlternate(tmp);
>> + tmp = NULL;
>
> Why do you need to clear tmp?
If we were succeeding on a single call by mere luck where tmp started
life as all 0 due to stack contents, but the second call has tmp
pointing to stale memory, then that would be an obvious reason. I'll
have to revisit what happens, because I don't recall any specific reason
for why I did this other than the symmetry of making sure each parse had
clean state (that is, I don't recall a crash happening if I didn't do
it, and haven't yet tested under valgrind to see if we are provably
using memory incorrectly if we don't initialize).
>> +
>> + /* FIXME: Integers should parse as numbers */
>
> Suggest to augment or replace this comment...
>
>> + v = visitor_input_test_init(data, "42");
>> + visit_type_AltTwo(v, &two, NULL, &err);
>
> ... with
>
> /* FIXME g_assert_cmpint(two->kind, ==, ALT_TWO_KIND_N); */
> /* FIXME g_assert_cmpfloat(two->n, ==, 42); */
Ah, to better document what the test will look like in the future when
the bugs are fixed. Sure, I can do that.
>
>> + g_assert(err);
>> + error_free(err);
>> + err = NULL;
>> + qapi_free_AltTwo(two);
>> + one = NULL;
>
> *chuckle* Why do you clear one here? More of the same below.
Too much copy-and-paste. Will fix.
>
>> +
>> + /* FIXME: Order of alternate should not affect semantics */
>
> Inhowfar does it affect semantics? Or asked differently: what exactly
> is wrong with this test now?
>
>> + v = visitor_input_test_init(data, "42");
>> + visit_type_AltThree(v, &three, NULL, &error_abort);
>> + g_assert_cmpint(three->kind, ==, ALT_THREE_KIND_N);
>> + g_assert_cmpfloat(three->n, ==, 42);
>> + qapi_free_AltThree(three);
>> + one = NULL;
AltTwo and AltThree are ostensibly the same struct (two branches, one
for 'str' and one for 'number', just in a different order), but they
parsed differently (AltTwo failed, AltThree succeeded). The bug is
fixed later when the order of the branch declaration no longer impacts
the result of the parse.
>
> Reading this, I had to refer back to the definition of AltOne, ...,
> AltSix all the time. Let's rename them to AltStrBool, AltStrNum, ...,
> AltNumInt.
Good idea, will do.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
