From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53616) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbC3Z-0000CK-IL for qemu-devel@nongnu.org; Thu, 26 Mar 2015 13:59:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YbC3Y-0005QA-Gi for qemu-devel@nongnu.org; Thu, 26 Mar 2015 13:59:01 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58507) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbC3Y-0005Pv-9D for qemu-devel@nongnu.org; Thu, 26 Mar 2015 13:59:00 -0400 From: Markus Armbruster References: <1427227433-5030-1-git-send-email-eblake@redhat.com> <1427227433-5030-20-git-send-email-eblake@redhat.com> Date: Thu, 26 Mar 2015 18:58:55 +0100 In-Reply-To: <1427227433-5030-20-git-send-email-eblake@redhat.com> (Eric Blake's message of "Tue, 24 Mar 2015 14:03:44 -0600") Message-ID: <87lhijwsgw.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v5 19/28] qapi: Add some type check tests List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, lcapitulino@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, wenchaoqemu@gmail.com Eric Blake 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 > --- [...] > 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