| Message ID | 1427237148-8728-3-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
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? > 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? --js
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? :) >> 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
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 >
On 25/03/2015 00:20, Andreas Färber wrote: >>> >> - 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? :) > You would have to wrap g_test_add_vtable with qtest_add_vtable, and then add a macro that mimicks g_test_add: /* hook up a test with fixture under test path */ #define qtest_add(testpath, Fixture, tdata, fsetup, ftest, fteardown) \ G_STMT_START { \ void (*add_vtable) (const char*, \ gsize, \ gconstpointer, \ void (*) (Fixture*, gconstpointer), \ void (*) (Fixture*, gconstpointer), \ void (*) (Fixture*, gconstpointer)) = (void (*) (const gchar *, gsize, gconstpointer, void ( *) (Fixture*, gconstpointer), void (*) (Fixture*, gconstpointer), void (*) (Fixture*, gconstpointer))) qtest_add_vtable; \ add_vtable \ (testpath, sizeof (Fixture), tdata, fsetup, ftest, fteardown); \ } G_STMT_END Paolo
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); } 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);
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(-)