(These are only for next: in particular, "Move tp_value_array_* to the core library" is an ABI break.)

Looks good.

Thanks, merged for 0.99.8.

Here's another batch, for which I'll recycle this bug since you had so few comments: http://cgit.freedesktop.org/~smcv/telepathy-glib/log/?h=gdbus-prep3

 - "account-request test: don't leak account manager": I like adding in setup:

  g_object_add_weak_pointer (G_OBJECT (foo), &foo);

  and in teardown:

  g_object_unref (foo);
  g_assert(foo == NULL);

 - "tp_tests_proxy_run_until_dbus_queue_processed: don't use Introspect": I don't like that hack, but I've got no better idea to workaround. Let's hope a proper fix can go into GDBus.

 - "account-request test: prepare account manager": that really is a job for the g_object_add_weak_pointer() trick I said above to be sure we are not reusing the same singleton in next test.

 - "_tp_channel_got_properties: if we were invalidated, don't continue": Why do we get the callback if the proxy is invalidated? I though that was one of the difference between tp_cli_foo_call_bar() and g_dbus_connection_call(), the former does not call callback and the later does call the callback but with a GError set.

 - "tp_private_proxy_set_implementation: don't use g_assert_cmp* family": you can use g_str_equal() when you know both strings are non-NULL.

 - "stream-tube test: don't assume one event per main loop iteration": In create_tube_service() you should set test->tube_conns to NULL after g_list_free_full() I think.

I tested here and make check pass, good job :-)

(In reply to comment #4)
>  - "account-request test: don't leak account manager": I like adding in
> setup:
> 
>   g_object_add_weak_pointer (G_OBJECT (foo), &foo);
> 
>   and in teardown:
> 
>   g_object_unref (foo);
>   g_assert(foo == NULL);

An ingenious hack, but I don't really like overloading @foo to mean two different things (the strong ref and the weak ref). I prefer the approach taken by tp_tests_assert_last_unref(), which I added recently.

In any case, this doesn't actually work in that test. We would have to add a tp_tests_await_last_unref() which spins the main context, because the TpAM is kept alive by pending method calls after at least some test-cases (usually the ones that do something incredibly trivial, like constructing a $object and immediately destroying it, leading to the test *after* the trivial one failing).

I'm not sure that I really want tp_tests_await_last_unref(), but I could be persuaded.

>  - "account-request test: prepare account manager": that really is a job for
> the g_object_add_weak_pointer() trick I said above to be sure we are not
> reusing the same singleton in next test.

This is the same situation. I did try fixing it by adding tp_tests_await_last_unref(), and it worked, but I didn't really like that solution: it's all too easy for it to become a hard-to-explain "timed out" and then you have to try to find where it was leaked. In general, leaked pointers having observable side-effects are Bad™ - I'm really regretting the weak_object parameters in our APIs now.

I wonder whether the default TpAM (or in Bug #76111 world, the default client factory) should "forget" otherwise-singleton objects if they are invalidated, not just if they are disposed. Then we'd never get this situation persisting beyond the short term "in real life", and regression tests could maybe even forcibly invalidate them.

Then again, in Bug #76111 world, we'd probably avoid using the global default TpClientFactory in tests (except in the single test designed to test it), and call tp_tests_assert_last_unref() on the temporary TpClientFactory in teardown?

>  - "_tp_channel_got_properties: if we were invalidated, don't continue": Why
> do we get the callback if the proxy is invalidated? I though that was one of
> the difference between tp_cli_foo_call_bar() and g_dbus_connection_call(),
> the former does not call callback and the later does call the callback but
> with a GError set.

No. I think you're thinking of cancellation: cancelling a tp_cli_foo_call_bar() call (either by explicit cancellation, or death of the weak object) means the callback is not called at all, even if the result has already been received and the only thing remaining to do is to actually call the callback (in an idle).

Cancelling a cancellable GIO async call means the callback is called with either the real result or G_IO_ERROR_CANCELLED (usually the latter, but there is no guarantee - you might be "too late").

I should write *simple* test-cases for this stuff. call-cancellation.c and disconnection.c don't count, because they're full of corner-cases (although they have been invaluable for getting the corner-cases right).

>  - "tp_private_proxy_set_implementation: don't use g_assert_cmp* family":
> you can use g_str_equal() when you know both strings are non-NULL.

Yeah, I suppose so. If one of the strings happens to be NULL then we'll jsut crash, which is what we wanted anyway, since this is "internal error, something has gone horribly wrong" territory...

>  - "stream-tube test: don't assume one event per main loop iteration": In
> create_tube_service() you should set test->tube_conns to NULL after
> g_list_free_full() I think.

Reasonable.

tp_tests_await_last_unref(): That's actually what GTestDBus does to ensure the GDBusConnection singleton goes away at the end of the test, to be sure it won't be reused for next test. I think it is important to make sure that the singleton goes away in one way or another at the end of each test to avoid subtle issues in the next test.

(In reply to comment #4)
>  - "account-request test: don't leak account manager"

Added a whole stack of commits to make sure everything is freed ("tp_tests_await_last_unref: add" and everything newer than it).

>  - "tp_tests_proxy_run_until_dbus_queue_processed: don't use Introspect": I
> don't like that hack, but I've got no better idea to workaround. Let's hope
> a proper fix can go into GDBus.

No action taken.

>  - "account-request test: prepare account manager"

See above. I replaced this with a tp_tests_await_last_unref call.

>  - "_tp_channel_got_properties: if we were invalidated, don't continue"

No action taken.

>  - "tp_private_proxy_set_implementation: don't use g_assert_cmp* family":
> you can use g_str_equal() when you know both strings are non-NULL.

Done, there's a fixup commit just after the one you're commenting on.

>  - "stream-tube test: don't assume one event per main loop iteration": In
> create_tube_service() you should set test->tube_conns to NULL after
> g_list_free_full() I think.

Done, there's a fixup commit just after the one you're commenting on. I'd have left it at the end of the branch, but it conflicts.

gdbus branch rebased onto it.

I really like tp_tests_assert_last_unref() and tp_tests_await_last_unref(), the implementation is even nicer than the code we have in GTestDBus, maybe they could be proposed to glib. I would just check time spent in the loop at each iteration and assert it's smaller than X seconds. But in tp-glib we already have a global timeout for tests, right?

+1 for that branch.

(In reply to comment #8)
> But in tp-glib we already have a global timeout for tests, right?

Yes, it's part of the test setup boilerplate.

Unfortunately, the Logger tests don't use that boilerplate, which is one of the reasons those tests are failing under GDBus.

Fixed in git for 0.99.8