| Message ID | 1443930073-19359-4-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > The alternate-good.json test was already covered by > qapi-schema-test.json. As future commits will be tweaking > how alternates are laid out, removing the duplicate test now > reduces churn. Do we have more positive tests? They should probably all live in qapi-schema-test.json, because there their generated code actually gets compiled. Patch looks good.
On 10/07/2015 10:15 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> The alternate-good.json test was already covered by >> qapi-schema-test.json. As future commits will be tweaking >> how alternates are laid out, removing the duplicate test now >> reduces churn. > > Do we have more positive tests? They should probably all live in > qapi-schema-test.json, because there their generated code actually gets > compiled. Hmm, any test that has an empty .err and non-empty .out, but which does not also have an TODO/FIXME stating that it is a bug, is worth checking. So first, here's the list of non-empty .out files: empty.out [1] enum-empty.out event-case.out [4] union-empty.out [2] include-simple.out include-repetition.out include-relpath.out comments.out alternate-empty.out [2] returns-int.out struct-base-clash-base.out [3] flat-union-empty.out [2] indented-expr.out union-clash-data.out [3] ident-with-escape.out args-member-array.out flat-union-reverse-define.out [1] must stay separate, because we can't really make qapi-schema-test empty :) [2] marked FIXME, and turns into proper error message later in the v5 series [3] marked FIXME, and already removed later in the v5 series once a positive test is inlined into qapi-schema-test [4] marked TODO For the ones I didn't tag with a footnote, we could probably cover things directly in qapi-schema-test instead; the question then becomes which ones are already covered, and which ones need content moved. > > Patch looks good. >
Eric Blake <eblake@redhat.com> writes: > On 10/07/2015 10:15 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> The alternate-good.json test was already covered by >>> qapi-schema-test.json. As future commits will be tweaking >>> how alternates are laid out, removing the duplicate test now >>> reduces churn. >> >> Do we have more positive tests? They should probably all live in >> qapi-schema-test.json, because there their generated code actually gets >> compiled. > > Hmm, any test that has an empty .err and non-empty .out, but which does > not also have an TODO/FIXME stating that it is a bug, is worth checking. > So first, here's the list of non-empty .out files: > > empty.out [1] > enum-empty.out > event-case.out [4] > union-empty.out [2] > include-simple.out > include-repetition.out > include-relpath.out > comments.out > alternate-empty.out [2] > returns-int.out > struct-base-clash-base.out [3] > flat-union-empty.out [2] > indented-expr.out > union-clash-data.out [3] > ident-with-escape.out > args-member-array.out > flat-union-reverse-define.out > > [1] must stay separate, because we can't really make qapi-schema-test > empty :) > [2] marked FIXME, and turns into proper error message later in the v5 series > [3] marked FIXME, and already removed later in the v5 series once a > positive test is inlined into qapi-schema-test > [4] marked TODO > > For the ones I didn't tag with a footnote, we could probably cover > things directly in qapi-schema-test instead; the question then becomes > which ones are already covered, and which ones need content moved. Actually, the point isn't to move the positive test to qapi-schema-test.json, the point is to compile-test its generated code. Moving it to qapi-schema-test.json accomplishes that. However, we may not want a single, monolithic positive test. Should we split up qapi-schema-test.json instead? I don't know. Anyway, let's flush our queue first. Third case: the generated code isn't worth compile-testing; comparing actual to expected .out suffices. Let's sort your untagged tests into buckets: Not worth compile-testing: * comments.json * include-simple.json * include-repetition.json * include-relpath.json * indented-expr.json * ident-with-escape.json Not (completely) covered in qapi-schema-test.json: * enum-empty.json Not covered, but should be. * flat-union-reverse-define.json UserDefOne covers forward reference to struct base, UserDefFlatUnion covers forward reference to union base, and UserDefFlatUnion2 covers forward reference to member. We may want to cover forward reference to the tag member's type. Covered: * returns-int.json 'user_def_cmd3' does the job. * args-member-array.json '__org.qemu_x-command' seems good enough.
On 10/13/2015 02:12 AM, Markus Armbruster wrote: >> Hmm, any test that has an empty .err and non-empty .out, but which does >> not also have an TODO/FIXME stating that it is a bug, is worth checking. >> So first, here's the list of non-empty .out files: >> > > Actually, the point isn't to move the positive test to > qapi-schema-test.json, the point is to compile-test its generated code. > Moving it to qapi-schema-test.json accomplishes that. However, we may > not want a single, monolithic positive test. Should we split up > qapi-schema-test.json instead? I don't know. Anyway, let's flush our > queue first. If we do split qapi-schema-test, it won't be until after my patches are flushed :) > > Third case: the generated code isn't worth compile-testing; comparing > actual to expected .out suffices. > > Let's sort your untagged tests into buckets: > > Not worth compile-testing: > * comments.json > * include-simple.json > * include-repetition.json > * include-relpath.json > * indented-expr.json > * ident-with-escape.json > > Not (completely) covered in qapi-schema-test.json: > * enum-empty.json > Not covered, but should be. Covered in v8: https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02869.html > * flat-union-reverse-define.json > UserDefOne covers forward reference to struct base, UserDefFlatUnion > covers forward reference to union base, and UserDefFlatUnion2 covers > forward reference to member. We may want to cover forward reference > to the tag member's type. Covered in v8 > > Covered: > * returns-int.json > 'user_def_cmd3' does the job. Cleaned up in v8 > * args-member-array.json > '__org.qemu_x-command' seems good enough. Not cleaned up yet, so I'll add it to my next round of subset C.
diff --git a/tests/Makefile b/tests/Makefile index edf310d..d18b3b2 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -227,7 +227,6 @@ qapi-schema += alternate-clash.json qapi-schema += alternate-conflict-dict.json qapi-schema += alternate-conflict-string.json qapi-schema += alternate-empty.json -qapi-schema += alternate-good.json qapi-schema += alternate-nested.json qapi-schema += alternate-unknown.json qapi-schema += args-alternate.json diff --git a/tests/qapi-schema/alternate-good.err b/tests/qapi-schema/alternate-good.err deleted file mode 100644 index e69de29..0000000 diff --git a/tests/qapi-schema/alternate-good.exit b/tests/qapi-schema/alternate-good.exit deleted file mode 100644 index 573541a..0000000 --- a/tests/qapi-schema/alternate-good.exit +++ /dev/null @@ -1 +0,0 @@ -0 diff --git a/tests/qapi-schema/alternate-good.json b/tests/qapi-schema/alternate-good.json deleted file mode 100644 index 3371770..0000000 --- a/tests/qapi-schema/alternate-good.json +++ /dev/null @@ -1,9 +0,0 @@ -# Working example of alternate -{ 'struct': 'Data', - 'data': { '*number': 'int', '*name': 'str' } } -{ 'enum': 'Enum', - 'data': [ 'hello', 'world' ] } -{ 'alternate': 'Alt', - 'data': { 'value': 'int', - 'string': 'Enum', - 'struct': 'Data' } } diff --git a/tests/qapi-schema/alternate-good.out b/tests/qapi-schema/alternate-good.out deleted file mode 100644 index 65af727..0000000 --- a/tests/qapi-schema/alternate-good.out +++ /dev/null @@ -1,10 +0,0 @@ -object :empty -alternate Alt - case value: int - case string: Enum - case struct: Data -enum AltKind ['value', 'string', 'struct'] -object Data - member number: int optional=True - member name: str optional=True -enum Enum ['hello', 'world']
The alternate-good.json test was already covered by qapi-schema-test.json. As future commits will be tweaking how alternates are laid out, removing the duplicate test now reduces churn. Signed-off-by: Eric Blake <eblake@redhat.com> --- v7: new patch --- tests/Makefile | 1 - tests/qapi-schema/alternate-good.err | 0 tests/qapi-schema/alternate-good.exit | 1 - tests/qapi-schema/alternate-good.json | 9 --------- tests/qapi-schema/alternate-good.out | 10 ---------- 5 files changed, 21 deletions(-) delete mode 100644 tests/qapi-schema/alternate-good.err delete mode 100644 tests/qapi-schema/alternate-good.exit delete mode 100644 tests/qapi-schema/alternate-good.json delete mode 100644 tests/qapi-schema/alternate-good.out