Bug 55761 - Use GTestDBus instead of home made with-session-bus.sh
Summary: Use GTestDBus instead of home made with-session-bus.sh
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-glib (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL: http://cgit.collabora.com/git/user/xc...
Whiteboard:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2012-10-08 12:48 UTC by Xavier Claessens
Modified: 2013-09-26 12:31 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Description Xavier Claessens 2012-10-08 12:48:36 UTC
GLib 2.34 now has GTestDBus API that ensure that a session bus is running. It is more convenient because we can run unit tests binary directly instead of passing by some bash wrappers. It is more flexible because we can restart session bus between tests to make sure a problem in previous test won't affect the next one.
Comment 1 Xavier Claessens 2012-10-08 13:27:01 UTC
Here is a patch: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=g-test-dbus

I got 3 tests failing with glib 2.34.0-1ubuntu1 but they all pass with current glib master. With 2.34.0 the test actually succeed, but then in g_test_dbus_down() something does an exit() that makes the process return failure. This is weird because g_test_dbus_down() calls g_dbus_connection_set_exit_on_close (connection, FALSE); to make sure that does not happen.
Comment 2 Xavier Claessens 2012-10-08 13:49:50 UTC
actually tested with glib 2.34.0 upstream and it works nicely. I only fails with ubuntu package... Maybe a patch on their side...
Comment 3 Simon McVittie 2012-10-08 14:12:44 UTC
> +tp_test_run (void)

The C namespace for this code is tp_tests. tp_tests_run_with_dbus_session() or tp_tests_run_with_bus() maybe?

> + TP_TESTS_SERVICES_DIR=@abs_srcdir@/services \

I would prefer these to return to @abs_srcdir@/dbus-1/services to mirror their location on real systems (/usr/share/dbus-1/services).

run-test.sh.in needs to set this environment variable, too (it should be @tpglibtestsdir@/dbus-1/services there, with dbusservicedir set accordingly).

> tp_tests_dbus_daemon_dup_or_die (void)
> {
> - TpDBusDaemon *d = tp_dbus_daemon_dup (NULL);
> + TpDBusDaemon *d;
> +
> + if (test_dbus == NULL)
> + {
> + start_dbus_session ();

I'm unsure about this function getting a non-obvious side-effect... I suppose it's not too bad, but I think I'd prefer it if you exposed tp_tests_start_dbus_session() and tp_tests_stop_dbus_session(), and called them from main() in each test that doesn't use tp_tests_run_with_dbus_session(). That way, leaking a reference to the TpDBusDaemon (which I'm sure we do, despite our best efforts) wouldn't mean we leaked a dbus-daemon process.

> With 2.34.0 the test actually succeed, but then in
> g_test_dbus_down() something does an exit() that makes the process return
> failure.

My guess would be that this is the libdbus shared connection doing its exit-on-close behaviour.

> This is weird because g_test_dbus_down() calls
> g_dbus_connection_set_exit_on_close (connection, FALSE); to make sure that
> does not happen

... but that doesn't affect libdbus.

Perhaps tp_tests_stop_dbus_session() should call dbus_g_bus_get() and turn off its exit-on-disconnect flag before terminating the session? (In practice we always leak the dbus-glib shared connection anyway.)

Then the only tests that would have to turn off exit-on-disconnect themselves would be those that create private bus connections, which seems fair.
Comment 4 Xavier Claessens 2012-10-08 16:04:05 UTC
(In reply to comment #3)
> > +tp_test_run (void)
> 
> The C namespace for this code is tp_tests. tp_tests_run_with_dbus_session()
> or tp_tests_run_with_bus() maybe?

Right, changed to tp_tests_run_with_bus().

> > + TP_TESTS_SERVICES_DIR=@abs_srcdir@/services \
> 
> I would prefer these to return to @abs_srcdir@/dbus-1/services to mirror
> their location on real systems (/usr/share/dbus-1/services).

done

> run-test.sh.in needs to set this environment variable, too (it should be
> @tpglibtestsdir@/dbus-1/services there, with dbusservicedir set accordingly).

Did not know about that run-test.sh, it was still using with-session-bus.sh.
Fixed.

> > tp_tests_dbus_daemon_dup_or_die (void)
> > {
> > - TpDBusDaemon *d = tp_dbus_daemon_dup (NULL);
> > + TpDBusDaemon *d;
> > +
> > + if (test_dbus == NULL)
> > + {
> > + start_dbus_session ();
> 
> I'm unsure about this function getting a non-obvious side-effect... I
> suppose it's not too bad, but I think I'd prefer it if you exposed
> tp_tests_start_dbus_session() and tp_tests_stop_dbus_session(), and called
> them from main() in each test that doesn't use
> tp_tests_run_with_dbus_session(). That way, leaking a reference to the
> TpDBusDaemon (which I'm sure we do, despite our best efforts) wouldn't mean
> we leaked a dbus-daemon process.

Yeah, I've done this to make change as less intrusive as possible, it's a pain to edit each unit test... ideally they should all use tp_tests_run_with_bus() but some outdated tests still does code in main().

GTestDBus fork() a cleanup process that takes care of killing the dbus-daemon if unit test doesn't.

g_test_dbus_down() also wait until the last ref on the GDBusConnection session singleton is dropped (with a timeout). I think we should do the same to ensure TpDBusDaemon isn't leaked, but that can be added later.

> > With 2.34.0 the test actually succeed, but then in
> > g_test_dbus_down() something does an exit() that makes the process return
> > failure.
> 
> My guess would be that this is the libdbus shared connection doing its
> exit-on-close behaviour.
> 
> > This is weird because g_test_dbus_down() calls
> > g_dbus_connection_set_exit_on_close (connection, FALSE); to make sure that
> > does not happen
> 
> ... but that doesn't affect libdbus.
> 
> Perhaps tp_tests_stop_dbus_session() should call dbus_g_bus_get() and turn
> off its exit-on-disconnect flag before terminating the session? (In practice
> we always leak the dbus-glib shared connection anyway.)
> 
> Then the only tests that would have to turn off exit-on-disconnect
> themselves would be those that create private bus connections, which seems
> fair.

Turned out this was not needed. It is dconf/gvfs that uses the bus session and don't release it. I've reported https://bugzilla.gnome.org/show_bug.cgi?id=685734
Comment 5 Xavier Claessens 2013-09-26 12:31:48 UTC
Fixed \o/


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.