Re: [Qemu-devel] [PATCH v3 5/5] tests: Add check-qobject for equality tests

[Eric Blake]

On 07/03/2017 07:25 AM, Max Reitz wrote:
> Add a new test file (check-qobject.c) for unit tests that concern
> QObjects as a whole.
> 
> Its only purpose for now is to test the qobject_is_equal() function.
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  tests/Makefile.include |   4 +-
>  tests/check-qobject.c  | 312 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 315 insertions(+), 1 deletion(-)
>  create mode 100644 tests/check-qobject.c
> 

> + * Note that qobject_is_equal() is not really an equivalence relation,
> + * so this function may not be used for all objects (reflexivity is
> + * not guaranteed).

May be worth expanding the comment to mention NaN in QNum as the culprit
for this fact.


> +static void do_test_equality(bool expected, ...)
> +{
> +    va_list ap_count, ap_extract;
> +    QObject **args;
> +    int arg_count = 0;
> +    int i, j;
> +
> +    va_start(ap_count, expected);
> +    va_copy(ap_extract, ap_count);
> +    while (va_arg(ap_count, QObject *) != &_test_equality_end_of_arguments) {

Here, you're careful to allow a NULL argument,


> +static void do_free_all(int _, ...)
> +{
> +    va_list ap;
> +    QObject *obj;
> +
> +    va_start(ap, _);
> +    while ((obj = va_arg(ap, QObject *)) != NULL) {
> +        qobject_decref(obj);
> +    }

Here, you stop on the first NULL, and aren't checking for the special
sentinel that can't be freed.

> +    va_end(ap);
> +}
> +
> +#define free_all(...) \
> +    do_free_all(0, __VA_ARGS__, NULL)

Then again, you don't pass the special sentinel. So as long as NULL is
the last argument(s) on any test that passes NULL (rather than an
intermediate argument), you don't need to use the sentinel, and stopping
iteration on the first NULL is okay.  A bit magic, but I can live with it.

> +
> +static void qobject_is_equal_null_test(void)
> +{
> +    test_equality(false, qnull(), NULL);
> +}

Where do you test_equality(true, NULL, NULL) ?

> +
> +static void qobject_is_equal_num_test(void)
> +{
> +    QNum *u0, *i0, *d0, *d0p25, *dnan, *um42, *im42, *dm42;
> +    QString *s0, *s_empty;
> +    QBool *bfalse;
> +
> +    u0 = qnum_from_uint(0u);
> +    i0 = qnum_from_int(0);
> +    d0 = qnum_from_double(0.0);
> +    d0p25 = qnum_from_double(0.25);
> +    dnan = qnum_from_double(0.0 / 0.0);

Ah, so you ARE testing NaN as a QNum, even though it can't occur in
JSON.  Might be worth a comment.


> +
> +    /* Containing an NaN value will make this dict compare unequal to

s/an/a/ (if you pronounce it "nan" or "not-a-number") (but I guess no
change if you pronounce it "en-a-en")

There might be some improvements to add, but as written the test is
reasonable, and some testing is better than none, so:
Reviewed-by: Eric Blake <address@hidden>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature