|
From: | John Snow |
Subject: | Re: [Qemu-devel] [PATCH for-2.3 2/2] i440fx-test: Fix test paths to include architecture |
Date: | Tue, 24 Mar 2015 19:28:32 -0400 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 |
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
[Prev in Thread] | Current Thread | [Next in Thread] |