Re: [PATCH] tests: add test for json-streamer.c error recovery

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On Tue, Apr 21, 2026 at 12:29 PM Markus Armbruster <armbru@redhat.com> wrote:
>> > +static void parse_emit(void *opaque, QObject *json, Error *err)
>> > +{
>> > +    ParseResult *r = opaque;
>> > +
>>
>> Recommend
>>
>>        assert(!json != !err);
>
> Makes sense.
>
>> > +static void test_simple(void)
>> > +{
>> > +    check_result("false", 0, QTYPE_QBOOL);
>> > +}
>>
>> This one isn't about error recovery.  It's arguably redundant with
>> check-qjson.c's keyword_literal().
>
> Or the other way round. :)  They test different APIs which makes both
> of them worthwhile.

See discussion at the end.

>> > +
>> > +static void test_extra_closing_braces(void)
>> > +{
>> > +    check_result("}}false", 2, QTYPE_QBOOL);
>> > +}
>>
>> Overlap with check-qjson.c's multiple_values().
>
> Overlapping input but completely different test, because this checks
> for success (after error) whereas multiple_values() checks for an
> error condition.  This also shows the difference between the two test
> files and APIs.
>
>> > +
>> > +static void test_bad_dict(void)
>> > +{
>> > +    check_result("{ 'abc' }false", 1, QTYPE_QBOOL);
>> > +}
>>
>> Interesting: just one error, because error recovery eats false.
>
> Why would it eat false here? The error is recovered after "{ 'abc' }"
> and then "false" is parsed successfully. One error and one successful
> value, as indicated by the arguments to check_result().

I got confused %-}

>> > +
>> > +static void test_lexer_recovery(void)
>> > +{
>> > +    check_result("\f{}", 1, QTYPE_QDICT);
>> > +    check_result("\f[]", 1, QTYPE_QLIST);
>> > +    check_result("\f:false", 2, QTYPE_QBOOL);
>> > +    check_result("\f,false", 2, QTYPE_QBOOL);
>> > +
>> > +    /*
>> > +     * alphabetic characters do not start a new parsing, this is slightly weird
>> > +     * but it keeps the lexer simple and works well for QMP (where valid input
>> > +     * is a sequence of dictionaries)
>> > +     */
>>
>> Please wrap comment lines a bit earlier for legibility:
>
> 79 characters are Just Fine.  While I appreciate careful reviews, and
> I will apply your suggested edit, this is perhaps a bit too much?

I asked nicely, because I do have real trouble reading long lines.  It's
not the right margin, it's the width of the text not counting
indentation.

Peace?

>> > +    /* same as test_lexer_recovery */
>> > +    check_result_error("{[{\ffalse", 1);
>>
>> Really?
>
> Same comment about alphabetic characters not starting a new parsing after \f.
>
>> check-json-parser.c and check-qjson.c both test the JSON parser.
>>
>> check-json-parser.c tests at the json-parser.h level, and focuses on
>> error recovery.
>
> That's what I focused on _for now_, because that's where the push
> parser changes are more tricky, but it doesn't have to be that way.
>
>> check-qjson.c tests one level up at the qjson.h level, where error
>> recovery is not testable.
>>
>> Perhaps the JSON parser tests should all be in one place.  This is not a
>> demand :)
>
> Objection, your honor - different APIs, different test files. :)

Separate APIs, yes, but one wraps around the other in a straightforward
manner.

I don't mind having a separate tests for both levels.

Ideally, the test at the lower level would be complete, and the test at
the upper level focused on what's added there.

As is, much functionality is only covered at the upper level.  Some is
covered at both.

I wanted to point that out.  I could've expressed myself more clearly,
though.  Whether and when to clean up this tangle is up for debate!