From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57672) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fwgkX-0006FG-IU for qemu-devel@nongnu.org; Mon, 03 Sep 2018 00:46:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fwgkU-0008Qx-9q for qemu-devel@nongnu.org; Mon, 03 Sep 2018 00:46:05 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:33592 helo=mx1.redhat.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fwgkU-0008QP-3O for qemu-devel@nongnu.org; Mon, 03 Sep 2018 00:46:02 -0400 References: <1535704738-8986-1-git-send-email-thuth@redhat.com> <1535704738-8986-9-git-send-email-thuth@redhat.com> <87efeerbcu.fsf@dusky.pond.sub.org> <593af39a-cc9b-d4e8-7e6c-efdbbe169096@redhat.com> <5b4cef8c-084c-5a2b-1fb6-210388054d38@redhat.com> <87va7qli3f.fsf@dusky.pond.sub.org> From: Thomas Huth Message-ID: <8b3a0b15-d7dd-c95d-bccd-062c9e073658@redhat.com> Date: Mon, 3 Sep 2018 06:45:59 +0200 MIME-Version: 1.0 In-Reply-To: <87va7qli3f.fsf@dusky.pond.sub.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PULL 8/9] tests: add qmp/qom-set-without-value test List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= , Laurent Vivier , Peter Maydell , Paolo Bonzini , QEMU On 2018-08-31 16:35, Markus Armbruster wrote: > Thomas Huth writes: >=20 >> On 2018-08-31 15:24, Marc-Andr=C3=A9 Lureau wrote: >>> Hi >>> On Fri, Aug 31, 2018 at 3:18 PM Thomas Huth wrote: >>>> >>>> On 2018-08-31 14:04, Markus Armbruster wrote: >>>>> Thomas Huth writes: >>>>> >>>>>> From: Marc-Andr=C3=A9 Lureau >>>>>> >>>>>> 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-te= st. >>>>>> >>>>>> Signed-off-by: Marc-Andr=C3=A9 Lureau >>>>>> Reviewed-by: Thomas Huth >>>>>> Signed-off-by: Thomas Huth >>>>>> --- >>>>>> 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 =3D qtest_init(common_args); >>>>>> + resp =3D qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':" >>>>>> + " { 'path': '/machine', 'property': 'rtc-tim= e' } }"); >>>>>> + 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_with= out_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.h= tml >>>> >>>> Sorry about that, I was not on CC: for that series. I used the patch= es >>>> from v5 where Marc-Andr=C3=A9 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 alread= y be >>>> away? ... so we've got plenty of time to sort this out anyway. >>>> Marc-Andr=C3=A9, 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.. >=20 > 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. >=20 >>> Naming it "invalid-arg" is so generic that I wouldn't be able what it= does. >=20 > 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. >=20 > If it was, then putting it in qmp-test.c would be wrong. >=20 >> 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 na= me >> right now. >=20 > 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