From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40186) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VmQ6r-00055d-NM for qemu-devel@nongnu.org; Fri, 29 Nov 2013 10:36:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VmQ6l-0005B3-KZ for qemu-devel@nongnu.org; Fri, 29 Nov 2013 10:36:01 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51871) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VmQ6l-0005Ar-2s for qemu-devel@nongnu.org; Fri, 29 Nov 2013 10:35:55 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rATFZrHE028151 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 29 Nov 2013 10:35:54 -0500 Message-ID: <5298B458.4050203@redhat.com> Date: Fri, 29 Nov 2013 16:35:52 +0100 From: Laszlo Ersek MIME-Version: 1.0 References: <1385662155-15212-1-git-send-email-lersek@redhat.com> <1385662155-15212-3-git-send-email-lersek@redhat.com> <20131129145346.GA8313@otherpad.lan.raisama.net> In-Reply-To: <20131129145346.GA8313@otherpad.lan.raisama.net> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 2/4] i440fx-test: give each GTest case its own qtest List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: qemu-devel@nongnu.org On 11/29/13 15:53, Eduardo Habkost wrote: > On Thu, Nov 28, 2013 at 07:09:13PM +0100, Laszlo Ersek wrote: >> The current two GTest cases, /i440fx/defaults and /i440fx/pam can share a >> qemu process, but the next two cases will need dedicated instances. It is >> messy (and order-dependent) to dynamically configure GTest cases one by >> one to start, stop, or keep the current qtest (*); let's just have each >> GTest work with its own qtest. The performance difference should be >> negligible. >> >> (*) As g_test_run() can be invoked at most once per process startup, and >> it runs GTest cases in sequence, we'd need clumsy data structures to >> control each GTest case to start/stop/keep the qemu instance. Or, we'd >> have to code the same information into the test methods themselves, which >> would make them even more order-dependent. >> >> Signed-off-by: Laszlo Ersek > [...] >> static void test_i440fx_defaults(gconstpointer opaque) >> { >> const TestData *s = opaque; >> + QPCIBus *bus; >> QPCIDevice *dev; >> uint32_t value; >> >> - dev = qpci_device_find(s->bus, QPCI_DEVFN(0, 0)); >> + bus = test_start_get_bus(s); >> + dev = qpci_device_find(bus, QPCI_DEVFN(0, 0)); > > You can use GTest setup/teardown functions to do this automatically on > all tests, but I am not sure the code really looks better when using it. > I gave it a try and it looks like this: > > diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c > index 6ac46bf..c9d6f6a 100644 > --- a/tests/i440fx-test.c > +++ b/tests/i440fx-test.c > @@ -25,15 +25,35 @@ > > #define ARRAY_SIZE(array) (sizeof(array) / sizeof((array)[0])) > > -typedef struct TestData > +typedef struct TestArgs > { > int num_cpus; > +} TestArgs; > + > +typedef struct TestData > +{ > QPCIBus *bus; > } TestData; > > -static void test_i440fx_defaults(gconstpointer opaque) > +static void test_setup(TestData *s, gconstpointer user_data) > +{ > + const TestArgs *args = user_data; > + char *cmdline; > + > + cmdline = g_strdup_printf("-smp %d", args->num_cpus); > + qtest_start(cmdline); > + g_free(cmdline); > + s->bus = qpci_init_pc(); > +} > + > +static void test_teardown(TestData *s, gconstpointer user_data) > +{ > + qtest_end(); > +} > + > +static void test_i440fx_defaults(TestData *s, gconstpointer user_data) > { > - const TestData *s = opaque; > + const TestArgs *args = user_data; > QPCIDevice *dev; > uint32_t value; > > @@ -62,7 +82,7 @@ static void test_i440fx_defaults(gconstpointer opaque) > > /* 3.2.11 */ > value = qpci_config_readw(dev, 0x50); /* PMCCFG */ > - if (s->num_cpus == 1) { /* WPE */ > + if (args->num_cpus == 1) { /* WPE */ > g_assert(!(value & (1 << 15))); > } else { > g_assert((value & (1 << 15))); > @@ -176,9 +196,8 @@ static void write_area(uint32_t start, uint32_t end, uint8_t value) > g_free(data); > } > > -static void test_i440fx_pam(gconstpointer opaque) > +static void test_i440fx_pam(TestData *s, gconstpointer user_data) > { > - const TestData *s = opaque; > QPCIDevice *dev; > int i; > static struct { > @@ -258,26 +277,16 @@ static void test_i440fx_pam(gconstpointer opaque) > > int main(int argc, char **argv) > { > - TestData data; > - char *cmdline; > int ret; > + TestArgs args = { .num_cpus = 1 }; > > g_test_init(&argc, &argv, NULL); > > - data.num_cpus = 1; > - > - cmdline = g_strdup_printf("-smp %d", data.num_cpus); > - qtest_start(cmdline); > - g_free(cmdline); > - > - data.bus = qpci_init_pc(); > - > - g_test_add_data_func("/i440fx/defaults", &data, test_i440fx_defaults); > - g_test_add_data_func("/i440fx/pam", &data, test_i440fx_pam); > + g_test_add("/i440fx/defaults", TestData, &args, test_setup, > + test_i440fx_defaults, test_teardown); > + g_test_add("/i440fx/pam", TestData, &args, test_setup, > + test_i440fx_pam, test_teardown); > > ret = g_test_run(); > - > - qtest_end(); > - > return ret; > } > I agree that this specific use of the fixtures doesn't really pay off. Thank you Laszlo