Bug 69814

Created attachment 86714 [details] [review]
add logger-test-helper.c

Created attachment 86715 [details] [review]
tests/dbus/test-tpl-log-store-xml: use tpl_test_create_and_prepare_account()

Created attachment 86716 [details] [review]
tests/dbus/test-tpl-log-iters-xml: use tpl_test_create_and_prepare_account()

Created attachment 86717 [details] [review]
use account's normalized name instead of its account id

Created attachment 86718 [details] [review]
log-manager: factor out start_async_op_in_thread()

Created attachment 86719 [details] [review]
Prepare TpAccount if needed

Comment on attachment 86714 [details] [review]
add logger-test-helper.c

Review of attachment 86714 [details] [review]:
-----------------------------------------------------------------

::: tests/lib/logger-test-helper.c
@@ +38,5 @@
> +
> +void
> +tpl_test_create_and_prepare_account (TpDBusDaemon *dbus,
> +    TpSimpleClientFactory *factory,
> +    GMainLoop *main_loop,

Possible refinement (non-review-blocker): if you used tp_tests_proxy_run_until_prepared(), you wouldn't have to pass in a main loop or have a callback, so your logic would be more linear (which is pretty useful for regression tests, which want to be as clear as possible).

This is fine though.

Comment on attachment 86719 [details] [review]
Prepare TpAccount if needed

Review of attachment 86719 [details] [review]:
-----------------------------------------------------------------

r+ to everything above here.

Insta-r+ for this one if you add the comment and assertion:

::: telepathy-logger/log-manager.c
@@ +873,5 @@
> +typedef struct
> +{
> +  GSimpleAsyncResult *result;
> +  GSimpleAsyncThreadFunc func;
> +} AsyncOpData;

If you used GTask you might not need this, but I think that's a job for post-1.0.

@@ +923,5 @@
> +  if (account != NULL)
> +    {
> +      GQuark features[] = { TP_ACCOUNT_FEATURE_CORE, 0 };
> +
> +      /* Most API rely on TpAccount being prepared so make sure it is */

I think it's worth being more specific, something like this:

       /* Most APIs rely on TpAccount being prepared, so make sure
        * it is. telepathy-glib is not thread-safe, so we must do
        * this in the main thread, before starting the actual
        * operation in the other thread. */

You can just use features = NULL I think? (No harm in being explicit, though.)

::: tests/dbus/test-log-manager.c
@@ +402,5 @@
> +  account_path = g_value_get_string (
> +      (const GValue *) g_hash_table_lookup (params, "account-path"));
> +
> +  account = tp_simple_client_factory_ensure_account (fixture->factory,
> +      account_path, NULL, NULL);

If you intend the account to be unprepared here, please assert that it is unprepared.

(If it might be prepared already, you might have to create a new factory per test run in order to ensure duplication...)

(In reply to comment #7)
> Possible refinement (non-review-blocker): if you used
> tp_tests_proxy_run_until_prepared(), you wouldn't have to pass in a main
> loop or have a callback, so your logic would be more linear (which is pretty
> useful for regression tests, which want to be as clear as possible).

changed.

(In reply to comment #8)
> ::: telepathy-logger/log-manager.c
> @@ +873,5 @@
> > +typedef struct
> > +{
> > +  GSimpleAsyncResult *result;
> > +  GSimpleAsyncThreadFunc func;
> > +} AsyncOpData;
> 
> If you used GTask you might not need this, but I think that's a job for
> post-1.0.

Agreed, post 1.0.

> @@ +923,5 @@
> > +  if (account != NULL)
> > +    {
> > +      GQuark features[] = { TP_ACCOUNT_FEATURE_CORE, 0 };
> > +
> > +      /* Most API rely on TpAccount being prepared so make sure it is */
> 
> I think it's worth being more specific, something like this:
> 
>        /* Most APIs rely on TpAccount being prepared, so make sure
>         * it is. telepathy-glib is not thread-safe, so we must do
>         * this in the main thread, before starting the actual
>         * operation in the other thread. */

changed.

> You can just use features = NULL I think? (No harm in being explicit,
> though.)

Yeah I prefer to be explicit.

> ::: tests/dbus/test-log-manager.c
> @@ +402,5 @@
> > +  account_path = g_value_get_string (
> > +      (const GValue *) g_hash_table_lookup (params, "account-path"));
> > +
> > +  account = tp_simple_client_factory_ensure_account (fixture->factory,
> > +      account_path, NULL, NULL);
> 
> If you intend the account to be unprepared here, please assert that it is
> unprepared.
> 
> (If it might be prepared already, you might have to create a new factory per
> test run in order to ensure duplication...)

Oh I used to have this assert actually but I guess I accidentally removed it. Re-added.

Thanks for the review.

Merged to master.