[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 04/27] qapi: Simplify error testing in test-q
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v9 04/27] qapi: Simplify error testing in test-qmp-* |
Date: |
Wed, 4 Nov 2015 14:05:44 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
On 11/04/2015 01:40 AM, Markus Armbruster wrote:
>
>> By moving err into data, we can let test teardown take care
>> of cleaning up any collected error; it also gives us fewer
>> lines of code between repeated tests where init runs teardown
>> on our behalf.
>
> This part isn't as obvious.
>
> Having two parts of differing obviousness indicates patch splitting
> could make sense. Especially when the parts are large and mechanical,
> because reviewing large mechanical changes is much easier when there's
> just one kind of it.
Will split.
>> @@ -364,21 +341,17 @@ static void
>> test_visitor_in_alternate_number(TestInputVisitorData *data,
>> /* Parsing an int */
>>
>> v = visitor_input_test_init(data, "42");
>> - visit_type_AltStrBool(v, &asb, NULL, &err);
>> - g_assert(err);
>> - error_free(err);
>> - err = NULL;
>> + visit_type_AltStrBool(v, &asb, NULL, &data->err);
>> + g_assert(data->err);
>> qapi_free_AltStrBool(asb);
>
> This leaves data->err non-null.
>
>>
>> /* FIXME: Order of alternate should not affect semantics; asn should
>> * parse the same as ans */
>> v = visitor_input_test_init(data, "42");
And this part wipes out data, so that data->err is once again NULL.
>> - visit_type_AltStrNum(v, &asn, NULL, &err);
>> + visit_type_AltStrNum(v, &asn, NULL, &data->err);
>
> If visit_type_AltStrNum() errors out, its error will be thrown away by
> its error_propagate(), because data->err is already non-null.
No, you missed that this patch is relying on the magic in 3/27 that
wipes out data on every new test.
>
> If it fails to error out, its error_propagate() does nothing, and we
> won't detect the test failure.
>
> Your patch makes forgetting to reset err an even easier mistake to make,
> because it removes most of the error_free() that serve as reminders.
>
> Is it a good idea anyway? Let's discuss before I check the remainder of
> this patch.
The point was that by moving err _into_ data, then the resetting of err
becomes as simple as resetting data, rather than having to be verbose at
every use of data->err. We just need a visitor_input_test_init() in
between.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v9 25/27] qapi: Add positive tests to qapi-schema-test, (continued)
[Qemu-devel] [PATCH v9 04/27] qapi: Simplify error testing in test-qmp-*, Eric Blake, 2015/11/04
[Qemu-devel] [PATCH v9 21/27] qapi: Factor out QAPISchemaObjectType.check_clash(), Eric Blake, 2015/11/04
- [Qemu-devel] [PATCH RFC 0/5] qapi: Use common name mangling for enumeration constants, Markus Armbruster, 2015/11/05
- [Qemu-devel] [PATCH RFC 1/5] qapi: Generate a sed script to help eliminate camel_to_upper(), Markus Armbruster, 2015/11/05
- [Qemu-devel] [PATCH RFC 2/5] Revert "qapi: Generate a sed script to help eliminate camel_to_upper()", Markus Armbruster, 2015/11/05
- [Qemu-devel] [PATCH RFC 4/5] crypto: Drop name mangling override, Markus Armbruster, 2015/11/05
- [Qemu-devel] [PATCH RFC 5/5] Revert "qapi: allow override of default enum prefix naming", Markus Armbruster, 2015/11/05
- [Qemu-devel] [PATCH RFC 3/5] qapi: Use common name mangling for enumeration constants, Markus Armbruster, 2015/11/05
- Re: [Qemu-devel] [PATCH RFC 3/5] qapi: Use common name mangling for enumeration constants, Daniel P. Berrange, 2015/11/05