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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3422 bytes --]

On 2017-07-03 16:15, Eric Blake wrote:
> 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 <mreitz@redhat.com>
>> ---
>>  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.

True.

>> +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.

I don't mind using the sentinel here, too, but passing NULL doesn't make
much sense here. It does for test_equality(), though.

>> +
>> +static void qobject_is_equal_null_test(void)
>> +{
>> +    test_equality(false, qnull(), NULL);
>> +}
> 
> Where do you test_equality(true, NULL, NULL) ?

Automatically in do_test_equality().

(It automatically tests whether all arguments are equal to itself --
which is why I can't use it to test NaN.)

>> +
>> +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.

Sure, why not.

>> +
>> +    /* 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")

I pronounce it en-a-en, but I don't know whether that's the usual way. ;-)

> 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 <eblake@redhat.com>

Thanks!

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]