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

[John Snow <jsnow@redhat.com>]

On 03/24/2015 07:20 PM, Andreas Färber wrote:
> 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 <afaerber@suse.de>
>>> ---
>>>    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? :)
>

Not enough of one to bother delaying this for 2.3 -- I did forget this 
was a macro.

Reviewed-by: John Snow <jsnow@redhat.com>

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

I can get behind "change as little as possible."

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

I can always fire up valgrind and figure out something to point a finger 
at :)

> Thanks a lot for your review.
>
> Regards,
> Andreas
>