Bug 55666

Summary: only monitors SelfHandleChanged if the CM is obsolete
Product: Telepathy Reporter: Simon McVittie <smcv>
Component: tp-glibAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium CC: guillaume.desmottes, jonny.lamb, xclaesse
Version: unspecifiedKeywords: patch
Hardware: Other   
OS: All   
URL: http://cgit.freedesktop.org/~smcv/telepathy-glib/log/?h=self-handle-changed
Whiteboard:
i915 platform: i915 features:
Attachments: Run the tests with G_MESSAGES_DEBUG=all
TpConnection: connect to SelfHandleChanged much earlier
_tp_dbus_properties_mixin_get_all: expose to internal code
self-handle test: test with a modern CM, not just an archaic one
self-handle test: test with a modern CM, not just an archaic one
[0.20] tests/lib: define _TP_COMPILATION for the internal variant
[next] vala: fix binding generation for 1.0
[next] TpConnection: don't leak old self-IDs
[next] TpConnection: correct an outdated comment
[next] TpConnection: don't block introspection if the self-contact changes
[next] TpConnection: ignore self-handle changes that aren't really changes
[next] self-handle test: reinstate test-case for self-contact changing at an inconvenient time

Description Simon McVittie 2012-10-05 15:10:30 UTC
I was refactoring Aliasing in MC (bug forthcoming) as a follow-up to Bug #55391. Then I noticed MC made no attempt to track the self-handle changing, which is clearly wrong for at least Idle (assuming Idle even implements SelfHandleChanged, which it might not: Bug #17332). Then I fixed that, wrote a regression test, and discovered it failed because telepathy-glib doesn't respond to the signal being emitted...

This turns out to be because the regression test emulates an old CM, because it hooks GetSelfHandle so it can trigger a particular corner-case; and that's a problem when emulating a newer CM because it's harder to hook GetAll. So to test this properly, we have to expose the mixin's GetAll() implementation to CM code. Additional yak-shaving.

When porting to 'next' we just deleted that test, which is ... not really the right approach.

How we test this depends which branch(es) we want to get the tests into.
Comment 1 Simon McVittie 2012-10-05 15:12:31 UTC
Created attachment 68122 [details] [review]
Run the tests with G_MESSAGES_DEBUG=all

---

This can safely go in stable branches. It should probably hit 0.20, at least.
Comment 2 Simon McVittie 2012-10-05 15:12:55 UTC
Created attachment 68123 [details] [review]
TpConnection: connect to SelfHandleChanged much earlier

Previously, we only connected to it just before calling GetSelfHandle -
but on the "modern" code path we no longer call that, because
GetAll("...Connection") is just as informative. This meant we missed
self-handle changes on modern connection managers.

This was masked by the fact that the self-handle regression test
deliberately breaks the "modern" code path, because some of its tests
rely on GetSelfHandle being called.
Comment 3 Simon McVittie 2012-10-05 15:15:05 UTC
Created attachment 68124 [details] [review]
_tp_dbus_properties_mixin_get_all: expose to internal  code

The self-handle test can't exercise certain situations without this,
except by pretending to be an obsolete CM, which means we don't test
the non-obsolete code path properly.

---

If we want the test that I'm about to attach on stable branches, we'll have to do this, since the alternative is to add ABI.

If we're only adding the test to 0.21 then we could just rename to tp_dbus_properties_mixin_get_all(), make it public and document it; but it's dbus-glib-type-based so I think we should (skip) it.

Or I could add a public-API version in terms of GVariant if people want...
Comment 4 Simon McVittie 2012-10-05 15:16:32 UTC
Created attachment 68125 [details] [review]
self-handle test: test with a modern CM, not just an  archaic one

If we'd done this at the time, we wouldn't have broken SelfHandleChanged.

---

On 'next', we should delete the "archaic" code path but keep the non-archaic one. This means /self-handle/fails will still not be tested on 'next' (it's a situation that just can't arise with current API), but /self-handle/change-inconveniently should be added back to 'next'.
Comment 5 Xavier Claessens 2012-10-08 08:12:48 UTC
Comment on attachment 68122 [details] [review]
Run the tests with G_MESSAGES_DEBUG=all

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

+1
Comment 6 Xavier Claessens 2012-10-08 08:14:39 UTC
Comment on attachment 68123 [details] [review]
TpConnection: connect to SelfHandleChanged much earlier

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

+1
Comment 7 Xavier Claessens 2012-10-08 08:25:17 UTC
Comment on attachment 68124 [details] [review]
_tp_dbus_properties_mixin_get_all: expose to internal  code

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

+1
Comment 8 Xavier Claessens 2012-10-08 08:30:58 UTC
Comment on attachment 68125 [details] [review]
self-handle test: test with a modern CM, not just an  archaic one

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

::: tests/lib/simple-conn.c
@@ +544,5 @@
> +      g_debug ("\t%s = '%s'", (const gchar *) k, pretty);
> +      g_free (pretty);
> +    }
> +
> +  g_debug ("\tend");

Do we need those g_debug(), and shouldn't them be DEBUG()?
Comment 9 Xavier Claessens 2012-10-08 08:34:18 UTC
tbh, I don't understand why we do both internal and non-internal libs for tests? Why can't all tests just link to tp-glib's internal by default?
Comment 10 Simon McVittie 2012-10-08 09:57:20 UTC
(In reply to comment #8)
> Do we need those g_debug(), and shouldn't them be DEBUG()?

1) Not really, I've deleted them.

2) If I'd kept them: yes, I should have #include'd "tests/lib/debug.h" (not that we use it very consistently).

(In reply to comment #9)
> tbh, I don't understand why we do both internal and non-internal libs for
> tests? Why can't all tests just link to tp-glib's internal by default?

Linking tests against the shared library means we're testing the same binary that library users use, which is after all what we wanted to achieve; it's also good for efforts like Debian's DEP-8, which run tests in the installed system.

I might turn _tp_dbus_properties_mixin_get_all() into public API on 0.21, at which point this test can go back to being linked to the shared library. Any objection?
Comment 11 Xavier Claessens 2012-10-08 10:25:07 UTC
(In reply to comment #10)
> I might turn _tp_dbus_properties_mixin_get_all() into public API on 0.21, at
> which point this test can go back to being linked to the shared library. Any
> objection?

We already have tp_dbus_properties_mixin_get() in public, so that makes sense, yes. But then, GVariant or GValue?
Comment 12 Simon McVittie 2012-10-08 10:50:49 UTC
Created attachment 68250 [details] [review]
self-handle test: test with a modern CM, not just an archaic  one

If we'd done this at the time, we wouldn't have broken SelfHandleChanged.

---

Now without g_debug(). The only change is in get_all().
Comment 13 Simon McVittie 2012-10-08 10:51:52 UTC
(In reply to comment #11)
> We already have tp_dbus_properties_mixin_get() in public, so that makes
> sense, yes. But then, GVariant or GValue?

GValue, I think. TpDBusPropertiesMixin is service-side (where everything is still tied to dbus-glib), and isn't usefully introspectable anyway.
Comment 14 Simon McVittie 2012-10-08 13:36:20 UTC
Fixed in git for 0.20.1 and 0.21.0, but needs some significant work for next.
Comment 15 Simon McVittie 2012-10-08 13:37:32 UTC
Created attachment 68255 [details] [review]
[0.20] tests/lib: define _TP_COMPILATION for the internal  variant

foo_CPPFLAGS overrides AM_CPPFLAGS, so if you want to include the latter
in the former, you have to do it explicitly.

---

This patch is against next, but would also be appropriate for 0.20.
Comment 16 Simon McVittie 2012-10-08 13:38:02 UTC
Created attachment 68256 [details] [review]
[next] vala: fix binding generation for 1.0
Comment 17 Simon McVittie 2012-10-08 13:38:18 UTC
Created attachment 68257 [details] [review]
[next] TpConnection: don't leak old self-IDs
Comment 18 Simon McVittie 2012-10-08 13:38:41 UTC
Created attachment 68258 [details] [review]
[next] TpConnection: correct an outdated comment

We previously said "I wonder what that means?" because we'd have to
inspect the new self-handle to find the new self-ID, but we no longer
need to do that.
Comment 19 Simon McVittie 2012-10-08 13:39:08 UTC
Created attachment 68259 [details] [review]
[next] TpConnection: don't block introspection if the  self-contact changes

If the self-contact changes while we're introspecting it,
the upgrade callback for the old self-contact does nothing, and we'll
wait forever - unless we introspect the *new* self-contact, to get
back on track.

Unfortunately, we deleted the test-case for this on the 1.0 branch.
It's a little harder to test, because introspecting the self-contact
is usually instantaneous (the exception being if we force a round-trip
while adding features).
Comment 20 Simon McVittie 2012-10-08 13:39:31 UTC
Created attachment 68260 [details] [review]
[next] TpConnection: ignore self-handle changes that aren't  really changes
Comment 21 Simon McVittie 2012-10-08 13:40:29 UTC
Created attachment 68261 [details] [review]
[next] self-handle test: reinstate test-case for self-contact  changing at an inconvenient time

---

For good measure, also retest the other two scenarios with a TpContact preparation step that is not just an idle.
Comment 22 Simon McVittie 2012-10-08 13:42:12 UTC
(In reply to comment #15)
> [0.20] tests/lib: define _TP_COMPILATION for the internal  variant
> 
> foo_CPPFLAGS overrides AM_CPPFLAGS, so if you want to include the latter
> in the former, you have to do it explicitly.
> 
> ---
> 
> This patch is against next, but would also be appropriate for 0.20.

Context: on 0.20 and master, I didn't notice this because single-include checking is off-by-default, so _TP_COMPILATION has no effect; but on next, it's on-by-default so the build failed.
Comment 23 Xavier Claessens 2012-10-08 14:13:10 UTC
+1
Comment 24 Simon McVittie 2012-10-08 15:23:33 UTC
Fixed in git for 0.20.1, 0.21.0 and 1.0.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.