Hi
On Tue, Oct 9, 2018 at 8:41 PM Markus Armbruster <arm...@redhat.com> wrote:
>
> Markus Armbruster <arm...@redhat.com> writes:
>
> > Thomas Huth <th...@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 <th...@redhat.com> wrote:
> >>>>
> >>>> On 2018-08-31 14:04, Markus Armbruster wrote:
> >>>>> Thomas Huth <th...@redhat.com> writes:
> >>>>>
> >>>>>> From: Marc-André Lureau <marcandre.lur...@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.lur...@redhat.com>
> >>>>>> Reviewed-by: Thomas Huth <th...@redhat.com>
> >>>>>> Signed-off-by: Thomas Huth <th...@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....@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
