Re: [Qemu-devel] [PATCH 1/9] tests: QAPI schema parser tests

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 07/26/2013 08:16 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> On 07/26/2013 06:39 AM, Markus Armbruster wrote:
>>>> The parser handles erroneous input badly.  To be improved shortly.
>>>>
>>>> Signed-off-by: Markus Armbruster <address@hidden>
>>>> ---
>>>
>>> Lots of proof on how bad it is!  I'd also like to see a couple tests on
>>> trailing commas:
>>>
>>> { 'enum': 'Foo', [ 'bar' ], }
>>> { 'enum': 'Gur', [ 'ble', ] }
>> 
>> I figure you mean
>> 
>> { 'enum': 'Foo', 'data': [ 'bar' ], }
>> { 'enum': 'Gur', 'data': [ 'ble', ] }
>
> Yep, you got my intent, even if I didn't type it right.
>
>> 
>> My parser rejects both:
>> 
>> <stdin>:1:37: Expected string
>> <stdin>:2:35: Expected "{", "[" or string
>
> Good!
>
>> 
>> I commented out the first to get the second error.  Making the parser
>> continue after errors didn't seem to be worthwhile.
>
> Agree about not continuing after errors; once the file is clean, anyone
> adding a new command will have errors in at most the one command they
> are trying to add, and even if it is an iterative approach to get them
> to find all the problems, it's a lot easier to have that one developer
> fix their work than trying to bake error recovery into the parser.
>
> If you do add these two test cases (which I recommend), it should indeed
> be two separate tests.

Could be done on top.  If I need to respin anyway, I'll add them in the
first patch, of course.

> Another thought - is it worth testing other valid JSON but invalid
> schema constructs, such as:
>
> { 'enum': 1, 'data': false }

Maybe, but I limited myself just to the parser in this series.