Re: [Qemu-devel] [PATCH v2 2/4] i440fx-test: give each GTest case its own qtest

[Laszlo Ersek <lersek@redhat.com>]

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