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

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Before rewriting the error recovery code to work in a push parsing
> setup, make sure that we have tests for it.
>
> Cover various cases of invalid JSON, to check that structural
> recovery based on balanced brackets and braces works; and
> lexer-based recovery which documents "\f" as a sure fire
> way to reset the lexer.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  tests/unit/check-json-parser.c | 145 +++++++++++++++++++++++++++++++++
>  tests/unit/meson.build         |   1 +
>  2 files changed, 146 insertions(+)
>  create mode 100644 tests/unit/check-json-parser.c
>
> diff --git a/tests/unit/check-json-parser.c b/tests/unit/check-json-parser.c
> new file mode 100644
> index 00000000000..ca2d5f41097
> --- /dev/null
> +++ b/tests/unit/check-json-parser.c
> @@ -0,0 +1,145 @@
> +/*
> + * Unit tests for JSON Parser error recovery
> + *
> + * Copyright 2026 Red Hat
> + * Author: Paolo Bonzini <pbonzini@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include "qapi/error.h"
> +#include "qobject/qbool.h"
> +#include "qobject/json-parser.h"
> +
> +typedef struct ParseResult {
> +    int errors;
> +    QObject *result;
> +} ParseResult;
> +
> +static void parse_emit(void *opaque, QObject *json, Error *err)
> +{
> +    ParseResult *r = opaque;
> +

Recommend

       assert(!json != !err);

> +    if (err) {
> +        r->errors++;
> +        error_free(err);

ParseResult.errors counts errors, and ...

> +    } else {
> +        qobject_unref(r->result);
> +        r->result = json;

.result is the last value successfully parsed.

Suggest to comment struct ParseResult accordingly.

> +    }
> +}
> +
> +static ParseResult do_parse(const char *input)
> +{
> +    ParseResult r = { 0, NULL };
> +    JSONMessageParser parser;
> +
> +    json_message_parser_init(&parser, parse_emit, &r, NULL);
> +    json_message_parser_feed(&parser, input, strlen(input));
> +    json_message_parser_flush(&parser);
> +    json_message_parser_destroy(&parser);
> +    return r;
> +}

This is similar to qobject_from_json(), but geared towards testing error
recovery:

* qobject_from_json() treats multiple JSON values as error, do_parse()
  returns the last one.

* qobject_from_json() returns the last error, do_parse() returns the
  number of errors.

Okay.

> +
> +static void check_result(const char *input, int expected_errors, QType expected_type)

Break the line after the second parameter, please.

> +{
> +    ParseResult r = do_parse(input);
> +
> +    g_assert_cmpint(r.errors, ==, expected_errors);
> +    g_assert_nonnull(r.result);
> +    g_assert_cmpint(qobject_type(r.result), ==, expected_type);
> +    qobject_unref(r.result);
> +}
> +
> +static void check_result_error(const char *input, int expected_errors)
> +{
> +    ParseResult r = do_parse(input);
> +
> +    g_assert_cmpint(r.errors, ==, expected_errors);
> +    g_assert_null(r.result);
> +}
> +
> +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().

> +
> +static void test_whitespace(void)
> +{
> +    check_result(" false", 0, QTYPE_QBOOL);
> +}

Also not about error recovery.  Sort of redundant with check-qjson.c's
simple_whitespace().

> +
> +static void test_extra_closing_braces(void)
> +{
> +    check_result("}}false", 2, QTYPE_QBOOL);
> +}

Overlap with check-qjson.c's multiple_values().

> +
> +static void test_bad_dict(void)
> +{
> +    check_result("{ 'abc' }false", 1, QTYPE_QBOOL);
> +}

Interesting: just one error, because error recovery eats false.

> +
> +static void test_trailing_comma(void)
> +{
> +    check_result("[ 'abc', ]false", 1, QTYPE_QBOOL);
> +}

Likewise.

> +
> +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:

       /*
        * 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).
        */

> +    check_result_error("\ffalse", 1);
> +    check_result_error("\f'str'", 1);
> +    check_result_error("\f\"str\"", 1);
> +}
> +
> +static void test_lexer_recovery_nested(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);
> +
> +    /* same as test_lexer_recovery */

Really?

> +    check_result_error("{[{\ffalse", 1);
> +    check_result_error("{[{\f'str'", 1);
> +    check_result_error("{[{\f\"str\"", 1);
> +}
> +
> +static void test_nested(void)
> +{
> +    check_result("[{'a']}false", 1, QTYPE_QBOOL);
> +}
> +
> +static void test_nested_multiple(void)
> +{
> +    check_result("[{'a']}[{'a']}false", 2, QTYPE_QBOOL);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    g_test_init(&argc, &argv, NULL);
> +
> +    g_test_add_func("/json-parser/simple", test_simple);
> +    g_test_add_func("/json-parser/whitespace", test_whitespace);
> +    g_test_add_func("/json-parser/error-recovery/extra-closing-braces", test_extra_closing_braces);
> +    g_test_add_func("/json-parser/error-recovery/bad-dict", test_bad_dict);
> +    g_test_add_func("/json-parser/error-recovery/trailing-comma", test_trailing_comma);
> +    g_test_add_func("/json-parser/error-recovery/lexer", test_lexer_recovery);
> +    g_test_add_func("/json-parser/error-recovery/lexer/nested", test_lexer_recovery_nested);
> +    g_test_add_func("/json-parser/error-recovery/nested", test_nested);
> +    g_test_add_func("/json-parser/error-recovery/nested/multiple", test_nested_multiple);
> +
> +    return g_test_run();
> +}
> diff --git a/tests/unit/meson.build b/tests/unit/meson.build
> index 41e8b06c339..03d36748c73 100644
> --- a/tests/unit/meson.build
> +++ b/tests/unit/meson.build
> @@ -10,6 +10,7 @@ tests = {
>    'check-qnull': [],
>    'check-qobject': [],
>    'check-qjson': [],
> +  'check-json-parser': [],
>    'check-qlit': [],
>    'test-error-report': [],
>    'test-qobject-output-visitor': [testqapi],

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.

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 :)