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.
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.
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.
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...
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 on attachment 68122 [details] [review] Run the tests with G_MESSAGES_DEBUG=all Review of attachment 68122 [details] [review]: ----------------------------------------------------------------- +1
Comment on attachment 68123 [details] [review] TpConnection: connect to SelfHandleChanged much earlier Review of attachment 68123 [details] [review]: ----------------------------------------------------------------- +1
Comment on attachment 68124 [details] [review] _tp_dbus_properties_mixin_get_all: expose to internal code Review of attachment 68124 [details] [review]: ----------------------------------------------------------------- +1
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()?
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?
(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?
(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?
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().
(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.
Fixed in git for 0.20.1 and 0.21.0, but needs some significant work for next.
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.
Created attachment 68256 [details] [review] [next] vala: fix binding generation for 1.0
Created attachment 68257 [details] [review] [next] TpConnection: don't leak old self-IDs
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.
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).
Created attachment 68260 [details] [review] [next] TpConnection: ignore self-handle changes that aren't really changes
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.
(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.
+1
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.