[Spice-devel] [PATCH spice-server v3 07/12] test-sasl: Add tests for different mechanism names

[Christophe Fergeau]

On Fri, Jan 05, 2018 at 04:47:55AM -0500, Frediano Ziglio wrote:
> > 
> > On Fri, Dec 22, 2017 at 10:07:08AM +0000, Frediano Ziglio wrote:
> > > @@ -395,16 +427,32 @@ idle_next_test(void *arg)
> > >  {
> > >      // end previous test
> > >      if (test_num >= 0) {
> > > -        g_assert(encode_called);
> > > +        const TestData *data = &tests_data[test_num];
> > > +        if (data->success) {
> > > +            g_assert(encode_called);
> > > +        } else {
> > > +            g_assert(mechlist_called);
> > > +            g_assert(!encode_called);
> > > +        }
> > >          reset_test();
> > > -        basic_event_loop_quit();
> > > -        return FALSE;
> > >      }
> > >  
> > >      // start next test
> > >      ++test_num;
> > >      alarm(4);
> > >  
> > > +    if (test_num >= G_N_ELEMENTS(tests_data)) {
> > > +        basic_event_loop_quit();
> > > +        return FALSE;
> > > +    }
> > > +
> > > +    TestData *data = &tests_data[test_num];
> > > +    if (data->mechlen < 0) {
> > > +        data->mechlen = strlen(data->mechname);
> > > +    }
> > > +    int len = data->mechlen;
> > > +    printf("\nRunning test %d ('%*.*s' %d)\n", test_num, len, len,
> > > data->mechname, len);
> > > +
> > >      int sv[2];
> > >      g_assert_cmpint(socketpair(AF_LOCAL, SOCK_STREAM, 0, sv), ==, 0);
> > 
> > I haven't read over the whole tests yet, but I find the way you iterate
> > over the tests through idle_next_test() quite hard to read. Adding the
> > changes below on top of this series make things easier to follow in my
> > opinion.
> > 
> > diff --git a/server/tests/basic-event-loop.c
> > b/server/tests/basic-event-loop.c
> > index 6fd8a2934..12205ecef 100644
> > --- a/server/tests/basic-event-loop.c
> > +++ b/server/tests/basic-event-loop.c
> > @@ -130,7 +130,7 @@ SpiceCoreInterface *basic_event_loop_init(void)
> >  {
> >      ignore_sigpipe();
> >      spice_assert(main_context == NULL);
> > -    main_context = g_main_context_new();
> > +    main_context = g_main_context_default();
> >      base_core_interface = event_loop_core;
> >      base_core_interface.main_context = main_context;
> >  
> 
> I would strongly avoid this. Qemu can use different contexts and using the default
> can lead tests works while spice-server won't work on real world cases.
> I would retain my idle_add function to deal with this. Rest of proposal make sense,
> I'll merge it.
> Would be also better to put some comments why not using the default context for
> future reference.

Yup, definitely, I did not realize you had a very strong (and good)
reason for not using the default context.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180105/eda44e05/attachment.sig>