Luiz Capitulino <lcapitul...@redhat.com> writes:
> On Fri, 12 May 2017 08:30:36 +0200
> Markus Armbruster <arm...@redhat.com> wrote:
>
>> Question for Luiz...
>>
>> Marc-André Lureau <marcandre.lur...@redhat.com> 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 <lcapitul...@redhat.com>
>> > + *
>> > + * 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?
