[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 2/3] tests: qapi: Test 'features' of commands
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v3 2/3] tests: qapi: Test 'features' of commands |
Date: |
Fri, 11 Oct 2019 10:06:11 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) |
Peter Krempa <address@hidden> writes:
> On Thu, Oct 10, 2019 at 15:53:30 +0200, Markus Armbruster wrote:
>> Peter Krempa <address@hidden> writes:
>>
>> > Signed-off-by: Peter Krempa <address@hidden>
>> > ---
>> > tests/qapi-schema/qapi-schema-test.json | 26 ++++++++++++++++++++++
>> > tests/qapi-schema/qapi-schema-test.out | 29 +++++++++++++++++++++++++
>> > tests/qapi-schema/test-qapi.py | 4 ++++
>> > tests/test-qmp-cmds.c | 28 ++++++++++++++++++++++++
>> > 4 files changed, 87 insertions(+)
>>
>> More thorough than I asked for. I'm not complaining :)
>>
>> >
>> > diff --git a/tests/qapi-schema/qapi-schema-test.json
>> > b/tests/qapi-schema/qapi-schema-test.json
>> > index 75c42eb0e3..220859d4c9 100644
>> > --- a/tests/qapi-schema/qapi-schema-test.json
>> > +++ b/tests/qapi-schema/qapi-schema-test.json
>> > @@ -290,3 +290,29 @@
>> > 'cfs1': 'CondFeatureStruct1',
>> > 'cfs2': 'CondFeatureStruct2',
>> > 'cfs3': 'CondFeatureStruct3' } }
>> > +
>> > +# test 'features' for command
>> > +
>> > +{ 'command': 'test-command-features1',
>> > + 'features': [] }
>> > +
>> > +{ 'command': 'test-command-features2',
>> > + 'features': [ 'feature1' ] }
>> > +
>> > +{ 'command': 'test-command-features3',
>> > + 'features': [ 'feature1', 'feature2' ] }
>> > +
>> > +{ 'command': 'test-command-features4',
>> > + 'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'}
>> > ] }
>> > +
>> > +{ 'command': 'test-command-features5',
>> > + 'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'},
>> > + { 'name': 'feature2', 'if': 'defined(TEST_IF_FEATURE_2)'}
>> > ] }
>> > +
>> > +{ 'command': 'test-command-features6',
>> > + 'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'},
>> > + { 'name': 'feature2', 'if': 'defined(TEST_IF_FEATURE_2)'}
>> > ] }
>>
>> Do you need both test-command-features5 and 6? They look the same to me...
>
> No. It can be dropped. Looks like I mistakenly appropriated
> 'CondFeatureStruct2' test twice :/
Easy enough to fix :)
>> > +{ 'command': 'test-command-features7',
>> > + 'features': [ { 'name': 'feature1', 'if': [ 'defined(TEST_IF_COND_1)',
>> > + 'defined(TEST_IF_COND_2)']
>> > } ] }
>> > diff --git a/tests/qapi-schema/qapi-schema-test.out
>> > b/tests/qapi-schema/qapi-schema-test.out
>> > index 98031da96f..a38e348d54 100644
>> > --- a/tests/qapi-schema/qapi-schema-test.out
>> > +++ b/tests/qapi-schema/qapi-schema-test.out
>> > @@ -412,3 +412,32 @@ object q_obj_test-features-arg
>> > member cfs3: CondFeatureStruct3 optional=False
>> > command test-features q_obj_test-features-arg -> None
>> > gen=True success_response=True boxed=False oob=False preconfig=False
>> > +command test-command-features1 None -> None
>> > + gen=True success_response=True boxed=False oob=False preconfig=False
>> > +command test-command-features2 None -> None
>> > + gen=True success_response=True boxed=False oob=False preconfig=False
>> > + feature feature1
>> > +command test-command-features3 None -> None
>> > + gen=True success_response=True boxed=False oob=False preconfig=False
>> > + feature feature1
>> > + feature feature2
>> > +command test-command-features4 None -> None
>> > + gen=True success_response=True boxed=False oob=False preconfig=False
>> > + feature feature1
>> > + if ['defined(TEST_IF_FEATURE_1)']
>> > +command test-command-features5 None -> None
>> > + gen=True success_response=True boxed=False oob=False preconfig=False
>> > + feature feature1
>> > + if ['defined(TEST_IF_FEATURE_1)']
>> > + feature feature2
>> > + if ['defined(TEST_IF_FEATURE_2)']
>> > +command test-command-features6 None -> None
>> > + gen=True success_response=True boxed=False oob=False preconfig=False
>> > + feature feature1
>> > + if ['defined(TEST_IF_FEATURE_1)']
>> > + feature feature2
>> > + if ['defined(TEST_IF_FEATURE_2)']
>> > +command test-command-features7 None -> None
>> > + gen=True success_response=True boxed=False oob=False preconfig=False
>> > + feature feature1
>> > + if ['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)']
>> > diff --git a/tests/qapi-schema/test-qapi.py
>> > b/tests/qapi-schema/test-qapi.py
>> > index e13c2e8671..62e65b6a7d 100755
>> > --- a/tests/qapi-schema/test-qapi.py
>> > +++ b/tests/qapi-schema/test-qapi.py
>> > @@ -80,6 +80,10 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
>> > print(' gen=%s success_response=%s boxed=%s oob=%s preconfig=%s'
>> > % (gen, success_response, boxed, allow_oob,
>> > allow_preconfig))
>> > self._print_if(ifcond)
>> > + if features:
>> > + for f in features:
>> > + print(' feature %s' % f.name)
>> > + self._print_if(f.ifcond, 8)
>>
>> Copied from visit_object_type(). Let's factor it into a @staticmethod
>> _print_features().
>
> I'm not sure if that's intentional but the 'struct' and 'command'
> feature printers differ in indentation level by one space. I went for
> aligning it with the 'gen' line above. I thought it's for visual
> separation with other possible lines.
You're right. I think I simply messed up the spacing in commit
156402e5042. I'd like to clean it up.
I think it's easiest if I do the touch-ups here and in PATCH 1 (they're
all straightforward). I'll post them as v4 so you can give them a quick
eye-over.
>> > def visit_event(self, name, info, ifcond, arg_type, boxed):
>> > print('event %s %s' % (name, arg_type and arg_type.name))
>> > diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
>> > index 36fdf5b115..19f6e06ba7 100644
>> > --- a/tests/test-qmp-cmds.c
>> > +++ b/tests/test-qmp-cmds.c
>> > @@ -51,6 +51,34 @@ void qmp_test_features(FeatureStruct0 *fs0,
>> > FeatureStruct1 *fs1,
>> > {
>> > }
>> >
>> > +void qmp_test_command_features1(Error **errp)
>> > +{
>> > +}
>> > +
>> > +void qmp_test_command_features2(Error **errp)
>> > +{
>> > +}
>> > +
>> > +void qmp_test_command_features3(Error **errp)
>> > +{
>> > +}
>> > +
>> > +void qmp_test_command_features4(Error **errp)
>> > +{
>> > +}
>> > +
>> > +void qmp_test_command_features5(Error **errp)
>> > +{
>> > +}
>> > +
>> > +void qmp_test_command_features6(Error **errp)
>> > +{
>> > +}
>> > +
>> > +void qmp_test_command_features7(Error **errp)
>> > +{
>> > +}
>> > +
>> > UserDefTwo *qmp_user_def_cmd2(UserDefOne *ud1a,
>> > bool has_udb1, UserDefOne *ud1b,
>> > Error **errp)
>>
>> Any particular reason why we shouldn't squash this into PATCH 1?
>
> Not really. I tend to prefer tests added separately and it was also done
> so in case of features for 'structs' so I went with that approach. Said
> that I'm perfectly fine with squashing them.
Okay.