GNOME Bugzilla – Bug 672985
Move gdbus-sessionbus to GTest
Last modified: 2012-04-19 08:23:16 UTC
gdbus-sessionbus.c,h has very smart and useful code to write DBus unit tests. Telepathy-glib does similar tricks but as ugly wrapper shell scripts, which prevents us starting the unit test just by ./tests/foo since a whole env is needed. That code does not seems to use any of the libgio/libgobject, so it could be moved down to libglib's GTest. Something like g_test_bus_up/down?
Yeah, would definitely be useful to have something like this in GLib. I think Canonical has something they call a dbus-test-runner which is similar - might be worth looking into that as well?
Do you have link to canonical's code?
(In reply to comment #2) > Do you have link to canonical's code? http://bazaar.launchpad.net/~indicator-applet-developers/dbus-test-runner/trunk/files
From a quick look (but I could be wrong) gdbus's code seems more convenient for a library, since it's functions I call from my app, and not a standalone application that needs to be used as wrapper for my app. Also dbus-test-runner is GPL, uncompatible with glib's LGPL. (And copyright assignment to canonical?)
(In reply to comment #4) > From a quick look (but I could be wrong) gdbus's code seems more convenient for > a library, since it's functions I call from my app, and not a standalone > application that needs to be used as wrapper for my app. > > Also dbus-test-runner is GPL, uncompatible with glib's LGPL. (And copyright > assignment to canonical?) Oh, I wasn't talking about using canonical's code... just looking at their interface. So.. let's start.. what should the interface in GTest look like? You mentioned something like g_test_bus_up()? My gut feeling is that we want something like g_test_trap_fork()... but I haven't given it much thought. Asking desrt and smcv what they think might be a good idea...
Oh before I forget I could mention here another requirement that could be related to this code: Add API on GTest to set XDG_*_HOME to a g_dir_make_tmp(). Then the forked process we use atm to kill dbus-daemon/dbus-monitor could be extended to also delete the temporary directory. (In reply to comment #5) > Oh, I wasn't talking about using canonical's code... just looking at their > interface. So.. let's start.. what should the interface in GTest look like? You > mentioned something like g_test_bus_up()? My gut feeling is that we want > something like g_test_trap_fork()... but I haven't given it much thought. > Asking desrt and smcv what they think might be a good idea... What do you mean by g_test_trap_fork() exactly?
(In reply to comment #6) > What do you mean by g_test_trap_fork() exactly? Yeah, sorry, wasn't really clear what I meant... Basically, I was thinking that the way you'd write the test was something like this static void my_test (void) { /* You are guaranteed that a session bus is running */ } g_test_add_func_with_session_bus (my_test); this way, the GTester core would simply just bring up the bus if needed. Now... thinking more about it, I'm not so sure it's a good idea. Maybe just having a cleaned up version of session_bus_*() as it currently exists in gio/tests/gdbus-sessionbus.h is "good enough". I mean, that way, people writing test cases simply just call g_test_dbus_session_bus_up() in main() ... if you want to do more complicated stuff, like seeing how your code reacts when the bus vanishes, you can do so too... but I doubt anyone wants to try and handle that [1] since, generally, apps should just exit if that happens (this is what we try to teach people). The reason I was thinking about just having g_test_add_func_with_session_bus() is that we wouldn't need to expose g_test_dbus_session_bus_up() / g_test_dbus_session_bus_down() at all... not having to expose this would be a Good Thing(tm) as it helps reinforce the advice that people shouldn't try to handle the case when a bus connection is closed. [1] : the exception here are the test cases for GDBus itself... e.g. we need to make sure that our core D-Bus routines behaves as advertised when the connection is closed
g_test_add_func_with_session_bus() would be nice, indeed. But there is also the case you need to give a setup/teardown functions, so that would be g_test_add_with_session_bus(). Quick note: currently each unit test calls this in main(): g_setenv ("DBUS_SESSION_BUS_ADDRESS", session_bus_get_temporary_address (), TRUE); clearly that should be hidden in the API somewhere.
I think up() and down() functions for use in setup() and teardown() would be a good first step: GDBus will want to use them directly, and other tests can either do so too, or migrate to something even more convenient. I'm not sure how the startup time of dbus-daemon compares with the runtime of a typical test case; perhaps it'd be better to "usually" carry over the same dbus-daemon for every test in an executable, so do the up/down in main rather than in setup/teardown? At least some tests in Telepathy need to point XDG_DATA_HOME, XDG_DATA_DIRS into the source and/or build trees; this would also be useful GTest functionality. It'd be even better if there was a way to point the test to a specified dbus-daemon executable, and specify a particular configuration file, so I can use this to run the just-compiled bus/dbus-daemon${EXEEXT} in libdbus :-) (At the moment I use g_spawn_async_with_pipes in libdbus, though, so that isn't necessary)
I do wonder whether future GTest extensions like this should be their own shared library, parallel to libglib/libgobject/libgio (but in the same source tree to avoid circular build-dependencies). It's already a bit odd that libglib-the-core-system-library contains a JUnit clone; the more test helpers we add to libglib, the more bloat we get in what's becoming a fairly fundamental library. In general there are good performance reasons for avoiding proliferation of shared libraries (and, e.g., killing off gthread), but I'm not sure that those reasons really apply to regression-test infrastructure.
(In reply to comment #10) > I do wonder whether future GTest extensions like this should be their own > shared library, parallel to libglib/libgobject/libgio (but in the same source > tree to avoid circular build-dependencies). It's already a bit odd that > libglib-the-core-system-library contains a JUnit clone; the more test helpers > we add to libglib, the more bloat we get in what's becoming a fairly > fundamental library. Yeah, I agree. A separate library (and separate git module) would make sense. If it's a developer-only module (in industry terms, part of the "SDK"), then we're much more free to stuff it with tons of utilities, debug helpers, etc.
Created attachment 210928 [details] [review] Add DBus helper in GTest framework gtestdbus.c,h is a copy of gio/tests/gdbus-sessionbus.c,h. It helpes unit tests to run with a private session bus.
Created attachment 210929 [details] [review] Use g_test_dbus_ instead of session_bus_
Attached patches to move a verbatim (except for #include tweaks) down to glib/. This is a base to work on. My git branch: http://cgit.collabora.com/git/user/xclaesse/glib.git/log/?h=dbus-test
oh, something to think about, is that it works only on unix AFAIK, because of the fork() call.
Note about not cluttering libglib: it could be moved to libgio and still keep the g_test_ namespace maybe? IIRC libgobject also has some symbols in libglib's namespace to extend with types, no? Any app using g_test_dbus_ will surely use libgio anyway.
> (and separate git module) would make sense I didn't say it should be a separate git module. I think it *shouldn't* be a separate git module / tarball release / SRPM / etc., because that'd be a circular dependency, which isn't really necessary if the same people are maintaining and releasing them anyway: * GTest requires GLib (for utility functions) * GLib requires GTest (assuming packagers want to enable tests) > If it's a developer-only module (in industry terms, part of the "SDK"), then > we're much more free to stuff it with tons of utilities, debug helpers, etc. I think a separate shared library, which packagers are encouraged to split into a separate binary package (is that "subpackage" in RPM jargon?), would make sense on this basis, though. (The jargon used in dpkg, which is what I'm used to, is that glib2.0 is a "source package", which builds libglib2.0-0 and libglib2.0-dev "binary packages" - analogous to each SRPM building one or more RPMs in the RPM world.)
Do you guys seriously think we should start a new lib for this? It's just about 400 lines of code, clearly not biggest thing pushed to libglib nowadays. But I agree that GTest could have been its own lib from the beginning, but that seems a bit late now.
Created attachment 211116 [details] [review] Add DBus helper in GTest framework gtestdbus.c,h is a copy of gio/tests/gdbus-sessionbus.c,h. It helpes unit tests to run with a private session bus.
Created attachment 211117 [details] [review] Use g_test_dbus_ instead of session_bus_
Created attachment 211118 [details] [review] Simplify g_test_dbus_ API Only expose g_test_dbus_up/down/unset which are the only features needed by current gio unit tests.
Created attachment 211122 [details] [review] Add g_test_xdg_set/unset()
I've taken a look to see if this meets tp-glib's needs, but it seems we need a way to add activatable services from source dir in the dbus-daemon config file. Maybe g_test_dbus_up() should take a "const gchar *path" arg to a config file (optionally NULL to use the build-in settings) ?
(In reply to comment #23) > I've taken a look to see if this meets tp-glib's needs, but it seems we need a > way to add activatable services from source dir in the dbus-daemon config file. > > Maybe g_test_dbus_up() should take a "const gchar *path" arg to a config file > (optionally NULL to use the build-in settings) ? Sounds reasonable to me.... is this used in addition to the built-in settings or does it replace it? I think the former... and if so, we should say something about filenames/priorities as well, right?
actually just a path to the dir where are .service files is enough, and that path can be included into build-in configs.
or would you prefer g_test_dbus_up_with_services_dir() or something?
(In reply to comment #25) > actually just a path to the dir where are .service files is enough, and that > path can be included into build-in configs. Just taking a path do a directory of .conf files sounds good to me, thanks.
Btw, another thing that I talked to Matthias about in the office today is that g_test_dbus_down() should block in the default mainloop until all references to the GDBusConnection is gone [1]... and loudly complain if this doesn't happen within, say, 5 seconds. If we don't do something like this, then we end up in a situation where g_bus_get_sync() will return the old singleton pointing to a now non-existing message bus. [1] : we should do this by - Get a reference to the bus - Wait until the reference count of the obtained GDBusConnection drops to 1 - then unref the obtained GDBusConnection Note that the GDBus test suite, in particular gio/tests/gdbus-connection.c already does this and it should be straightforward to copy-paste it.
Review of attachment 211122 [details] [review]: I don't like the name. Something more descriptive would be better. Also, the set of directories seems a bit random. What about XDG_RUNTIME_DIR or /var/tmp ?
Review of attachment 211116 [details] [review]: I think this api should live with gdbus, in gio. In addition to the functions you have here, we need a way to say 'ignore a preexisting session bus'. Ie the equivalent of g_unsetenv ("DBUS_SESSION_BUS_ADDRESS")
Review of attachment 211118 [details] [review]: Ah ok, the api is fine here - still want to move this to gio, though. And, one more thing: g_test_dbus_down() should arrange for g_bus_get_sync(G_BUS_TYPE_SESSION,...) to return NULL once it returns. This involves something like _g_object_wait_for_single_ref (c) g_object_unref (c) in the current code.
(In reply to comment #29) > Review of attachment 211122 [details] [review]: > > I don't like the name. Something more descriptive would be better. > Also, the set of directories seems a bit random. What about XDG_RUNTIME_DIR or > /var/tmp ? I don't like the name neither, tbh. Suggestions? I was thinking about adding a flag argument for dirs to set, like typedef enum { G_TEST_XDG_DIR_CONFIG_HOME = 1 << 0, G_TEST_XDG_DIR_DATA_HOME = 1 << 1, G_TEST_XDG_DIR_CACHE_HOME = 1 << 2, etc... } GTestXDGDirFlags /* Better name here */ And multiple calls to the function would sum the flags. Also something annoying is that g_get_user_cache_dir() and friends cache the path, so that will work only if g_test_xdg_set() is called before any call to g_get_foo_dir() and g_test_xdg_unset() won't change the value returned by those functions... Unless we add in g_setenv() something that clear the cached path? Ideas?
(In reply to comment #28) > [1] : we should do this by > > - Get a reference to the bus > - Wait until the reference count of the obtained GDBusConnection drops to 1 > - then unref the obtained GDBusConnection > > Note that the GDBus test suite, in particular gio/tests/gdbus-connection.c > already does this and it should be straightforward to copy-paste it. Good idea, yes. That would add a dep on libgio so the code can't be in libglib anymore, but I think that's better there anyway. Note that the app using this won't necessary use GDBusConnection, for example tp-glib doesn't. It should just be smart enough to not create a new GDBusConnection if the singleton already does not exists.
Created attachment 211377 [details] [review] Make DBus unit test helpers public API g_test_dbus_up/down/unset() are useful APIs for any application doing DBus unit tests
I've moved the code to gio, removed XDG stuff for now, and squashed commits. I'm investigating the _g_object_wait_for_single_ref() stuff, but I see 2 problems: 1) _g_object_wait_for_single_ref() is in tests/, how do I share it between libgio and tests? 2) Some unit tests still use their connection after the bus is down, to check if doing a call fails as expected for example.
(In reply to comment #36) > I've moved the code to gio, removed XDG stuff for now, and squashed commits. > > I'm investigating the _g_object_wait_for_single_ref() stuff, but I see 2 > problems: > > 1) _g_object_wait_for_single_ref() is in tests/, how do I share it between > libgio and tests? I would just include a private copy of it in gtestdbus.c - it's not really a lot of code anyway. Make sure you copy the newest one, mclasen just switched it yesterday :-) > 2) Some unit tests still use their connection after the bus is down, to check > if doing a call fails as expected for example. Good point. I would introduce G_TEST_DBUS_DOWN_FLAGS_ALLOW_CLOSED_CONNECTIONS in a GTestDBusDownFlags enumeration. On that note, I would also suggest having a GTestDBusUpFlags enumeration for g_test_dbus_up() (empty at first, but allows room for expansion).
Created attachment 211398 [details] [review] Make DBus unit test helpers public API g_test_dbus_up/down/unset() are useful APIs for any application doing DBus unit tests
Created attachment 211399 [details] [review] g_test_dbus_down(): ensure session singleton gets destroyed
hm, the flags idea makes sense too... Here I just splited g_test_dbus_down() and g_test_dbus_stop(), so unit tests that want to verify behaviour after the bus is stopped can first call _stop() then when all is done and ref on the GDBusConnection are droped it can call _down(). Is that good, or do you prefer adding flags for future expansion?
Created attachment 211481 [details] [review] Add a copy of gio/tests/gdbus-tests.c,h to gio/
Created attachment 211482 [details] [review] Add private _g_bus_get_singleton_if_exists() function
Created attachment 211483 [details] [review] g_test_dbus_down(): Ensure the session singleton is destroyed Add g_test_dbus_stop() to stop the session bus without waiting for the singleton to be destroyed.
Created attachment 211484 [details] [review] Make DBus unit test helpers public API g_test_dbus_up/down/unset() are useful APIs for any application doing DBus unit tests
With that changes, gdbus-test-codegen:/gdbus/codegen/object-manager fails because the connection is leaked, did not found the reason. Probably not related, but gdbus-peer:/gdbus/codegen-peer-to-peer is racy: if the server exit before the GDBusConnection is destroyed, it will exit-on-close and that makes the test fail.
Oh and: /* TODO: wait a bit for the bus to come up.. ideally g_test_dbus_up() won't return * until one can connect to the bus but that's not how things work right now */ usleep (500 * 1000); Some unit tests still have that, should I move that down to g_test_dbus_up() or is there a proper fix?
Created attachment 211832 [details] [review] Make DBus unit test helpers public API g_test_dbus_up/down/unset() are useful APIs for any application doing DBus unit tests
Created attachment 211833 [details] [review] Add a copy of gio/tests/gdbus-tests.c,h to gio/
Created attachment 211834 [details] [review] Add private _g_bus_get_singleton_if_exists() function
Created attachment 211835 [details] [review] g_test_dbus_down(): Ensure the session singleton is destroyed Add g_test_dbus_stop() to stop the session bus without waiting for the singleton to be destroyed.
Did cosmetic change. I've ported tp-glib to use this, and it work like a charm: http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=g-test-dbus
Review of attachment 211832 [details] [review]: Looks like the new files are missing here ?
Review of attachment 211833 [details] [review]: ::: gio/gdbus-tests.h @@ +24,3 @@ +#define __TESTS_H__ + +#include <gio/gio.h> Shouldn't include gio.h here, but just the individual headers we need.
Review of attachment 211834 [details] [review]: Would be nice to say something about why this function is needed in the commit message.
Review of attachment 211835 [details] [review]: Looks ok
Review of attachment 211832 [details] [review]: ::: docs/reference/gio/gio-sections.txt @@ +3860,3 @@ +<SECTION> +<FILE>gtestdbus</FILE> +<TITLE>Testing</TITLE> The title should probably a bit more concrete, like 'D-Bus testing utilities' ::: gio/tests/gdbus-sessionbus.h @@ +36,3 @@ +void g_test_dbus_up (const gchar *service_dir); +void g_test_dbus_down (void); +void g_test_dbus_unset (void); The new api needs G_AVAILABLE_IN_2_34 decorations
As mentioned on IRC, hope to get to look at comment 45 and comment 46 next week. A couple of other thoughts - Probably want flags for g_test_dbus_up() - one thought that comes to mind is an ALLOW_ANONYMOUS (this is related to the tests I'm adding in bug 673943) - service_dir is a dbus-daemon implementation specific detail which other message bus implementations may not use. Discuss. Apart from that it looks great so far. Thanks!
(In reply to comment #52) > Review of attachment 211832 [details] [review]: > > Looks like the new files are missing here ? No, git detected it as a rename and display the diff of the file instead of removing the old and adding the new.
Created attachment 212119 [details] [review] gtestdbus: fix includes
(In reply to comment #56) > Review of attachment 211832 [details] [review]: > > ::: docs/reference/gio/gio-sections.txt > @@ +3860,3 @@ > +<SECTION> > +<FILE>gtestdbus</FILE> > +<TITLE>Testing</TITLE> > > The title should probably a bit more concrete, like 'D-Bus testing utilities' Amended the commit with that title. > ::: gio/tests/gdbus-sessionbus.h > @@ +36,3 @@ > +void g_test_dbus_up (const gchar *service_dir); > +void g_test_dbus_down (void); > +void g_test_dbus_unset (void); > > The new api needs G_AVAILABLE_IN_2_34 decorations That does not exist, none of glib/gio API has such annotation... I don't understand what you mean.
(In reply to comment #54) > Review of attachment 211834 [details] [review]: > > Would be nice to say something about why this function is needed in the commit > message. Commit msg amended
Created attachment 212123 [details] [review] Make DBus unit test helpers public API g_test_dbus_up/down/unset() are useful APIs for any application doing DBus unit tests
Created attachment 212124 [details] [review] Add a copy of gio/tests/gdbus-tests.c,h to gio/
Created attachment 212125 [details] [review] Add private _g_bus_get_singleton_if_exists() function This is used by g_test_dbus_down() to ensure the GDBusConnection gets disposed, but not create one if the singleton already got disposed.
Created attachment 212126 [details] [review] g_test_dbus_down(): Ensure the session singleton is destroyed Add g_test_dbus_stop() to stop the session bus without waiting for the singleton to be destroyed.
Created attachment 212127 [details] [review] gtestdbus: fix includes
Created attachment 212128 [details] [review] gtestdbus: APIs works only on unix
Rebased on master, the new gmenumodel unit test also leak the session bus connection, so it timeout. (In reply to comment #57) > - Probably want flags for g_test_dbus_up() - one thought that comes > to mind is an ALLOW_ANONYMOUS (this is related to the tests I'm > adding in bug 673943) The patches on that bug does not modify gdbus-sessionbus.c so I don't understand why gtestdbus would need any change. I'm not against adding flags for future expansion but I don't understand what ALLOW_ANONYMOUS would do. > - service_dir is a dbus-daemon implementation specific detail which > other message bus implementations may not use. Discuss. What would you prefer between: 0) keep API unchanged, if we do other implementation later that param can just be left to NULL. 1) Remove services_dir arg from g_test_dbus_up() and add an extra g_test_dbus_up_with_services_dir(). 2) Remove services_dir arg from g_test_dbus_up() and add an extra g_test_dbus_add_services_dir() that must be called before g_test_dbus_up() if unit test needs it. I guess in this case g_test_dbus_down() would clear the list of services dirs. btw can config file contain multiple services dirs to tell dbus-daemon to lookup in each?
(In reply to comment #57) > - Probably want flags for g_test_dbus_up() Having an empty flags enum (other than NONE) would be a good start. > - service_dir is a dbus-daemon implementation specific detail which > other message bus implementations may not use. Discuss. I disagree: the D-Bus Specification says you can write .service files, so there has to be some way to induce the dbus-daemon implementation to load them. In any case, our tests (I'm mainly thinking of Telepathy, but probably anything else with similar tests) need to provide .service files - so any dbus-daemon implementation that's suitable for our tests must necessarily be able to specify .service files. (We can't always just fork a D-Bus service and have it take a well-known name, because some of our tests involve the behaviour of ListActivatableNames - for instance, the function to list all Telepathy connection managers is expected to look at activatable names, not just currently-owned names.) I can't see why we'd ever want to switch to a dbus-daemon less capable than the reference implementation anyway - we'd only want to switch to another implementation (gdbus-daemon?) if it was strictly better than libdbus' dbus-daemon. (In reply to comment #68) > What would you prefer between: > 0) keep API unchanged, if we do other implementation later that param can just > be left to NULL. IMO this would be OK. > 1) Remove services_dir arg from g_test_dbus_up() and add an extra > g_test_dbus_up_with_services_dir(). I think this should only be a last resort... > 2) Remove services_dir arg from g_test_dbus_up() and add an extra > g_test_dbus_add_services_dir() that must be called before g_test_dbus_up() if > unit test needs it ... because this scales much better if we need more adjustable parameters later. I wonder whether it should even be a GObject? Straw man: GTestDBus *g_test_dbus_new (GTestDBusFlags); void g_test_dbus_add_service_dir (GTestDBus *, const gchar *); void g_test_dbus_up (GTestDBus *); void g_test_dbus_down (GTestDBus *); > btw can config file contain multiple services dirs to > tell dbus-daemon to lookup in each? Yes, at least in the reference implementation (and the D-Bus Specification says the default is ${XDG_DATA_DIRS}/dbus-1/services, so any spec-compliant implementation needs the ability to search more than one directory). One concrete situation where this could be necessary: adding both ${srcdir}/dbus-services and ${builddir}/dbus-services to the search path.
(In reply to comment #60) > (In reply to comment #56) > > > > The new api needs G_AVAILABLE_IN_2_34 decorations > > That does not exist, none of glib/gio API has such annotation... I don't > understand what you mean. Its been added in the meantime.
(In reply to comment #70) > (In reply to comment #60) > > (In reply to comment #56) > > > > > > > The new api needs G_AVAILABLE_IN_2_34 decorations > > > > That does not exist, none of glib/gio API has such annotation... I don't > > understand what you mean. > > Its been added in the meantime. git grep G_AVAILABLE in master gives nothing. Or is that in a branch not merged yet?
oh it is GLIB_ and not G_, ok :)
Created attachment 212141 [details] [review] gtestdbus: annotate new functions with GLIB_AVAILABLE_IN_2_34
Note that this creates build warning: gtestdbus.c:422:5: warning: 'g_test_dbus_stop' is deprecated (declared at gtestdbus.c:382): Not available before 2.34 [-Wdeprecated-declarations] because glib version won't be 3.4 before stable release
(In reply to comment #69) > (In reply to comment #57) > > - Probably want flags for g_test_dbus_up() > > Having an empty flags enum (other than NONE) would be a good start. Yeah. > > - service_dir is a dbus-daemon implementation specific detail which > > other message bus implementations may not use. Discuss. > > I disagree: the D-Bus Specification says you can write .service files, so there > has to be some way to induce the dbus-daemon implementation to load them. Ah, right. I'm confused, sorry - I thought the proposal was that about taking a path to load .conf files from (cf. /etc/dbus-1/system.d/). Since it's not I guess it's fine to have "const gchar *service_dir" as a parameter to g_test_dbus_up(). Sorry for the confusion. > I can't see why we'd ever want to switch to a dbus-daemon less capable than the > reference implementation anyway - we'd only want to switch to another > implementation (gdbus-daemon?) if it was strictly better than libdbus' > dbus-daemon. To be able to run the test suite without having libdbus' dbus-daemon around (useful on non-Linux platforms, I guess). > I wonder whether it should even be a GObject? Straw man: > > GTestDBus *g_test_dbus_new (GTestDBusFlags); > void g_test_dbus_add_service_dir (GTestDBus *, const gchar *); > void g_test_dbus_up (GTestDBus *); > void g_test_dbus_down (GTestDBus *); I like it. A lot more extensible that way. That way we can even do concrete GTestDBusLibDBusDaemon and GTestDBusGDBusDaemon implementations (if we want that level of complexity).
> (In reply to comment #57) > > - Probably want flags for g_test_dbus_up() - one thought that comes > > to mind is an ALLOW_ANONYMOUS (this is related to the tests I'm > > adding in bug 673943) > > The patches on that bug does not modify gdbus-sessionbus.c so I don't > understand why gtestdbus would need any change. I'm not against adding flags > for future expansion but I don't understand what ALLOW_ANONYMOUS would do. This flag would allow peers to connect after successfully authenticating with the ANONYMOUS method - ie. allow anyone to connect .. it's generally not useful a useful flag to have and it's even potentially dangerous especially if you let the bus listen on a TCP socket. Anyway, thinking more about it, I should just rewrite gio/tests/gdbus-auth.c to use peer-to-peer for instead of using a bus daemon as the other peer. That said, I would still like to have flags in g_test_dbus() even if there initially is no flags. Future expansion, all that jazz.
(In reply to comment #75) > To be able to run the test suite without having libdbus' dbus-daemon around > (useful on non-Linux platforms, I guess). The only other implementation I'm aware of is in ndesk-dbus (C# reimplementation) and I don't think it's widely used. As far as I can tell, on Windows, people use libdbus, either with its parallel CMake build system (compiling with MSVC or gcc), or cross-compiled from Linux or Cygwin/MSYS using a mingw32 or mingw-w64 gcc.
(In reply to comment #77) > (In reply to comment #75) > > To be able to run the test suite without having libdbus' dbus-daemon around > > (useful on non-Linux platforms, I guess). > > The only other implementation I'm aware of is in ndesk-dbus (C# > reimplementation) and I don't think it's widely used. As far as I can tell, on > Windows, people use libdbus, either with its parallel CMake build system > (compiling with MSVC or gcc), or cross-compiled from Linux or Cygwin/MSYS using > a mingw32 or mingw-w64 gcc. In case you didn't know, alexl is working on gdbus-daemon for use on Win32 and, I think, OS X too. There's a branch http://git.gnome.org/browse/glib/log/?h=gdbus-daemon which already has support for using his gdbus-daemon in the GLib test suite if the G_DBUS_DAEMON environment variable is set.
Created attachment 212231 [details] [review] Add private _g_bus_get_singleton_if_exists() function This is used by g_test_dbus_down() to ensure the GDBusConnection gets disposed, but not create one if the singleton already got disposed.
Created attachment 212232 [details] [review] Add a private copy of gio/tests/gdbus-tests.c,h to gio/
Created attachment 212233 [details] [review] Add GTestDBus object This is a helper to write unit tests using a private dbus-daemon
Here is a first step at GObject-ify. I would like to keep simple up/down functions hiding the object details because it is what most tests needs, and it would be a pain to port all existing tests to first create the object. Comments are welcome !
(In reply to comment #45) > With that changes, gdbus-test-codegen:/gdbus/codegen/object-manager fails > because the connection is leaked, did not found the reason. > > Probably not related, but gdbus-peer:/gdbus/codegen-peer-to-peer is racy: if > the server exit before the GDBusConnection is destroyed, it will exit-on-close > and that makes the test fail. Found two leaks in the library and a bunch of leaks in the test. Committed patches for all of this, see http://git.gnome.org/browse/glib/commit/?id=3964e708e9a4eb7c23c0aa651ccf8bc7a57cbbf0 http://git.gnome.org/browse/glib/commit/?id=eedb6d8366a9cf4e638fe34ebdca17b387e41ce5 http://git.gnome.org/browse/glib/commit/?id=7f5f47ae15268a1dd96fabd360eb25e712724ac4
(In reply to comment #46) > Oh and: > /* TODO: wait a bit for the bus to come up.. ideally g_test_dbus_up() won't > return > * until one can connect to the bus but that's not how things work right now > */ > usleep (500 * 1000); > > Some unit tests still have that, should I move that down to g_test_dbus_up() or > is there a proper fix? Just try removing it? I mean, Ideally g_test_dbus_up() should _not_ return successfully until it's possible to establish a connection. Maybe we should explicitly verify that a connection can be established ... but note it may break some tests expecting to be the "first" connection, e.g. have a unique name of :1.0 or something.
Created attachment 212275 [details] [review] Add GTestDBus object This is a helper to write unit tests using a private dbus-daemon. session_bus_up/down() are now just wrappers around a GTestDBus singleton.
Current status: * Ported the code to GIOChannel which makes things much easier and closer to portable. * We now have a nice GTestDBus object with methods on it. * Since it is code to be used in unit tests, I simplified the error paths using g_assert_no_error(), instead of warning() and goto out. * I kept gdbus-sessionbus.c,h but totally rewrote it. It is now a trivial wrapper around a GTestDBus singleton. It makes porting all the unit test infinitely easier. * All unit tests pass, except: - contenttype: totally unrelated to this. - /gdbus/codegen-peer-to-peer see comment #45 - gmenumodel leaks the GDBusConnection singleton so it timeout in g_test_dbus_down(). * Removed the usleep hacks and it still works. I think it should be fine since we block reading on dbus-daemon's stdout to get its address, so surely dbus-daemon started listening on it before writing on its stdout. * Methods does not appear in the gtkdoc, it says: ./gio-sections.txt:3879: warning: No declaration found for g_test_dbus_new. I don't understand why.
Created attachment 212286 [details] [review] Add GTestDBus object This is a helper to write unit tests using a private dbus-daemon. session_bus_up/down() are now just wrappers around a GTestDBus singleton.
(In reply to comment #86) > * Methods does not appear in the gtkdoc, it says: ./gio-sections.txt:3879: > warning: No declaration found for g_test_dbus_new. I don't understand why. Fixed, turns out GLIB_AVAILABLE_IN_2_34 can't be on the same line in header file. Thanks gtkdoc!
(In reply to comment #86) > - /gdbus/codegen-peer-to-peer see comment #45 See comment 83 - maybe you just need to rebase to master? > - gmenumodel leaks the GDBusConnection singleton so it timeout in > g_test_dbus_down(). Is anyone working on that? mclasen, desrt? > * Removed the usleep hacks and it still works. I think it should be fine since > we block reading on dbus-daemon's stdout to get its address, so surely > dbus-daemon started listening on it before writing on its stdout. Maybe this requires a recent dbus version? Fedora has an ancient one, 1.4.10, I'll check with that version...
(In reply to comment #89) > (In reply to comment #86) > > - /gdbus/codegen-peer-to-peer see comment #45 > > See comment 83 - maybe you just need to rebase to master? codegen-peer-to-peer is not related to the leaks you fixed, see 2nd paragraph of comment #45. I confirm that after rebasing to master, the bug with gdbus-test-codegen is fixed.
> Do you guys seriously think we should start a new lib for this? It's just about > 400 lines of code, clearly not biggest thing pushed to libglib nowadays. Appears to be ~800 lines of code now...just saying.
Review of attachment 212232 [details] [review]: I think this is a bit overkill - all we need is wait_for_single_ref(), right? I would suggest just copying that macro+function to gio/gtestdbus.c
(In reply to comment #86) > - gmenumodel leaks the GDBusConnection singleton so it timeout in > g_test_dbus_down(). Fixed this, see http://git.gnome.org/browse/glib/commit/?id=9dce93514eb80cd54c2c02db3abf50f2d5a34fb0 I also, ugh, accidentally pushed your patches, sorry... Would you like me to revert or do you think they are ready to go?
(In reply to comment #93) > (In reply to comment #86) > > - gmenumodel leaks the GDBusConnection singleton so it timeout in > > g_test_dbus_down(). > > Fixed this, see > > > http://git.gnome.org/browse/glib/commit/?id=9dce93514eb80cd54c2c02db3abf50f2d5a34fb0 > > I also, ugh, accidentally pushed your patches, sorry... Would you like me to > revert or do you think they are ready to go? Forgot to mention: 'make check' seems to work for me so... I'd vote for keeping them in... but it's up to you. Sorry again.
(In reply to comment #94) > > I also, ugh, accidentally pushed your patches, sorry... Would you like me to > > revert or do you think they are ready to go? > > Forgot to mention: 'make check' seems to work for me so... I'd vote for keeping > them in... but it's up to you. Sorry again. Didn't find you on IRC so reverted them for now.
Review of attachment 212231 [details] [review]: Sounds like a good idea to me. ::: gio/gdbusconnection.c @@ +6771,3 @@ +/* Called in any user thread, without holding locks. */ +GDBusConnection * I'd just say "May be called from any thread. Must not hold message_bus_lock."
Review of attachment 212286 [details] [review]: Looks good to me, just details here I think. ::: gio/gioenums.h @@ +1655,3 @@ + G_TEST_DBUS_FOO = (1 << 0), +} GTestDBusFlags; + * Since: 2.34 There's a way to do this without the DBUS_FOO - just add /*< flags >*/ after enum and before { (see GTlsDatabaseVerifyFlags just above) ::: gio/gtestdbus.c @@ +241,3 @@ + * Since: 2.34 + */ +struct _GTestDBusClass { No point in documenting the class struct since it's not public. ::: gio/gtestdbus.h @@ +48,3 @@ +#define G_TEST_DBUS_GET_CLASS(obj) \ + (G_TYPE_INSTANCE_GET_CLASS ((obj), G_TYPE_TEST_DBUS, \ + GTestDBusClass)) Since we are not exposing the Class struct, we don't need any of the *CLASS* macros.
Created attachment 212317 [details] [review] Add GTestDBus object This is a helper to write unit tests using a private dbus-daemon.
Created attachment 212318 [details] [review] Use GTestDBus in all GDBus unit tests To make port easier, this rewrites dbus-sessionbus.c using a GTestDBus singleton internally.
(In reply to comment #96) > Review of attachment 212231 [details] [review]: > +/* Called in any user thread, without holding locks. */ > +GDBusConnection * > > I'd just say "May be called from any thread. Must not hold message_bus_lock." I just copied the comment on get_uninitialized_connection() above. I've noticed that I've removed all usages of _g_object_wait_for_single_ref() so I could move (instead of copy) that part directly to gtestdbus.c. However I do believe (but could be wrong, I'm not threading expert) that code is not thread-safe. What I noticed: 1) reading ->ref_count without g_atomic_int_get() is unsafe in case another thread is (un)reffing it at the same time. 2) if another thread unref just between the g_object_unref(); g_main_loop_run(); it means that on_wait_single_ref_timeout could be called before starting the mainloop and then the g_main_loop_run() afterward will never stop (well, on timeout). I think the only way to fix this is to unref in an idle callback to be sure the mainloop is running. So my code is different and I think is easier to understand, but please tell me if I'm wrong :-) Voilà for me the branch is ready to merge: http://cgit.collabora.com/git/user/xclaesse/glib.git/log/?h=dbus-test
Created attachment 212323 [details] [review] Call session_bus_down() (In reply to comment #99) > Created an attachment (id=212318) [details] [review] > Use GTestDBus in all GDBus unit tests > > To make port easier, this rewrites dbus-sessionbus.c using a > GTestDBus singleton internally. Some of the tests don't call session_bus_down() and I'd argue they should - the attached patch does this except for gio/tests/gdbus-proxy-threads.c which is a bit... harder. Anyway, with (something like) this patch on top (ie. squashed into your patch), OK to commit. Thanks!
Comment on attachment 212317 [details] [review] Add GTestDBus object Looks good to me. Thanks.
Comment on attachment 212231 [details] [review] Add private _g_bus_get_singleton_if_exists() function Good to commit with the small change requested in comment 96
Comment on attachment 212318 [details] [review] Use GTestDBus in all GDBus unit tests Good to commit with the patch from comment 101 squashed in (or something similar).
Squashed your patch and fixed comment #96. Branch merged \o/ Thanks for your help!