Re: [Qemu-devel] [PATCH for-2.3 2/2] i440fx-test: Fix test paths to include architecture

[Andreas Färber]

Am 25.03.2015 um 00:09 schrieb John Snow:
> On 03/24/2015 06:45 PM, Andreas Färber wrote:
>> Replace g_test_add_func() with new qtest_add_func() and modify the path
>> passed to g_test_add() macro.
>>
>> Signed-off-by: Andreas Färber <address@hidden>
>> ---
>>   tests/i440fx-test.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c
>> index a3f7279..bc3f54c 100644
>> --- a/tests/i440fx-test.c
>> +++ b/tests/i440fx-test.c
>> @@ -383,8 +383,10 @@ static void add_firmware_test(const char *testpath,
>>                                 void
>> (*setup_fixture)(FirmwareTestFixture *f,
>>                                                       gconstpointer
>> test_data))
>>   {
>> -    g_test_add(testpath, FirmwareTestFixture, NULL, setup_fixture,
>> +    char *path = g_strdup_printf("/%s%s", qtest_get_arch(), testpath);
>> +    g_test_add(path, FirmwareTestFixture, NULL, setup_fixture,
>>                  test_i440fx_firmware, NULL);
>> +    g_free(path);
>>   }
>>
> 
> Is it not worth adding an even more generic wrapper to prevent future
> desynch from our preferred path format?

As mentioned in the commit message, g_test_add() is a macro, not a
function, so seemed more complicated to wrap. Can you post a patch if
you have an idea? :)

>>   static void request_bios(FirmwareTestFixture *fixture,
>> @@ -408,8 +410,8 @@ int main(int argc, char **argv)
>>
>>       data.num_cpus = 1;
>>
>> -    g_test_add_data_func("/i440fx/defaults", &data,
>> test_i440fx_defaults);
>> -    g_test_add_data_func("/i440fx/pam", &data, test_i440fx_pam);
>> +    qtest_add_data_func("i440fx/defaults", &data, test_i440fx_defaults);
>> +    qtest_add_data_func("i440fx/pam", &data, test_i440fx_pam);
>>       add_firmware_test("/i440fx/firmware/bios", request_bios);
>>       add_firmware_test("/i440fx/firmware/pflash", request_pflash);
>>
>>
> 
> Similar to above, is it worth replacing other test's usage of
> g_test_add_data_func to force this standard path format everywhere?

I reviewed the test output from -rc0 while going through test failures.
fw_cfg-test and i440fx-test were the only nits I found. I wondered
whether we might poison the g_test_* versions or convert the other
tests, but did not want to unnecessarily risk this for 2.3.

Another issue that I've come across is that several tests misuse qmp(),
ignoring the return value instead of using qmp_discard_response().

Also I had the impression that my qom-test has noticeably degraded in
performance, and I wondered whether Paolo's changes to qmp(), parsing
the string argument into a QObject, might be to blame, given that a lot
of QMP qom-gets are performed by my test, and the number of objects and
properties tested keeps increasing (MemoryRegion, qemu_irq, machines).

Thanks a lot for your review.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)