| Message ID | 1535704738-8986-9-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,1/9] Remove the deprecated -balloon option | expand |
Thomas Huth <thuth@redhat.com> writes: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > test_qom_set_without_value() is about a bug in infrastructure used by > the QMP core, fixed in commit c489780203. We covered the bug in > infrastructure unit tests (commit bce3035a44). I wrote that test > earlier, to cover QMP level as well, the test could go into qmp-test. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Reviewed-by: Thomas Huth <thuth@redhat.com> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > tests/qmp-test.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/tests/qmp-test.c b/tests/qmp-test.c > index b347228..2b923f0 100644 > --- a/tests/qmp-test.c > +++ b/tests/qmp-test.c > @@ -321,6 +321,19 @@ static void test_qmp_preconfig(void) > qtest_quit(qs); > } > > +static void test_qom_set_without_value(void) > +{ > + QTestState *qts; > + QDict *resp; > + > + qts = qtest_init(common_args); > + resp = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':" > + " { 'path': '/machine', 'property': 'rtc-time' } }"); > + g_assert_nonnull(resp); > + qmp_assert_error_class(resp, "GenericError"); > + qtest_quit(qts); > +} > + > int main(int argc, char *argv[]) > { > g_test_init(&argc, &argv, NULL); > @@ -328,6 +341,7 @@ int main(int argc, char *argv[]) > qtest_add_func("qmp/protocol", test_qmp_protocol); > qtest_add_func("qmp/oob", test_qmp_oob); > qtest_add_func("qmp/preconfig", test_qmp_preconfig); > + qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value); > > return g_test_run(); > } I'm afraid you missed my objection to naming: Message-ID: <8736uvujxx.fsf@dusky.pond.sub.org> https://lists.nongnu.org/archive/html/qemu-devel/2018-08/msg06368.html If you could work that into PULL v2, I'd be obliged. If not, I'll have to address it in a follow-up patch.
On 2018-08-31 14:04, Markus Armbruster wrote: > Thomas Huth <thuth@redhat.com> writes: > >> From: Marc-André Lureau <marcandre.lureau@redhat.com> >> >> test_qom_set_without_value() is about a bug in infrastructure used by >> the QMP core, fixed in commit c489780203. We covered the bug in >> infrastructure unit tests (commit bce3035a44). I wrote that test >> earlier, to cover QMP level as well, the test could go into qmp-test. >> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> Reviewed-by: Thomas Huth <thuth@redhat.com> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> tests/qmp-test.c | 14 ++++++++++++++ >> 1 file changed, 14 insertions(+) >> >> diff --git a/tests/qmp-test.c b/tests/qmp-test.c >> index b347228..2b923f0 100644 >> --- a/tests/qmp-test.c >> +++ b/tests/qmp-test.c >> @@ -321,6 +321,19 @@ static void test_qmp_preconfig(void) >> qtest_quit(qs); >> } >> >> +static void test_qom_set_without_value(void) >> +{ >> + QTestState *qts; >> + QDict *resp; >> + >> + qts = qtest_init(common_args); >> + resp = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':" >> + " { 'path': '/machine', 'property': 'rtc-time' } }"); >> + g_assert_nonnull(resp); >> + qmp_assert_error_class(resp, "GenericError"); >> + qtest_quit(qts); >> +} >> + >> int main(int argc, char *argv[]) >> { >> g_test_init(&argc, &argv, NULL); >> @@ -328,6 +341,7 @@ int main(int argc, char *argv[]) >> qtest_add_func("qmp/protocol", test_qmp_protocol); >> qtest_add_func("qmp/oob", test_qmp_oob); >> qtest_add_func("qmp/preconfig", test_qmp_preconfig); >> + qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value); >> >> return g_test_run(); >> } > > I'm afraid you missed my objection to naming: > Message-ID: <8736uvujxx.fsf@dusky.pond.sub.org> > https://lists.nongnu.org/archive/html/qemu-devel/2018-08/msg06368.html Sorry about that, I was not on CC: for that series. I used the patches from v5 where Marc-André put me on CC:. > If you could work that into PULL v2, I'd be obliged. If not, I'll have > to address it in a follow-up patch. IMHO the naming is not that bad ... OTOH, I think Peter might already be away? ... so we've got plenty of time to sort this out anyway. Marc-André, could you send a new version of the patch? Thomas
Hi On Fri, Aug 31, 2018 at 3:18 PM Thomas Huth <thuth@redhat.com> wrote: > > On 2018-08-31 14:04, Markus Armbruster wrote: > > Thomas Huth <thuth@redhat.com> writes: > > > >> From: Marc-André Lureau <marcandre.lureau@redhat.com> > >> > >> test_qom_set_without_value() is about a bug in infrastructure used by > >> the QMP core, fixed in commit c489780203. We covered the bug in > >> infrastructure unit tests (commit bce3035a44). I wrote that test > >> earlier, to cover QMP level as well, the test could go into qmp-test. > >> > >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > >> Reviewed-by: Thomas Huth <thuth@redhat.com> > >> Signed-off-by: Thomas Huth <thuth@redhat.com> > >> --- > >> tests/qmp-test.c | 14 ++++++++++++++ > >> 1 file changed, 14 insertions(+) > >> > >> diff --git a/tests/qmp-test.c b/tests/qmp-test.c > >> index b347228..2b923f0 100644 > >> --- a/tests/qmp-test.c > >> +++ b/tests/qmp-test.c > >> @@ -321,6 +321,19 @@ static void test_qmp_preconfig(void) > >> qtest_quit(qs); > >> } > >> > >> +static void test_qom_set_without_value(void) > >> +{ > >> + QTestState *qts; > >> + QDict *resp; > >> + > >> + qts = qtest_init(common_args); > >> + resp = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':" > >> + " { 'path': '/machine', 'property': 'rtc-time' } }"); > >> + g_assert_nonnull(resp); > >> + qmp_assert_error_class(resp, "GenericError"); > >> + qtest_quit(qts); > >> +} > >> + > >> int main(int argc, char *argv[]) > >> { > >> g_test_init(&argc, &argv, NULL); > >> @@ -328,6 +341,7 @@ int main(int argc, char *argv[]) > >> qtest_add_func("qmp/protocol", test_qmp_protocol); > >> qtest_add_func("qmp/oob", test_qmp_oob); > >> qtest_add_func("qmp/preconfig", test_qmp_preconfig); > >> + qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value); > >> > >> return g_test_run(); > >> } > > > > I'm afraid you missed my objection to naming: > > Message-ID: <8736uvujxx.fsf@dusky.pond.sub.org> > > https://lists.nongnu.org/archive/html/qemu-devel/2018-08/msg06368.html > > Sorry about that, I was not on CC: for that series. I used the patches > from v5 where Marc-André put me on CC:. > > > If you could work that into PULL v2, I'd be obliged. If not, I'll have > > to address it in a follow-up patch. > > IMHO the naming is not that bad ... OTOH, I think Peter might already be > away? ... so we've got plenty of time to sort this out anyway. > Marc-André, could you send a new version of the patch? Tbh, I don't care so much about the naming of the test, but (for once) I respectfully don't think Markus suggestion is better. The function checks "qom-set" without 'value' argument: "qom-set-without-value", no brainer.. Naming it "invalid-arg" is so generic that I wouldn't be able what it does.
On 2018-08-31 15:24, Marc-André Lureau wrote: > Hi > On Fri, Aug 31, 2018 at 3:18 PM Thomas Huth <thuth@redhat.com> wrote: >> >> On 2018-08-31 14:04, Markus Armbruster wrote: >>> Thomas Huth <thuth@redhat.com> writes: >>> >>>> From: Marc-André Lureau <marcandre.lureau@redhat.com> >>>> >>>> test_qom_set_without_value() is about a bug in infrastructure used by >>>> the QMP core, fixed in commit c489780203. We covered the bug in >>>> infrastructure unit tests (commit bce3035a44). I wrote that test >>>> earlier, to cover QMP level as well, the test could go into qmp-test. >>>> >>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>> --- >>>> tests/qmp-test.c | 14 ++++++++++++++ >>>> 1 file changed, 14 insertions(+) >>>> >>>> diff --git a/tests/qmp-test.c b/tests/qmp-test.c >>>> index b347228..2b923f0 100644 >>>> --- a/tests/qmp-test.c >>>> +++ b/tests/qmp-test.c >>>> @@ -321,6 +321,19 @@ static void test_qmp_preconfig(void) >>>> qtest_quit(qs); >>>> } >>>> >>>> +static void test_qom_set_without_value(void) >>>> +{ >>>> + QTestState *qts; >>>> + QDict *resp; >>>> + >>>> + qts = qtest_init(common_args); >>>> + resp = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':" >>>> + " { 'path': '/machine', 'property': 'rtc-time' } }"); >>>> + g_assert_nonnull(resp); >>>> + qmp_assert_error_class(resp, "GenericError"); >>>> + qtest_quit(qts); >>>> +} >>>> + >>>> int main(int argc, char *argv[]) >>>> { >>>> g_test_init(&argc, &argv, NULL); >>>> @@ -328,6 +341,7 @@ int main(int argc, char *argv[]) >>>> qtest_add_func("qmp/protocol", test_qmp_protocol); >>>> qtest_add_func("qmp/oob", test_qmp_oob); >>>> qtest_add_func("qmp/preconfig", test_qmp_preconfig); >>>> + qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value); >>>> >>>> return g_test_run(); >>>> } >>> >>> I'm afraid you missed my objection to naming: >>> Message-ID: <8736uvujxx.fsf@dusky.pond.sub.org> >>> https://lists.nongnu.org/archive/html/qemu-devel/2018-08/msg06368.html >> >> Sorry about that, I was not on CC: for that series. I used the patches >> from v5 where Marc-André put me on CC:. >> >>> If you could work that into PULL v2, I'd be obliged. If not, I'll have >>> to address it in a follow-up patch. >> >> IMHO the naming is not that bad ... OTOH, I think Peter might already be >> away? ... so we've got plenty of time to sort this out anyway. >> Marc-André, could you send a new version of the patch? > > Tbh, I don't care so much about the naming of the test, but (for once) > I respectfully don't think Markus suggestion is better. > > The function checks "qom-set" without 'value' argument: > "qom-set-without-value", no brainer.. > > Naming it "invalid-arg" is so generic that I wouldn't be able what it does. Ok, then let's keep it this way. As I said, IMHO the current naming is not really bad, and I also don't have any suggestions for a perfect name right now. Thomas
Thomas Huth <thuth@redhat.com> writes: > On 2018-08-31 15:24, Marc-André Lureau wrote: >> Hi >> On Fri, Aug 31, 2018 at 3:18 PM Thomas Huth <thuth@redhat.com> wrote: >>> >>> On 2018-08-31 14:04, Markus Armbruster wrote: >>>> Thomas Huth <thuth@redhat.com> writes: >>>> >>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com> >>>>> >>>>> test_qom_set_without_value() is about a bug in infrastructure used by >>>>> the QMP core, fixed in commit c489780203. We covered the bug in >>>>> infrastructure unit tests (commit bce3035a44). I wrote that test >>>>> earlier, to cover QMP level as well, the test could go into qmp-test. >>>>> >>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>>>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>>> --- >>>>> tests/qmp-test.c | 14 ++++++++++++++ >>>>> 1 file changed, 14 insertions(+) >>>>> >>>>> diff --git a/tests/qmp-test.c b/tests/qmp-test.c >>>>> index b347228..2b923f0 100644 >>>>> --- a/tests/qmp-test.c >>>>> +++ b/tests/qmp-test.c >>>>> @@ -321,6 +321,19 @@ static void test_qmp_preconfig(void) >>>>> qtest_quit(qs); >>>>> } >>>>> >>>>> +static void test_qom_set_without_value(void) >>>>> +{ >>>>> + QTestState *qts; >>>>> + QDict *resp; >>>>> + >>>>> + qts = qtest_init(common_args); >>>>> + resp = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':" >>>>> + " { 'path': '/machine', 'property': 'rtc-time' } }"); >>>>> + g_assert_nonnull(resp); >>>>> + qmp_assert_error_class(resp, "GenericError"); >>>>> + qtest_quit(qts); >>>>> +} >>>>> + >>>>> int main(int argc, char *argv[]) >>>>> { >>>>> g_test_init(&argc, &argv, NULL); >>>>> @@ -328,6 +341,7 @@ int main(int argc, char *argv[]) >>>>> qtest_add_func("qmp/protocol", test_qmp_protocol); >>>>> qtest_add_func("qmp/oob", test_qmp_oob); >>>>> qtest_add_func("qmp/preconfig", test_qmp_preconfig); >>>>> + qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value); >>>>> >>>>> return g_test_run(); >>>>> } >>>> >>>> I'm afraid you missed my objection to naming: >>>> Message-ID: <8736uvujxx.fsf@dusky.pond.sub.org> >>>> https://lists.nongnu.org/archive/html/qemu-devel/2018-08/msg06368.html >>> >>> Sorry about that, I was not on CC: for that series. I used the patches >>> from v5 where Marc-André put me on CC:. >>> >>>> If you could work that into PULL v2, I'd be obliged. If not, I'll have >>>> to address it in a follow-up patch. >>> >>> IMHO the naming is not that bad ... OTOH, I think Peter might already be >>> away? ... so we've got plenty of time to sort this out anyway. >>> Marc-André, could you send a new version of the patch? >> >> Tbh, I don't care so much about the naming of the test, but (for once) >> I respectfully don't think Markus suggestion is better. >> >> The function checks "qom-set" without 'value' argument: >> "qom-set-without-value", no brainer.. Nope, that's not what it tests. It tests the visitor, the marshalling code generator, and the QMP core handle a certain kind of invalid arguments correctly. It does not test qom-set. I explained all that already. >> Naming it "invalid-arg" is so generic that I wouldn't be able what it does. I can accept "missing-any" or "missing-any-arg". I object to any name involving qom-set, because the test is not about qom-set at all. If it was, then putting it in qmp-test.c would be wrong. > Ok, then let's keep it this way. As I said, IMHO the current naming is > not really bad, and I also don't have any suggestions for a perfect name > right now. We don't need a perfect name. We need one that's not actively misleading.
On 2018-08-31 16:35, Markus Armbruster wrote: > Thomas Huth <thuth@redhat.com> writes: > >> On 2018-08-31 15:24, Marc-André Lureau wrote: >>> Hi >>> On Fri, Aug 31, 2018 at 3:18 PM Thomas Huth <thuth@redhat.com> wrote: >>>> >>>> On 2018-08-31 14:04, Markus Armbruster wrote: >>>>> Thomas Huth <thuth@redhat.com> writes: >>>>> >>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com> >>>>>> >>>>>> test_qom_set_without_value() is about a bug in infrastructure used by >>>>>> the QMP core, fixed in commit c489780203. We covered the bug in >>>>>> infrastructure unit tests (commit bce3035a44). I wrote that test >>>>>> earlier, to cover QMP level as well, the test could go into qmp-test. >>>>>> >>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>>>>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>>>> --- >>>>>> tests/qmp-test.c | 14 ++++++++++++++ >>>>>> 1 file changed, 14 insertions(+) >>>>>> >>>>>> diff --git a/tests/qmp-test.c b/tests/qmp-test.c >>>>>> index b347228..2b923f0 100644 >>>>>> --- a/tests/qmp-test.c >>>>>> +++ b/tests/qmp-test.c >>>>>> @@ -321,6 +321,19 @@ static void test_qmp_preconfig(void) >>>>>> qtest_quit(qs); >>>>>> } >>>>>> >>>>>> +static void test_qom_set_without_value(void) >>>>>> +{ >>>>>> + QTestState *qts; >>>>>> + QDict *resp; >>>>>> + >>>>>> + qts = qtest_init(common_args); >>>>>> + resp = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':" >>>>>> + " { 'path': '/machine', 'property': 'rtc-time' } }"); >>>>>> + g_assert_nonnull(resp); >>>>>> + qmp_assert_error_class(resp, "GenericError"); >>>>>> + qtest_quit(qts); >>>>>> +} >>>>>> + >>>>>> int main(int argc, char *argv[]) >>>>>> { >>>>>> g_test_init(&argc, &argv, NULL); >>>>>> @@ -328,6 +341,7 @@ int main(int argc, char *argv[]) >>>>>> qtest_add_func("qmp/protocol", test_qmp_protocol); >>>>>> qtest_add_func("qmp/oob", test_qmp_oob); >>>>>> qtest_add_func("qmp/preconfig", test_qmp_preconfig); >>>>>> + qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value); >>>>>> >>>>>> return g_test_run(); >>>>>> } >>>>> >>>>> I'm afraid you missed my objection to naming: >>>>> Message-ID: <8736uvujxx.fsf@dusky.pond.sub.org> >>>>> https://lists.nongnu.org/archive/html/qemu-devel/2018-08/msg06368.html >>>> >>>> Sorry about that, I was not on CC: for that series. I used the patches >>>> from v5 where Marc-André put me on CC:. >>>> >>>>> If you could work that into PULL v2, I'd be obliged. If not, I'll have >>>>> to address it in a follow-up patch. >>>> >>>> IMHO the naming is not that bad ... OTOH, I think Peter might already be >>>> away? ... so we've got plenty of time to sort this out anyway. >>>> Marc-André, could you send a new version of the patch? >>> >>> Tbh, I don't care so much about the naming of the test, but (for once) >>> I respectfully don't think Markus suggestion is better. >>> >>> The function checks "qom-set" without 'value' argument: >>> "qom-set-without-value", no brainer.. > > Nope, that's not what it tests. It tests the visitor, the marshalling > code generator, and the QMP core handle a certain kind of invalid > arguments correctly. It does not test qom-set. I explained all that > already. > >>> Naming it "invalid-arg" is so generic that I wouldn't be able what it does. > > I can accept "missing-any" or "missing-any-arg". I object to any name > involving qom-set, because the test is not about qom-set at all. > > If it was, then putting it in qmp-test.c would be wrong. > >> Ok, then let's keep it this way. As I said, IMHO the current naming is >> not really bad, and I also don't have any suggestions for a perfect name >> right now. > > We don't need a perfect name. We need one that's not actively > misleading. Ok, then let's cancel this PULL request. I'll send a new one after the "vacation freeze" (i.e. in three weeks), that should be enough time for both of you to come to an agreement about the best naming here. Thomas
Markus Armbruster <armbru@redhat.com> writes: > Thomas Huth <thuth@redhat.com> writes: > >> On 2018-08-31 15:24, Marc-André Lureau wrote: >>> Hi >>> On Fri, Aug 31, 2018 at 3:18 PM Thomas Huth <thuth@redhat.com> wrote: >>>> >>>> On 2018-08-31 14:04, Markus Armbruster wrote: >>>>> Thomas Huth <thuth@redhat.com> writes: >>>>> >>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com> >>>>>> >>>>>> test_qom_set_without_value() is about a bug in infrastructure used by >>>>>> the QMP core, fixed in commit c489780203. We covered the bug in >>>>>> infrastructure unit tests (commit bce3035a44). I wrote that test >>>>>> earlier, to cover QMP level as well, the test could go into qmp-test. >>>>>> >>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>>>>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>>>> --- >>>>>> tests/qmp-test.c | 14 ++++++++++++++ >>>>>> 1 file changed, 14 insertions(+) >>>>>> >>>>>> diff --git a/tests/qmp-test.c b/tests/qmp-test.c >>>>>> index b347228..2b923f0 100644 >>>>>> --- a/tests/qmp-test.c >>>>>> +++ b/tests/qmp-test.c >>>>>> @@ -321,6 +321,19 @@ static void test_qmp_preconfig(void) >>>>>> qtest_quit(qs); >>>>>> } >>>>>> >>>>>> +static void test_qom_set_without_value(void) >>>>>> +{ >>>>>> + QTestState *qts; >>>>>> + QDict *resp; >>>>>> + >>>>>> + qts = qtest_init(common_args); >>>>>> + resp = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':" >>>>>> + " { 'path': '/machine', 'property': 'rtc-time' } }"); >>>>>> + g_assert_nonnull(resp); >>>>>> + qmp_assert_error_class(resp, "GenericError"); >>>>>> + qtest_quit(qts); >>>>>> +} >>>>>> + >>>>>> int main(int argc, char *argv[]) >>>>>> { >>>>>> g_test_init(&argc, &argv, NULL); >>>>>> @@ -328,6 +341,7 @@ int main(int argc, char *argv[]) >>>>>> qtest_add_func("qmp/protocol", test_qmp_protocol); >>>>>> qtest_add_func("qmp/oob", test_qmp_oob); >>>>>> qtest_add_func("qmp/preconfig", test_qmp_preconfig); >>>>>> + qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value); >>>>>> >>>>>> return g_test_run(); >>>>>> } >>>>> >>>>> I'm afraid you missed my objection to naming: >>>>> Message-ID: <8736uvujxx.fsf@dusky.pond.sub.org> >>>>> https://lists.nongnu.org/archive/html/qemu-devel/2018-08/msg06368.html >>>> >>>> Sorry about that, I was not on CC: for that series. I used the patches >>>> from v5 where Marc-André put me on CC:. >>>> >>>>> If you could work that into PULL v2, I'd be obliged. If not, I'll have >>>>> to address it in a follow-up patch. >>>> >>>> IMHO the naming is not that bad ... OTOH, I think Peter might already be >>>> away? ... so we've got plenty of time to sort this out anyway. >>>> Marc-André, could you send a new version of the patch? >>> >>> Tbh, I don't care so much about the naming of the test, but (for once) >>> I respectfully don't think Markus suggestion is better. >>> >>> The function checks "qom-set" without 'value' argument: >>> "qom-set-without-value", no brainer.. > > Nope, that's not what it tests. It tests the visitor, the marshalling > code generator, and the QMP core handle a certain kind of invalid > arguments correctly. It does not test qom-set. I explained all that > already. > >>> Naming it "invalid-arg" is so generic that I wouldn't be able what it does. > > I can accept "missing-any" or "missing-any-arg". I object to any name > involving qom-set, because the test is not about qom-set at all. > > If it was, then putting it in qmp-test.c would be wrong. > >> Ok, then let's keep it this way. As I said, IMHO the current naming is >> not really bad, and I also don't have any suggestions for a perfect name >> right now. > > We don't need a perfect name. We need one that's not actively > misleading. Marc-André, would "qmp/missing-any-arg" and test_missing_any_arg() work for you?
Hi On Tue, Oct 9, 2018 at 8:41 PM Markus Armbruster <armbru@redhat.com> wrote: > > Markus Armbruster <armbru@redhat.com> writes: > > > Thomas Huth <thuth@redhat.com> writes: > > > >> On 2018-08-31 15:24, Marc-André Lureau wrote: > >>> Hi > >>> On Fri, Aug 31, 2018 at 3:18 PM Thomas Huth <thuth@redhat.com> wrote: > >>>> > >>>> On 2018-08-31 14:04, Markus Armbruster wrote: > >>>>> Thomas Huth <thuth@redhat.com> writes: > >>>>> > >>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com> > >>>>>> > >>>>>> test_qom_set_without_value() is about a bug in infrastructure used by > >>>>>> the QMP core, fixed in commit c489780203. We covered the bug in > >>>>>> infrastructure unit tests (commit bce3035a44). I wrote that test > >>>>>> earlier, to cover QMP level as well, the test could go into qmp-test. > >>>>>> > >>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > >>>>>> Reviewed-by: Thomas Huth <thuth@redhat.com> > >>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com> > >>>>>> --- > >>>>>> tests/qmp-test.c | 14 ++++++++++++++ > >>>>>> 1 file changed, 14 insertions(+) > >>>>>> > >>>>>> diff --git a/tests/qmp-test.c b/tests/qmp-test.c > >>>>>> index b347228..2b923f0 100644 > >>>>>> --- a/tests/qmp-test.c > >>>>>> +++ b/tests/qmp-test.c > >>>>>> @@ -321,6 +321,19 @@ static void test_qmp_preconfig(void) > >>>>>> qtest_quit(qs); > >>>>>> } > >>>>>> > >>>>>> +static void test_qom_set_without_value(void) > >>>>>> +{ > >>>>>> + QTestState *qts; > >>>>>> + QDict *resp; > >>>>>> + > >>>>>> + qts = qtest_init(common_args); > >>>>>> + resp = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':" > >>>>>> + " { 'path': '/machine', 'property': 'rtc-time' } }"); > >>>>>> + g_assert_nonnull(resp); > >>>>>> + qmp_assert_error_class(resp, "GenericError"); > >>>>>> + qtest_quit(qts); > >>>>>> +} > >>>>>> + > >>>>>> int main(int argc, char *argv[]) > >>>>>> { > >>>>>> g_test_init(&argc, &argv, NULL); > >>>>>> @@ -328,6 +341,7 @@ int main(int argc, char *argv[]) > >>>>>> qtest_add_func("qmp/protocol", test_qmp_protocol); > >>>>>> qtest_add_func("qmp/oob", test_qmp_oob); > >>>>>> qtest_add_func("qmp/preconfig", test_qmp_preconfig); > >>>>>> + qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value); > >>>>>> > >>>>>> return g_test_run(); > >>>>>> } > >>>>> > >>>>> I'm afraid you missed my objection to naming: > >>>>> Message-ID: <8736uvujxx.fsf@dusky.pond.sub.org> > >>>>> https://lists.nongnu.org/archive/html/qemu-devel/2018-08/msg06368.html > >>>> > >>>> Sorry about that, I was not on CC: for that series. I used the patches > >>>> from v5 where Marc-André put me on CC:. > >>>> > >>>>> If you could work that into PULL v2, I'd be obliged. If not, I'll have > >>>>> to address it in a follow-up patch. > >>>> > >>>> IMHO the naming is not that bad ... OTOH, I think Peter might already be > >>>> away? ... so we've got plenty of time to sort this out anyway. > >>>> Marc-André, could you send a new version of the patch? > >>> > >>> Tbh, I don't care so much about the naming of the test, but (for once) > >>> I respectfully don't think Markus suggestion is better. > >>> > >>> The function checks "qom-set" without 'value' argument: > >>> "qom-set-without-value", no brainer.. > > > > Nope, that's not what it tests. It tests the visitor, the marshalling > > code generator, and the QMP core handle a certain kind of invalid > > arguments correctly. It does not test qom-set. I explained all that > > already. > > > >>> Naming it "invalid-arg" is so generic that I wouldn't be able what it does. > > > > I can accept "missing-any" or "missing-any-arg". I object to any name > > involving qom-set, because the test is not about qom-set at all. > > > > If it was, then putting it in qmp-test.c would be wrong. > > > >> Ok, then let's keep it this way. As I said, IMHO the current naming is > >> not really bad, and I also don't have any suggestions for a perfect name > >> right now. > > > > We don't need a perfect name. We need one that's not actively > > misleading. > > Marc-André, would "qmp/missing-any-arg" and test_missing_any_arg() work > for you? Yes, do you want me to update the patch?
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Tue, Oct 9, 2018 at 8:41 PM Markus Armbruster <armbru@redhat.com> wrote: >> >> Markus Armbruster <armbru@redhat.com> writes: >> >> > Thomas Huth <thuth@redhat.com> writes: >> > >> >> On 2018-08-31 15:24, Marc-André Lureau wrote: [...] >> >>> Tbh, I don't care so much about the naming of the test, but (for once) >> >>> I respectfully don't think Markus suggestion is better. >> >>> >> >>> The function checks "qom-set" without 'value' argument: >> >>> "qom-set-without-value", no brainer.. >> > >> > Nope, that's not what it tests. It tests the visitor, the marshalling >> > code generator, and the QMP core handle a certain kind of invalid >> > arguments correctly. It does not test qom-set. I explained all that >> > already. >> > >> >>> Naming it "invalid-arg" is so generic that I wouldn't be able what it does. >> > >> > I can accept "missing-any" or "missing-any-arg". I object to any name >> > involving qom-set, because the test is not about qom-set at all. >> > >> > If it was, then putting it in qmp-test.c would be wrong. >> > >> >> Ok, then let's keep it this way. As I said, IMHO the current naming is >> >> not really bad, and I also don't have any suggestions for a perfect name >> >> right now. >> > >> > We don't need a perfect name. We need one that's not actively >> > misleading. >> >> Marc-André, would "qmp/missing-any-arg" and test_missing_any_arg() work >> for you? > > Yes, do you want me to update the patch? Yes, please.
diff --git a/tests/qmp-test.c b/tests/qmp-test.c index b347228..2b923f0 100644 --- a/tests/qmp-test.c +++ b/tests/qmp-test.c @@ -321,6 +321,19 @@ static void test_qmp_preconfig(void) qtest_quit(qs); } +static void test_qom_set_without_value(void) +{ + QTestState *qts; + QDict *resp; + + qts = qtest_init(common_args); + resp = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':" + " { 'path': '/machine', 'property': 'rtc-time' } }"); + g_assert_nonnull(resp); + qmp_assert_error_class(resp, "GenericError"); + qtest_quit(qts); +} + int main(int argc, char *argv[]) { g_test_init(&argc, &argv, NULL); @@ -328,6 +341,7 @@ int main(int argc, char *argv[]) qtest_add_func("qmp/protocol", test_qmp_protocol); qtest_add_func("qmp/oob", test_qmp_oob); qtest_add_func("qmp/preconfig", test_qmp_preconfig); + qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value); return g_test_run(); }