Re: [Qemu-devel] [PULL 03/12] test-qdev-global-props: Run tests on subprocess

[Michael Roth]

Quoting Eduardo Habkost (2014-09-18 12:27:12)
> On Thu, Sep 18, 2014 at 11:29:47AM -0500, Michael Roth wrote:
> > Quoting Michael S. Tsirkin (2014-09-14 13:41:23)
> > > From: Eduardo Habkost <address@hidden>
> > > 
> > > There are multiple reasons for running the global property tests on a
> > > subprocess:
> > > 
> > > * We need the global_props lists to be empty for each test case, so
> > >   global properties from the previous test won't affect the next one;
> > > * We don't want the qdev_prop_check_global() warnings to pollute test
> > >   output;
> > > * With a subprocess, we can ensure qdev_prop_check_global() is printing
> > >   the warning messages it should.
> > > 
> > > Signed-off-by: Eduardo Habkost <address@hidden>
> > > Acked-by: Michael S. Tsirkin <address@hidden>
> > > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > > ---
> > >  tests/test-qdev-global-props.c | 49 
> > > ++++++++++++++++++++++++++++++++++++------
> > >  1 file changed, 43 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/tests/test-qdev-global-props.c 
> > > b/tests/test-qdev-global-props.c
> > > index e1a1317..34223a7 100644
> > > --- a/tests/test-qdev-global-props.c
> > > +++ b/tests/test-qdev-global-props.c
> > > @@ -65,7 +65,7 @@ static const TypeInfo static_prop_type = {
> > >  };
> > > 
> > >  /* Test simple static property setting to default value */
> > > -static void test_static_prop(void)
> > > +static void test_static_prop_subprocess(void)
> > >  {
> > >      MyType *mt;
> > > 
> > > @@ -75,8 +75,16 @@ static void test_static_prop(void)
> > >      g_assert_cmpuint(mt->prop1, ==, PROP_DEFAULT);
> > >  }
> > > 
> > > +static void test_static_prop(void)
> > > +{
> > > +    g_test_trap_subprocess("/qdev/properties/static/default/subprocess", 
> > > 0, 0);
> > > +    g_test_trap_assert_passed();
> > > +    g_test_trap_assert_stderr("");
> > > +    g_test_trap_assert_stdout("");
> > > +}
> > 
> > <snip>
> > 
> > >  int main(int argc, char **argv)
> > >  {
> > >      g_test_init(&argc, &argv, NULL);
> > > @@ -174,9 +200,20 @@ int main(int argc, char **argv)
> > >      type_register_static(&static_prop_type);
> > >      type_register_static(&dynamic_prop_type);
> > > 
> > > -    g_test_add_func("/qdev/properties/static/default", test_static_prop);
> > > -    g_test_add_func("/qdev/properties/static/global", 
> > > test_static_globalprop);
> > > -    g_test_add_func("/qdev/properties/dynamic/global", 
> > > test_dynamic_globalprop);
> > > +    g_test_add_func("/qdev/properties/static/default/subprocess",
> > > +                    test_static_prop_subprocess);
> > > +    g_test_add_func("/qdev/properties/static/default",
> > > +                    test_static_prop);
> > 
> > Since in the code above test_static_prop is actually the test that re-runs
> > /qdev/properties/static/default/subprocess under g_test_trap_subprocess, 
> > aren't
> > the tests (or test function implementations) backwards?
> 
> Your description is correct: ./static/default is test_static_prop(),
> which runs ./static/default/subprocess (test_static_prop_subprocess())
> in a subprocess. But I don't understand what's backwards. Are you
> talking about the function names, test path strings, or about the
> ordering of the g_test_add_func() calls?

I was under the false impression that this was adding a "subprocess" variant of
each existing test, where the subprocess variants were supposed to have 
"subprocess"
after them in the test paths to differentiate them, and the original cases would
retain the same paths. In that context the tests seemed reverse.

Sorry for the confusion, reading the glib documentation would've made this
fairly clear (though still a bit surprising, imo)

> 
> Note that the test case that will be run in a subprocess must contain
> the "subprocess" component in its path, otherwise g_test_run() will try
> to run it as a normal test case. So for the test path, we don't have a
> choice. For the function names, I am just following the same convention
> used for the test path.
> 
> -- 
> Eduardo