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

[John Snow]

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 <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? :)

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

Reviewed-by: John Snow <address@hidden>

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