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>
---
 tests/qmp-test.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 4ae2245484..fdfe73b6d2 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -348,6 +348,23 @@ static void test_qmp_preconfig(void)
     qtest_quit(qs);
 }
 
+static void test_qom_set_without_value(void)
+{
+    QTestState *qts;
+    QDict *ret;
+
+    qts = qtest_init(common_args);
+
+    ret = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':"
+                    " { 'path': '/machine', 'property': 'rtc-time' } }");
+    g_assert_nonnull(ret);
+
+    g_assert_cmpstr(get_error_class(ret), ==, "GenericError");
+
+    qobject_unref(ret);
+    qtest_quit(qts);
+}
+
 int main(int argc, char *argv[])
 {
     g_test_init(&argc, &argv, NULL);
@@ -355,6 +372,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();
 }
-- 
2.19.0.rc0.48.gb9dfa238d5

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> 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>
> ---
>  tests/qmp-test.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
> index 4ae2245484..fdfe73b6d2 100644
> --- a/tests/qmp-test.c
> +++ b/tests/qmp-test.c
> @@ -348,6 +348,23 @@ static void test_qmp_preconfig(void)
>      qtest_quit(qs);
>  }
>  
> +static void test_qom_set_without_value(void)
> +{
> +    QTestState *qts;
> +    QDict *ret;
> +
> +    qts = qtest_init(common_args);
> +
> +    ret = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':"
> +                    " { 'path': '/machine', 'property': 'rtc-time' } }");
> +    g_assert_nonnull(ret);
> +
> +    g_assert_cmpstr(get_error_class(ret), ==, "GenericError");
> +
> +    qobject_unref(ret);
> +    qtest_quit(qts);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>      g_test_init(&argc, &argv, NULL);
> @@ -355,6 +372,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();
>  }

What we're testing here is a missing 'any' parameter.  The test should
be named accordingly.  Perhaps:

       qtest_add_func("qmp/missing-any", test_missing_any);

I can't bring myself to care for this test.  There are countless ways to
run a command with invalid parameters, and this is just one.  The only
thing that sets it apart is it used to tickle a visitor bug we've long
fixed, complete with a unit test to prevent regressions.  But the cost
of carrying the test is pretty low, so if you really want it, then
taking it without further argument is probably cheaper even if it's as
redundant as I suspect it is.

Hi

On Thu, Aug 30, 2018 at 3:05 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
>> 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>
>> ---
>>  tests/qmp-test.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
>> index 4ae2245484..fdfe73b6d2 100644
>> --- a/tests/qmp-test.c
>> +++ b/tests/qmp-test.c
>> @@ -348,6 +348,23 @@ static void test_qmp_preconfig(void)
>>      qtest_quit(qs);
>>  }
>>
>> +static void test_qom_set_without_value(void)
>> +{
>> +    QTestState *qts;
>> +    QDict *ret;
>> +
>> +    qts = qtest_init(common_args);
>> +
>> +    ret = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':"
>> +                    " { 'path': '/machine', 'property': 'rtc-time' } }");
>> +    g_assert_nonnull(ret);
>> +
>> +    g_assert_cmpstr(get_error_class(ret), ==, "GenericError");
>> +
>> +    qobject_unref(ret);
>> +    qtest_quit(qts);
>> +}
>> +
>>  int main(int argc, char *argv[])
>>  {
>>      g_test_init(&argc, &argv, NULL);
>> @@ -355,6 +372,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();
>>  }
>
> What we're testing here is a missing 'any' parameter.  The test should
> be named accordingly.  Perhaps:
>
>        qtest_add_func("qmp/missing-any", test_missing_any);
>
> I can't bring myself to care for this test.  There are countless ways to
> run a command with invalid parameters, and this is just one.  The only
> thing that sets it apart is it used to tickle a visitor bug we've long
> fixed, complete with a unit test to prevent regressions.  But the cost
> of carrying the test is pretty low, so if you really want it, then
> taking it without further argument is probably cheaper even if it's as
> redundant as I suspect it is.

Yes, I would rather have it than drop it. It covers the QMP level as well.

Hi

On Thu, Aug 30, 2018 at 3:05 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > 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>
> > ---
> >  tests/qmp-test.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/tests/qmp-test.c b/tests/qmp-test.c
> > index 4ae2245484..fdfe73b6d2 100644
> > --- a/tests/qmp-test.c
> > +++ b/tests/qmp-test.c
> > @@ -348,6 +348,23 @@ static void test_qmp_preconfig(void)
> >      qtest_quit(qs);
> >  }
> >
> > +static void test_qom_set_without_value(void)
> > +{
> > +    QTestState *qts;
> > +    QDict *ret;
> > +
> > +    qts = qtest_init(common_args);
> > +
> > +    ret = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':"
> > +                    " { 'path': '/machine', 'property': 'rtc-time' } }");
> > +    g_assert_nonnull(ret);
> > +
> > +    g_assert_cmpstr(get_error_class(ret), ==, "GenericError");
> > +
> > +    qobject_unref(ret);
> > +    qtest_quit(qts);
> > +}
> > +
> >  int main(int argc, char *argv[])
> >  {
> >      g_test_init(&argc, &argv, NULL);
> > @@ -355,6 +372,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();
> >  }
>
> What we're testing here is a missing 'any' parameter.  The test should
> be named accordingly.  Perhaps:
>
>        qtest_add_func("qmp/missing-any", test_missing_any);

Eh, the inner visitor fix was about an 'any' parameter missing.

But the test is more about about checking the behaviour of qom-set
without 'value' argument. I would not rename it based on the internal
bug that was fixed.

-- 
Marc-André Lureau

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Thu, Aug 30, 2018 at 3:05 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>> > 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>
>> > ---
>> >  tests/qmp-test.c | 18 ++++++++++++++++++
>> >  1 file changed, 18 insertions(+)
>> >
>> > diff --git a/tests/qmp-test.c b/tests/qmp-test.c
>> > index 4ae2245484..fdfe73b6d2 100644
>> > --- a/tests/qmp-test.c
>> > +++ b/tests/qmp-test.c
>> > @@ -348,6 +348,23 @@ static void test_qmp_preconfig(void)
>> >      qtest_quit(qs);
>> >  }
>> >
>> > +static void test_qom_set_without_value(void)
>> > +{
>> > +    QTestState *qts;
>> > +    QDict *ret;
>> > +
>> > +    qts = qtest_init(common_args);
>> > +
>> > +    ret = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':"
>> > +                    " { 'path': '/machine', 'property': 'rtc-time' } }");
>> > +    g_assert_nonnull(ret);
>> > +
>> > +    g_assert_cmpstr(get_error_class(ret), ==, "GenericError");
>> > +
>> > +    qobject_unref(ret);
>> > +    qtest_quit(qts);
>> > +}
>> > +
>> >  int main(int argc, char *argv[])
>> >  {
>> >      g_test_init(&argc, &argv, NULL);
>> > @@ -355,6 +372,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();
>> >  }
>>
>> What we're testing here is a missing 'any' parameter.  The test should
>> be named accordingly.  Perhaps:
>>
>>        qtest_add_func("qmp/missing-any", test_missing_any);
>
> Eh, the inner visitor fix was about an 'any' parameter missing.
>
> But the test is more about about checking the behaviour of qom-set
> without 'value' argument. I would not rename it based on the internal
> bug that was fixed.

Missing arguments are caught by the QObject input visitor on behalf of
the generated command marshalling code.  The command handler isn't
called then.  Thus, qom-set doesn't get to behave!  All the test really
tests is that

* the visitor behaves (redundant with test-qobject-input-visitor.c's
  test_visitor_in_fail_struct_missing()),

* the generated marshalling code handles visitor failure (I guess that's
  worth covering here),

* and the QMP core handles command failure (redundant with numerous
  other tests, but covering it here once more won't hurt).

The test could just as well any other command with a mandatory argument
of type 'any', such as qom-set or object-add.

Same for arguments of inadmissible JSON type.

Having written all this, I now think the test should be named
"qmp/invalid-arg".