Re: [Qemu-devel] [PATCH v5 19/28] qapi: Add some type check tests

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Demonstrate that the qapi generator silently parses confusing
> types, which may cause other errors later on. Later patches
> will update the expected results as the generator is made stricter.
>
> Signed-off-by: Eric Blake <address@hidden>
> ---
[...]
> diff --git a/tests/qapi-schema/data-array-empty.err 
> b/tests/qapi-schema/data-array-empty.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/data-array-empty.exit 
> b/tests/qapi-schema/data-array-empty.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/data-array-empty.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/data-array-empty.json 
> b/tests/qapi-schema/data-array-empty.json
> new file mode 100644
> index 0000000..edb543a
> --- /dev/null
> +++ b/tests/qapi-schema/data-array-empty.json
> @@ -0,0 +1,2 @@
> +# FIXME: we should reject an array for data if it does not contain a known 
> type
> +{ 'command': 'oops', 'data': { 'empty': [ ] } }

v4 tested

    { 'command': 'oops', 'data': [ ] }

Is that still covered somewhere else?

> diff --git a/tests/qapi-schema/data-array-empty.out 
> b/tests/qapi-schema/data-array-empty.out
> new file mode 100644
> index 0000000..6f25c9e
> --- /dev/null
> +++ b/tests/qapi-schema/data-array-empty.out
> @@ -0,0 +1,3 @@
> +[OrderedDict([('command', 'oops'), ('data', OrderedDict([('empty', [])]))])]
> +[]
> +[]
[...]
> diff --git a/tests/qapi-schema/returns-whitelist.err 
> b/tests/qapi-schema/returns-whitelist.err
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/returns-whitelist.exit 
> b/tests/qapi-schema/returns-whitelist.exit
> new file mode 100644
> index 0000000..573541a
> --- /dev/null
> +++ b/tests/qapi-schema/returns-whitelist.exit
> @@ -0,0 +1 @@
> +0
> diff --git a/tests/qapi-schema/returns-whitelist.json 
> b/tests/qapi-schema/returns-whitelist.json
> new file mode 100644
> index 0000000..8328563
> --- /dev/null
> +++ b/tests/qapi-schema/returns-whitelist.json
> @@ -0,0 +1,11 @@
> +# FIXME: we should enforce that 'returns' be a dict or array of dict unless 
> whitelisted
> +{ 'command': 'human-monitor-command',
> +  'data': {'command-line': 'str', '*cpu-index': 'int'},
> +  'returns': 'str' }
> +{ 'enum': 'TpmModel', 'data': [ 'tpm-tis' ] }
> +{ 'command': 'query-tpm-models', 'returns': ['TpmModel'] }
> +{ 'command': 'guest-get-time',
> +  'returns': 'int' }
> +
> +{ 'command': 'no-way-this-will-get-whitelisted',
> +  'returns': [ 'int' ] }

I like the pattern "add tests to demonstrate issues, then code to
address the issues".  But this test appears too much out of the blue for
my taste.

You could use the commit message to prepare the reader.

Or you could deviate from the pattern and add this test together with
the actual whitelist.  That's what I'd try.

> diff --git a/tests/qapi-schema/returns-whitelist.out 
> b/tests/qapi-schema/returns-whitelist.out
> new file mode 100644
> index 0000000..2adcd8b
> --- /dev/null
> +++ b/tests/qapi-schema/returns-whitelist.out
> @@ -0,0 +1,7 @@
> +[OrderedDict([('command', 'human-monitor-command'), ('data', 
> OrderedDict([('command-line', 'str'), ('*cpu-index', 'int')])), ('returns', 
> 'str')]),
> + OrderedDict([('enum', 'TpmModel'), ('data', ['tpm-tis'])]),
> + OrderedDict([('command', 'query-tpm-models'), ('returns', ['TpmModel'])]),
> + OrderedDict([('command', 'guest-get-time'), ('returns', 'int')]),
> + OrderedDict([('command', 'no-way-this-will-get-whitelisted'), ('returns', 
> ['int'])])]
> +[{'enum_name': 'TpmModel', 'enum_values': ['tpm-tis']}]
> +[]

Since I found nothing that's actually wrong:

Reviewed-by: Markus Armbruster <address@hidden>