Re: [Qemu-devel] [PATCH 04/17] qapi: merge QInt and QFloat in QNum

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Fri, 12 May 2017 08:30:36 +0200
> Markus Armbruster <address@hidden> wrote:
>
>> Question for Luiz...
>> 
>> Marc-André Lureau <address@hidden> writes:
>> 
>> [...]
>> > diff --git a/tests/check-qnum.c b/tests/check-qnum.c
>> > new file mode 100644
>> > index 0000000000..d08d35e85a
>> > --- /dev/null
>> > +++ b/tests/check-qnum.c
>> > @@ -0,0 +1,131 @@
>> > +/*
>> > + * QNum unit-tests.
>> > + *
>> > + * Copyright (C) 2009 Red Hat Inc.
>> > + *
>> > + * Authors:
>> > + *  Luiz Capitulino <address@hidden>
>> > + *
>> > + * This work is licensed under the terms of the GNU LGPL, version 2.1 or 
>> > later.
>> > + * See the COPYING.LIB file in the top-level directory.
>> > + */
>> > +#include "qemu/osdep.h"
>> > +
>> > +#include "qapi/qmp/qnum.h"
>> > +#include "qapi/error.h"
>> > +#include "qemu-common.h"
>> > +
>> > +/*
>> > + * Public Interface test-cases
>> > + *
>> > + * (with some violations to access 'private' data)
>> > + */
>> > +
>> > +static void qnum_from_int_test(void)
>> > +{
>> > +    QNum *qi;
>> > +    const int value = -42;
>> > +
>> > +    qi = qnum_from_int(value);
>> > +    g_assert(qi != NULL);
>> > +    g_assert_cmpint(qi->u.i64, ==, value);
>> > +    g_assert_cmpint(qi->base.refcnt, ==, 1);
>> > +    g_assert_cmpint(qobject_type(QOBJECT(qi)), ==, QTYPE_QNUM);
>> > +
>> > +    // destroy doesn't exit yet
>> > +    g_free(qi);
>> > +}  
>> 
>> The comment is enigmatic. 
>
> It was meant for future generations to figure it out :)

Hah!

>> It was first written in commit 33837ba
>> "Introduce QInt unit-tests", and got copied around since.  In
>> check-qlist.c, it's spelled "exist yet".
>
> Yes, "exit" is a typo it should be "exist".
>
>> What is "destroy", why doesn't it exit / exist now, but will exit /
>> exist later?  It can't be qnum_destroy_obj(), because that certainly
>> exists already, exits already in the sense of returning, and shouldn't
>> ever exit in the sense of terminating the program.
>> 
>> The comment applies to a g_free().  Why do we free directly instead
>> decrementing the reference count?  Perhaps the comment tries to explain
>> that (if it does, it fails).
>
> In my personal style of writing unit-tests, I never use a method
> in a test before testing it. So, as QDECREF() wasn't tested yet,
> I wasn't allowed to use it.

It's a good principle for organizing tests.

> While I keep this principle when writing unit-tests today, this
> particular case is very extreme and not useful at all. Today I'd just
> go ahead and use QDECREF().

Makes sense.

>                             The qint_destroy_test() in the original
> commit is also very bogus, it's not really doing an useful test.

It can demonstrate leaks under valgrind.  But pretty much every other
test can just as well, so...

Marc-André, care to stick a cleanup patch into your series?