|
Description
Simon McVittie
2012-10-05 15:10:30 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. 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.