| Message ID | 20170911110623.24981-25-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Hi, | expand |
Marc-André Lureau <marcandre.lureau@redhat.com> writes: > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > tests/Makefile.include | 2 ++ > tests/qapi-schema/struct-if-invalid.err | 1 + > tests/qapi-schema/struct-if-invalid.exit | 1 + > tests/qapi-schema/struct-if-invalid.json | 3 +++ > tests/qapi-schema/struct-if-invalid.out | 0 > tests/qapi-schema/struct-member-type.err | 0 > tests/qapi-schema/struct-member-type.exit | 1 + > tests/qapi-schema/struct-member-type.json | 2 ++ > tests/qapi-schema/struct-member-type.out | 12 ++++++++++++ > 9 files changed, 22 insertions(+) > create mode 100644 tests/qapi-schema/struct-if-invalid.err > create mode 100644 tests/qapi-schema/struct-if-invalid.exit > create mode 100644 tests/qapi-schema/struct-if-invalid.json > create mode 100644 tests/qapi-schema/struct-if-invalid.out > create mode 100644 tests/qapi-schema/struct-member-type.err > create mode 100644 tests/qapi-schema/struct-member-type.exit > create mode 100644 tests/qapi-schema/struct-member-type.json > create mode 100644 tests/qapi-schema/struct-member-type.out > > diff --git a/tests/Makefile.include b/tests/Makefile.include > index 0aa532f029..44a3d8895e 100644 > --- a/tests/Makefile.include > +++ b/tests/Makefile.include > @@ -520,7 +520,9 @@ qapi-schema += returns-whitelist.json > qapi-schema += struct-base-clash-deep.json > qapi-schema += struct-base-clash.json > qapi-schema += struct-data-invalid.json > +qapi-schema += struct-if-invalid.json > qapi-schema += struct-member-invalid.json > +qapi-schema += struct-member-type.json > qapi-schema += trailing-comma-list.json > qapi-schema += trailing-comma-object.json > qapi-schema += type-bypass-bad-gen.json > diff --git a/tests/qapi-schema/struct-if-invalid.err b/tests/qapi-schema/struct-if-invalid.err > new file mode 100644 > index 0000000000..42245262a9 > --- /dev/null > +++ b/tests/qapi-schema/struct-if-invalid.err > @@ -0,0 +1 @@ > +tests/qapi-schema/struct-if-invalid.json:2: Member 'bar' of 'data' for struct 'TestIfStruct' should be a type name > diff --git a/tests/qapi-schema/struct-if-invalid.exit b/tests/qapi-schema/struct-if-invalid.exit > new file mode 100644 > index 0000000000..d00491fd7e > --- /dev/null > +++ b/tests/qapi-schema/struct-if-invalid.exit > @@ -0,0 +1 @@ > +1 > diff --git a/tests/qapi-schema/struct-if-invalid.json b/tests/qapi-schema/struct-if-invalid.json > new file mode 100644 > index 0000000000..466cdb61e1 > --- /dev/null > +++ b/tests/qapi-schema/struct-if-invalid.json > @@ -0,0 +1,3 @@ > +# check missing 'type' > +{ 'struct': 'TestIfStruct', 'data': > + { 'foo': 'int', 'bar': { 'if': 'defined(TEST_IF_STRUCT_BAR)' } } } Hmm. This tests the previous patch's change to check_type(). It demonstrates that the "should be a type name" error message can be suboptimal: it gets triggered when not isinstance(value, str) and not (isinstance(value, dict) and 'type' in value) Fine when the value is neither str not dict. Suboptimal when it's dict and 'type' is missing. A more obvious example of suboptimality would be 'bar': { 'tvpe': 'int' } The previous patch's if isinstance(value, dict) and 'type' in value: check_type(info, source, value['type'], allow_array, allow_dict, allow_optional, allow_metas) if 'if' in value: check_if(value, info) check_unknown_keys(info, value, {'type', 'if'}) return else: raise QAPISemError(info, "%s should be a type name" % source) should be improved to something like if not isinstance(value, dict): raise QAPISemError( info, "%s should be a type name or dictionary" % source) if 'type' not in value: raise QAPISemError( info, "%s must have a member 'type'" % source) check_type(info, source, value['type'], allow_array, allow_dict, allow_optional, allow_metas) if 'if' in value: check_if(value, info) check_unknown_keys(info, value, {'type', 'if'}) return producing struct-if-invalid.json:2: Member 'bar' of 'data' for struct 'TestIfStruct' must have a member 'type' The fact that I missed this in review of the code, but noticed it in the tests is why I want tests added together with the code they test. Nitpick: the file name struct-if-invalid makes me expect an invalid if. Not the case. It's a missing type. Let's rename accordingly. > diff --git a/tests/qapi-schema/struct-if-invalid.out b/tests/qapi-schema/struct-if-invalid.out > new file mode 100644 > index 0000000000..e69de29bb2 > diff --git a/tests/qapi-schema/struct-member-type.err b/tests/qapi-schema/struct-member-type.err > new file mode 100644 > index 0000000000..e69de29bb2 > diff --git a/tests/qapi-schema/struct-member-type.exit b/tests/qapi-schema/struct-member-type.exit > new file mode 100644 > index 0000000000..573541ac97 > --- /dev/null > +++ b/tests/qapi-schema/struct-member-type.exit > @@ -0,0 +1 @@ > +0 > diff --git a/tests/qapi-schema/struct-member-type.json b/tests/qapi-schema/struct-member-type.json > new file mode 100644 > index 0000000000..8b33027817 > --- /dev/null > +++ b/tests/qapi-schema/struct-member-type.json > @@ -0,0 +1,2 @@ > +# check member 'a' with 'type' key only > +{ 'struct': 'foo', 'data': { 'a': { 'type': 'str' } } } > diff --git a/tests/qapi-schema/struct-member-type.out b/tests/qapi-schema/struct-member-type.out > new file mode 100644 > index 0000000000..04b969d2e3 > --- /dev/null > +++ b/tests/qapi-schema/struct-member-type.out > @@ -0,0 +1,12 @@ > +enum QType > + prefix QTYPE > + member none: > + member qnull: > + member qnum: > + member qstring: > + member qdict: > + member qlist: > + member qbool: > +object foo > + member a: str optional=False > +object q_empty This is a positive test, isn't it? Positive tests go into qapi-schema-test.json.
Hi On Sat, Dec 9, 2017 at 10:07 AM, Markus Armbruster <armbru@redhat.com> wrote: > Marc-André Lureau <marcandre.lureau@redhat.com> writes: > >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> --- >> tests/Makefile.include | 2 ++ >> tests/qapi-schema/struct-if-invalid.err | 1 + >> tests/qapi-schema/struct-if-invalid.exit | 1 + >> tests/qapi-schema/struct-if-invalid.json | 3 +++ >> tests/qapi-schema/struct-if-invalid.out | 0 >> tests/qapi-schema/struct-member-type.err | 0 >> tests/qapi-schema/struct-member-type.exit | 1 + >> tests/qapi-schema/struct-member-type.json | 2 ++ >> tests/qapi-schema/struct-member-type.out | 12 ++++++++++++ >> 9 files changed, 22 insertions(+) >> create mode 100644 tests/qapi-schema/struct-if-invalid.err >> create mode 100644 tests/qapi-schema/struct-if-invalid.exit >> create mode 100644 tests/qapi-schema/struct-if-invalid.json >> create mode 100644 tests/qapi-schema/struct-if-invalid.out >> create mode 100644 tests/qapi-schema/struct-member-type.err >> create mode 100644 tests/qapi-schema/struct-member-type.exit >> create mode 100644 tests/qapi-schema/struct-member-type.json >> create mode 100644 tests/qapi-schema/struct-member-type.out >> >> diff --git a/tests/Makefile.include b/tests/Makefile.include >> index 0aa532f029..44a3d8895e 100644 >> --- a/tests/Makefile.include >> +++ b/tests/Makefile.include >> @@ -520,7 +520,9 @@ qapi-schema += returns-whitelist.json >> qapi-schema += struct-base-clash-deep.json >> qapi-schema += struct-base-clash.json >> qapi-schema += struct-data-invalid.json >> +qapi-schema += struct-if-invalid.json >> qapi-schema += struct-member-invalid.json >> +qapi-schema += struct-member-type.json >> qapi-schema += trailing-comma-list.json >> qapi-schema += trailing-comma-object.json >> qapi-schema += type-bypass-bad-gen.json >> diff --git a/tests/qapi-schema/struct-if-invalid.err b/tests/qapi-schema/struct-if-invalid.err >> new file mode 100644 >> index 0000000000..42245262a9 >> --- /dev/null >> +++ b/tests/qapi-schema/struct-if-invalid.err >> @@ -0,0 +1 @@ >> +tests/qapi-schema/struct-if-invalid.json:2: Member 'bar' of 'data' for struct 'TestIfStruct' should be a type name >> diff --git a/tests/qapi-schema/struct-if-invalid.exit b/tests/qapi-schema/struct-if-invalid.exit >> new file mode 100644 >> index 0000000000..d00491fd7e >> --- /dev/null >> +++ b/tests/qapi-schema/struct-if-invalid.exit >> @@ -0,0 +1 @@ >> +1 >> diff --git a/tests/qapi-schema/struct-if-invalid.json b/tests/qapi-schema/struct-if-invalid.json >> new file mode 100644 >> index 0000000000..466cdb61e1 >> --- /dev/null >> +++ b/tests/qapi-schema/struct-if-invalid.json >> @@ -0,0 +1,3 @@ >> +# check missing 'type' >> +{ 'struct': 'TestIfStruct', 'data': >> + { 'foo': 'int', 'bar': { 'if': 'defined(TEST_IF_STRUCT_BAR)' } } } > > Hmm. This tests the previous patch's change to check_type(). It > demonstrates that the "should be a type name" error message can be > suboptimal: it gets triggered when > > not isinstance(value, str) > and not (isinstance(value, dict) and 'type' in value) > > Fine when the value is neither str not dict. Suboptimal when it's dict > and 'type' is missing. A more obvious example of suboptimality would be > > 'bar': { 'tvpe': 'int' } > > The previous patch's > > if isinstance(value, dict) and 'type' in value: > check_type(info, source, value['type'], allow_array, > allow_dict, allow_optional, allow_metas) > if 'if' in value: > check_if(value, info) > check_unknown_keys(info, value, {'type', 'if'}) > return > else: > raise QAPISemError(info, "%s should be a type name" % source) > > should be improved to something like > > if not isinstance(value, dict): > raise QAPISemError( > info, "%s should be a type name or dictionary" % source) > if 'type' not in value: > raise QAPISemError( > info, "%s must have a member 'type'" % source) > check_type(info, source, value['type'], allow_array, > allow_dict, allow_optional, allow_metas) > if 'if' in value: > check_if(value, info) > check_unknown_keys(info, value, {'type', 'if'}) > return > > producing > > struct-if-invalid.json:2: Member 'bar' of 'data' for struct 'TestIfStruct' must have a member 'type' > Fixed differently in v4 > The fact that I missed this in review of the code, but noticed it in the > tests is why I want tests added together with the code they test. > Changed in v4 > Nitpick: the file name struct-if-invalid makes me expect an invalid if. > Not the case. It's a missing type. Let's rename accordingly. Done > >> diff --git a/tests/qapi-schema/struct-if-invalid.out b/tests/qapi-schema/struct-if-invalid.out >> new file mode 100644 >> index 0000000000..e69de29bb2 >> diff --git a/tests/qapi-schema/struct-member-type.err b/tests/qapi-schema/struct-member-type.err >> new file mode 100644 >> index 0000000000..e69de29bb2 >> diff --git a/tests/qapi-schema/struct-member-type.exit b/tests/qapi-schema/struct-member-type.exit >> new file mode 100644 >> index 0000000000..573541ac97 >> --- /dev/null >> +++ b/tests/qapi-schema/struct-member-type.exit >> @@ -0,0 +1 @@ >> +0 >> diff --git a/tests/qapi-schema/struct-member-type.json b/tests/qapi-schema/struct-member-type.json >> new file mode 100644 >> index 0000000000..8b33027817 >> --- /dev/null >> +++ b/tests/qapi-schema/struct-member-type.json >> @@ -0,0 +1,2 @@ >> +# check member 'a' with 'type' key only >> +{ 'struct': 'foo', 'data': { 'a': { 'type': 'str' } } } >> diff --git a/tests/qapi-schema/struct-member-type.out b/tests/qapi-schema/struct-member-type.out >> new file mode 100644 >> index 0000000000..04b969d2e3 >> --- /dev/null >> +++ b/tests/qapi-schema/struct-member-type.out >> @@ -0,0 +1,12 @@ >> +enum QType >> + prefix QTYPE >> + member none: >> + member qnull: >> + member qnum: >> + member qstring: >> + member qdict: >> + member qlist: >> + member qbool: >> +object foo >> + member a: str optional=False >> +object q_empty > > This is a positive test, isn't it? Positive tests go into > qapi-schema-test.json. > Right, I wonder why we have .exit files then. Perhaps the few ones that return 0 shouldn't exist. thanks
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Sat, Dec 9, 2017 at 10:07 AM, Markus Armbruster <armbru@redhat.com> wrote: >> Marc-André Lureau <marcandre.lureau@redhat.com> writes: [...] >>> diff --git a/tests/qapi-schema/struct-member-type.json b/tests/qapi-schema/struct-member-type.json >>> new file mode 100644 >>> index 0000000000..8b33027817 >>> --- /dev/null >>> +++ b/tests/qapi-schema/struct-member-type.json >>> @@ -0,0 +1,2 @@ >>> +# check member 'a' with 'type' key only >>> +{ 'struct': 'foo', 'data': { 'a': { 'type': 'str' } } } >>> diff --git a/tests/qapi-schema/struct-member-type.out b/tests/qapi-schema/struct-member-type.out >>> new file mode 100644 >>> index 0000000000..04b969d2e3 >>> --- /dev/null >>> +++ b/tests/qapi-schema/struct-member-type.out >>> @@ -0,0 +1,12 @@ >>> +enum QType >>> + prefix QTYPE >>> + member none: >>> + member qnull: >>> + member qnum: >>> + member qstring: >>> + member qdict: >>> + member qlist: >>> + member qbool: >>> +object foo >>> + member a: str optional=False >>> +object q_empty >> >> This is a positive test, isn't it? Positive tests go into >> qapi-schema-test.json. >> > > Right, I wonder why we have .exit files then. Perhaps the few ones > that return 0 shouldn't exist. There are a few legitimate positive test cases, such as empty.json and doc-good.json. Moreover, we occasionally add negative test cases that fail to fail, demonstrating a bug. Example: quoted-structural-chars in commit 98626572f1, fixed in commit c7a3f25200.
diff --git a/tests/Makefile.include b/tests/Makefile.include index 0aa532f029..44a3d8895e 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -520,7 +520,9 @@ qapi-schema += returns-whitelist.json qapi-schema += struct-base-clash-deep.json qapi-schema += struct-base-clash.json qapi-schema += struct-data-invalid.json +qapi-schema += struct-if-invalid.json qapi-schema += struct-member-invalid.json +qapi-schema += struct-member-type.json qapi-schema += trailing-comma-list.json qapi-schema += trailing-comma-object.json qapi-schema += type-bypass-bad-gen.json diff --git a/tests/qapi-schema/struct-if-invalid.err b/tests/qapi-schema/struct-if-invalid.err new file mode 100644 index 0000000000..42245262a9 --- /dev/null +++ b/tests/qapi-schema/struct-if-invalid.err @@ -0,0 +1 @@ +tests/qapi-schema/struct-if-invalid.json:2: Member 'bar' of 'data' for struct 'TestIfStruct' should be a type name diff --git a/tests/qapi-schema/struct-if-invalid.exit b/tests/qapi-schema/struct-if-invalid.exit new file mode 100644 index 0000000000..d00491fd7e --- /dev/null +++ b/tests/qapi-schema/struct-if-invalid.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/struct-if-invalid.json b/tests/qapi-schema/struct-if-invalid.json new file mode 100644 index 0000000000..466cdb61e1 --- /dev/null +++ b/tests/qapi-schema/struct-if-invalid.json @@ -0,0 +1,3 @@ +# check missing 'type' +{ 'struct': 'TestIfStruct', 'data': + { 'foo': 'int', 'bar': { 'if': 'defined(TEST_IF_STRUCT_BAR)' } } } diff --git a/tests/qapi-schema/struct-if-invalid.out b/tests/qapi-schema/struct-if-invalid.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/struct-member-type.err b/tests/qapi-schema/struct-member-type.err new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/struct-member-type.exit b/tests/qapi-schema/struct-member-type.exit new file mode 100644 index 0000000000..573541ac97 --- /dev/null +++ b/tests/qapi-schema/struct-member-type.exit @@ -0,0 +1 @@ +0 diff --git a/tests/qapi-schema/struct-member-type.json b/tests/qapi-schema/struct-member-type.json new file mode 100644 index 0000000000..8b33027817 --- /dev/null +++ b/tests/qapi-schema/struct-member-type.json @@ -0,0 +1,2 @@ +# check member 'a' with 'type' key only +{ 'struct': 'foo', 'data': { 'a': { 'type': 'str' } } } diff --git a/tests/qapi-schema/struct-member-type.out b/tests/qapi-schema/struct-member-type.out new file mode 100644 index 0000000000..04b969d2e3 --- /dev/null +++ b/tests/qapi-schema/struct-member-type.out @@ -0,0 +1,12 @@ +enum QType + prefix QTYPE + member none: + member qnull: + member qnum: + member qstring: + member qdict: + member qlist: + member qbool: +object foo + member a: str optional=False +object q_empty
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- tests/Makefile.include | 2 ++ tests/qapi-schema/struct-if-invalid.err | 1 + tests/qapi-schema/struct-if-invalid.exit | 1 + tests/qapi-schema/struct-if-invalid.json | 3 +++ tests/qapi-schema/struct-if-invalid.out | 0 tests/qapi-schema/struct-member-type.err | 0 tests/qapi-schema/struct-member-type.exit | 1 + tests/qapi-schema/struct-member-type.json | 2 ++ tests/qapi-schema/struct-member-type.out | 12 ++++++++++++ 9 files changed, 22 insertions(+) create mode 100644 tests/qapi-schema/struct-if-invalid.err create mode 100644 tests/qapi-schema/struct-if-invalid.exit create mode 100644 tests/qapi-schema/struct-if-invalid.json create mode 100644 tests/qapi-schema/struct-if-invalid.out create mode 100644 tests/qapi-schema/struct-member-type.err create mode 100644 tests/qapi-schema/struct-member-type.exit create mode 100644 tests/qapi-schema/struct-member-type.json create mode 100644 tests/qapi-schema/struct-member-type.out