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 <ler...@redhat.com>
> [...]
>> 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
