GNOME Bugzilla – Bug 690830
eds: "make *.gdb" tests retain dirty state after first run
Last modified: 2014-02-17 00:03:13 UTC
For repeatability, it's critical that our tests start from the same (blank) state every time they're run. This seems to be true for a regular "make check", but when debugging the tests individually with "make <test-name>.gdb", the EDS backend or test library can retain EContacts which survived the last run. This can obviously change test results in some cases, making them unreliable (and thus useless). For example, I ran the following command (from tests/eds): G_MESSAGES_DEBUG=all make set-emails.gdb Without quitting gdb in between, I ran the program three times, getting these messages (among many more debug lines and slightly different behavior each run): run 1: (/home/treitter/collabora/folks/tests/eds/.libs/lt-set-emails:11889): eds-DEBUG: edsf-persona.vala:935: Creating new Edsf.Persona with IID 'test:pas-id-50DDE69A00000000' run 2: (/home/treitter/collabora/folks/tests/eds/.libs/lt-set-emails:11903): eds-DEBUG: edsf-persona.vala:935: Creating new Edsf.Persona with IID 'test:pas-id-50DDE69A00000000' (/home/treitter/collabora/folks/tests/eds/.libs/lt-set-emails:11903): eds-DEBUG: edsf-persona.vala:935: Creating new Edsf.Persona with IID 'test:pas-id-50DDE6A200000001' run 3: (/home/treitter/collabora/folks/tests/eds/.libs/lt-set-emails:12020): eds-DEBUG: edsf-persona.vala:935: Creating new Edsf.Persona with IID 'test:pas-id-50DDE6A200000001' (/home/treitter/collabora/folks/tests/eds/.libs/lt-set-emails:12020): eds-DEBUG: edsf-persona.vala:935: Creating new Edsf.Persona with IID 'test:pas-id-50DDE69A00000000' (/home/treitter/collabora/folks/tests/eds/.libs/lt-set-emails:12020): eds-DEBUG: edsf-persona.vala:935: Creating new Edsf.Persona with IID 'test:pas-id-50DDE80100000002' If I quit gdb between runs, the state seems to be reset as expected (though the EContact ID is still incremented some amount, which is a little worrying, since it's at least a superficial state correlation and, at worst, a separate issue we'll also need to deal with). I'm not sure if this is just a problem for the EDS backend/EDS test backend library or our test suite in general, but there's certainly potential for it to be a problem with other backends independently.
I guess this is because check.mk runs gdb _inside_ the $(TESTS_ENVIRONMENT), which for the EDS tests is a shell script (http://git.gnome.org/browse/folks/tree/tests/tools/with-session-bus-eds.sh) which creates some temporary directories then runs the given program (in this case, gdb with some arguments). It only cleans up those temporary directories once the program (i.e. gdb) has exited. I can’t immediately think of a clean way to fix this apart from moving the code from the shell script inside the test executables themselves. That wouldn’t be too bad a fix.
Created attachment 238800 [details] [review] Replace with-session-bus*.sh with Vala code If we embed this stuff in the test binaries themselves, then it should become safe to run "./tests/eds/add-persona" without having to worry about whether it will trample over your real address book, which makes debugging easier. --- > I can’t immediately think of a clean way to fix this apart from moving > the code from the shell script inside the test executables themselves. Here you go! This requires my remaining unreviewed patches from Bug #695381. This patch reduces the need to have "make foo.gdb" at all - each test directory only sets a couple of environment variables now. It would be reasonably straightforward to avoid "make foo.gdb" altogether, by hard-coding the ${abs_top_srcdir} and ${abs_top_builddir} into the test executables instead of using the TESTS_ENVIRONMENT - then you could just "gdb tests/eds/add-persona". However, it would be nice to be able to use the same executables as optionally-installed black-box tests for a system installation of Folks (see recent discussion on desktop-devel). I think the best we can do is to require one environment variable, as a switch between "look for data in ${abs_top_srcdir} and/or ${abs_top_builddir}, and libraries in the builddir" and "look for data in ${datadir}/folks-tests and/or ${libdir}/folks-tests, and libraries in the libdir" - then hard-code those paths into the executable via config.h and build-conf.vapi.
Review of attachment 238800 [details] [review]: ::: tests/lib/eds/test-case.vala @@ +61,3 @@ + + try + { Some comments describing what’s going on for each region of this block would be useful, since each line of shell script has turned into ~10 lines of Vala. @@ +90,3 @@ + } + + message ("starting Evolution processes..."); Can these be debug()s rather than message()s please, to reduce log spew. @@ +97,3 @@ + { "%s/evolution-addressbook-factory".printf (libexecdir), + "--wait-for-client" }, + null, flags, null, null); Would it be a good idea to keep around the process’ PIDs and explicitly kill them on private_bus_down() to reduce the risk of buggy EDS processes hanging around from old test runs? @@ +100,3 @@ + + /* FIXME: waiting for the D-Bus name would be much better, but this + * is what eds.sh did */ Any chance of fixing this? Having sleeps in the tests is horrible. I know other tests are very guilty of this, but let’s not make the problem worse. ::: tests/lib/test-case.vala @@ +38,3 @@ + { + if (this._transient_dir == null) + this._transient_dir = create_transient_dir (); Please use ‘this.’ for local method calls. @@ +44,3 @@ + } + + public virtual string create_transient_dir () This method could do with some brief comments for each region of code, as well as some API documentation for the method itself. @@ +53,3 @@ + tmp, GLib.strerror (GLib.errno)); + + message ("setting up in transient directory %s", transient); Can debug() be used instead of message() please. @@ +70,3 @@ + + /* This would be more realistic if it was .local/share, but this + * is consistent with what eds.sh used to do... */ Please change it to use .local/share instead then. @@ +85,3 @@ this._suite = new GLib.TestSuite (name); + this._transient_dir = create_transient_dir (); ‘this.create_transient_dir()’. @@ +95,3 @@ public GLib.TestDBus? test_dbus = null; + public virtual bool private_bus_allows_session_services This property could do with some API documentation. @@ +115,3 @@ + /* FIXME: indiscriminately allowing any session service seems + * a bad idea, but it's how eds.sh and tracker.sh have always + * worked, so let's go with that for now... */ What would you suggest instead? Please don’t take eds.sh and tracker.sh as ground truth. @@ +165,3 @@ + unowned string dir = (!) this._transient_dir; + Folks.TestUtils.remove_directory_recursively (dir); + /* if at first you don't succeed... (again, this matches eds.sh) */ I suspect this was due to a race between the EDS processes dying (and thus closing their open files in the directory) and attempting to remove the directory, which would fail on open files. Explicitly killing the EDS processes might mean this hack can be removed.
(In reply to comment #3) > Some comments describing what’s going on for each region of this block would be > useful, since each line of shell script has turned into ~10 lines of Vala. I'll append this to Bug #695381. > Can these be debug()s rather than message()s please, to reduce log spew. Sure, I'll do that on the other bug. > Would it be a good idea to keep around the process’ PIDs and explicitly kill > them on private_bus_down() to reduce the risk of buggy EDS processes hanging > around from old test runs? I'm not sure; they're D-Bus services, so they will die with the bus anyway (as will anything started by activation. (Also, getting explicit kill() requires us to use Posix and disambiguate a bunch of function names - do'able, but awkward.) > + /* FIXME: waiting for the D-Bus name would be much better, but this > + * is what eds.sh did */ > > Any chance of fixing this? Having sleeps in the tests is horrible. I know other > tests are very guilty of this, but let’s not make the problem worse. I'd prefer to make this a separate commit, and keep this one as essentially "rewrite shell scripts in Vala"; but yes I'll give it a go. However, what I'm really trying to do here is set up a performance test for the performance bugs I've been working on; so I don't have infinite time to clean up non-regressions :-) > Please use ‘this.’ for local method calls. Noted, I'll revise that. > This method could do with some brief comments for each region of code, as well > as some API documentation for the method itself. Done on Bug #695381 (I did documentation for the finished state rather than trying to document intermediate states). > Please change it to use .local/share instead then. I'll do that when I've grep'd for any explicit references to .local. > This property could do with some API documentation. Done on #695381. > + /* FIXME: indiscriminately allowing any session service seems > + * a bad idea, but it's how eds.sh and tracker.sh have always > + * worked, so let's go with that for now... */ > > What would you suggest instead? Please don’t take eds.sh and tracker.sh as > ground truth. Well, they work, and the Tracker tests (at least) rely on us having done this. Possibilities include: 1. decide that allowing every session service is "probably" not going to trash the user's files, and remove the FIXME 2. set more variables (HOME, G_HOME, XDG_RUNTIME_DIR, unset XDG_DESKTOP_DIR and friends?) to increase the amount of insulation between the tests and the user's real setup 3. only allow services from ${builddir}/tests/data/dbus-1-services (or something), and symlink all necessary services' service files into that directory 4. start any necessary services explicitly (e-d-s does this now, Tracker could do it too?) and don't allow any service-activation 5. 2 and 3 together
Patches moved to Bug #695381 - I have one big patchset for both (it seemed better to document the end state after I'd done everything I wanted to for now, rather than to document intermediate states).
(In reply to comment #4) > > Would it be a good idea to keep around the process’ PIDs and explicitly kill > > them on private_bus_down() to reduce the risk of buggy EDS processes hanging > > around from old test runs? > > I'm not sure; they're D-Bus services, so they will die with the bus anyway (as > will anything started by activation. > > (Also, getting explicit kill() requires us to use Posix and disambiguate a > bunch of function names - do'able, but awkward.) Up to you then. > > + /* FIXME: indiscriminately allowing any session service seems > > + * a bad idea, but it's how eds.sh and tracker.sh have always > > + * worked, so let's go with that for now... */ > > > > What would you suggest instead? Please don’t take eds.sh and tracker.sh as > > ground truth. > > Well, they work, and the Tracker tests (at least) rely on us having done this. > > Possibilities include: > > 1. decide that allowing every session service is "probably" not going to trash > the user's files, and remove the FIXME Eww. > 2. set more variables (HOME, G_HOME, XDG_RUNTIME_DIR, unset XDG_DESKTOP_DIR and > friends?) to increase the amount of insulation between the tests and the user's > real setup This seems like a good idea in any case. > 3. only allow services from ${builddir}/tests/data/dbus-1-services (or > something), and symlink all necessary services' service files into that > directory > > 4. start any necessary services explicitly (e-d-s does this now, Tracker could > do it too?) and don't allow any service-activation I think I prefer 3 over 4, since it’s how folks would be used in practice, and insulates us from changes to the binary names (etc.) of other services, such as happened with EDS a few cycles ago. > 5. 2 and 3 together So, this one.
Assigning to myself, more yaks need shaving before this can be closed... * libebook warnings need to be made non-fatal * things using libdbus (Telepathy, libsocialweb) need to turn off exit-on-disconnect * the one Tracker test I tried converting fails because it gets an unexpected individuals-changed signal during teardown - that signal will need disconnecting * the libsocialweb tests end up with refs to the GDBusConnection that are never released, which means waiting until GLib gives up on waiting for its last-unref and times out * and more! I have patches for some of these, but not all yet :-(
Created attachment 239281 [details] [review] [01/13] Add infrastructure to run tests under GTestDBus For now, all library TestCase subclasses except the one for key-files override this back to "do nothing" and rely on being run under with-session-bus.sh, because I haven't checked whether they survive having the D-Bus session bus disconnected from under them. --- This initial series of 13 patches does not close this bug, but it's a start...
Created attachment 239282 [details] [review] [02/13] LibsocialwebTest: don't try to keep ownership of name through tear_down() This is a step towards replacing with-session-bus*.sh with GTestDBus, which cleans up our D-Bus connection while the test is still running. If we do that without this change, then the NameLost callback is triggered when we get disconnected from D-Bus, causing an assertion failure. This also should reduce circular references, by making sure closures that contain "this" get cleaned up.
Created attachment 239283 [details] [review] [03/13] LibsocialwebTest.Backend: clean up object registrations in tear_down()
Created attachment 239284 [details] [review] [04/13] Run libsocialweb tests under GTestDBus instead of with-session-bus.sh
Created attachment 239285 [details] [review] [05/13] Run Telepathy test cases under GTestDBus instead of with-session-bus.sh
Created attachment 239286 [details] [review] [06/13] In tests that do not rely on with-session-bus*.sh, use a transient $HOME
Created attachment 239287 [details] [review] [07/13] Add infrastructure to run helper test binaries I've included basic infrastructure to run them from an installed path instead of the builddir, because there seems to be considerable interest in that at the moment. --- Not amazingly closely-related to this bug, I will admit, but the next patch in the series does the same as this for data files. I could rebase this out if required.
Created attachment 239288 [details] [review] [08/13] tools: use the right locale directory I happened to notice that they're misusing $(pkgdatadir) - $(pkgdatadir) is $(datadir)/$(PACKAGE), i.e. typically /usr/share/folks, but localization is installed to $(localedir), typically /usr/share/locale. This resulted in the tools looking in /opt/gnome-3.8/share/folks/locale in my installation, instead of the correct /opt/gnome-3.8/share/locale.
Created attachment 239289 [details] [review] [09/13] Simplify how we get PACKAGE_DATADIR DATA_DIR and PACKAGE_DATADIR appear to have been intended to both be ${prefix}/share/folks, but due to a typo (sharedir vs. shareddir) DATA_DIR was actually defined to an empty string. Neither is actually used anywhere at the moment.
Created attachment 239290 [details] [review] [10/13] Add infrastructure for finding test data files, and use it for avatars Again, I've speculatively added support for a FOLKS_TESTS_INSTALLED variable, because one day I'd like "make installcheck" to work.
Created attachment 239291 [details] [review] [11/13] EdsTest.Backend: clear up more thoroughly after tests
Created attachment 239292 [details] [review] [12/13] Add DisconnectionQueue, which disconnects signals semi-automatically
Created attachment 239293 [details] [review] [13/13] libsocialweb tests: disconnect all signals
Next steps in shaving this yak: * try to convert the e-d-s tests to GDBus, which requires: * unprepare backends in tear_down() so we aren't holding the private GDBus connection open, which requires: * make unprepare() safer (at the moment it has the remove-while-iterating anti-pattern) * disconnect all the signals in the e-d-s tests so that when either the private D-Bus connection goes down or the backend is unprepared, we don't trip assertion failures because we weren't expecting to get another signal
Created attachment 239295 [details] [review] [untested] Eds.Backend: use a safe remove-while-iterating pattern Otherwise, if we unprepare with more than 0 address books, we'll remove from a hash map while iterating over it, resulting in a crash. --- This is work in progress - it might not actually work, I won't know until I get the e-d-s tests to pass :-)
Created attachment 239296 [details] [review] WiP: unprepare all backends in tear_down() --- This is basically what I'm aiming for, but it doesn't work yet, because unpreparing trips lots of signals that the tests aren't expecting.
Review of attachment 239281 [details] [review]: Looks good. ::: configure.ac @@ +190,3 @@ PKG_CHECK_MODULES([GMODULE], [gmodule-no-export-2.0]) PKG_CHECK_MODULES([GIO], [gio-2.0 >= $GLIB_REQUIRED]) +PKG_CHECK_MODULES([DBUS_GLIB], [dbus-glib-1 dbus-1]) This makes me sad. We were trying to get rid of these dependencies! I guess it’s unavoidable, though we should add a FIXME to remove it once our dependencies get rid of it.
Review of attachment 239282 [details] [review]: ++
Review of attachment 239283 [details] [review]: Looks good. ::: tests/lib/libsocialweb/backend.vala @@ +257,3 @@ + private DBusConnection _conn; + private uint[] _registrations; + private HashTable<LibsocialwebServiceTest, ulong> _register_child_signals; These could do with a brief comment.
Review of attachment 239284 [details] [review]: ++
Review of attachment 239285 [details] [review]: ++
Review of attachment 239286 [details] [review]: ++ ::: tests/lib/test-case.vala @@ +43,3 @@ this._suite = new GLib.TestSuite (name); + this._transient_dir = create_transient_dir (); ‘this.create_transient_dir ()’.
Review of attachment 239287 [details] [review]: ++ ::: tests/lib/test-utils.vala @@ +146,3 @@ + * + * @param argv Arguments for the executable. The first is the path of + * the executable itself, relative to ${builddir}/tests. Missing ‘@param capture_stdout …’
Review of attachment 239288 [details] [review]: This might have an effect on packagers. Please add a note to NEWS (in the ‘Major changes’ section) noting that this has been fixed.
Review of attachment 239289 [details] [review]: Sigh.
Review of attachment 239290 [details] [review]: ++, with the question below resolved. ::: tests/lib/test-utils.vala @@ +198,3 @@ + + /** + * Return the path to a test file that is distributed in the source tarball Don’t you mean “built from the source tarball”?
Review of attachment 239291 [details] [review]: ++ with the minor change below. ::: tests/lib/eds/backend.vala @@ +346,3 @@ this._addressbook = null; + this._source_registry = null; + this._addressbook = null; this._addressbook is nullified just above here.
Review of attachment 239292 [details] [review]: Could do with a bit of documentation (just to say why it’s necessary, really), but looks good other than that.
Review of attachment 239293 [details] [review]: Looks good, although I’m a little dubious as to why the set of signals to be disconnected needs to be calculated at runtime rather than compile time? ::: tests/libsocialweb/aggregation.vala @@ +105,3 @@ /* Populate mysocialnetwork1 */ + signals.push (mysocialnetwork1, + mysocialnetwork1.OpenViewCalled.connect((query, p, path) => Space before the ‘(’ (and several times below; sorry to be picky).
Review of attachment 239295 [details] [review]: A bit icky, but there’s not much between this approach and the “bung everything in a stores_to_remove set” approach.
Review of attachment 239296 [details] [review]: Looks good so far. ::: tests/lib/test-case.vala @@ +291,3 @@ + + while (result == null) + MainContext.default ().iteration (true); Would be worthwhile adding a FIXME to tidy this all up once bug #604827 is fixed and we can make tear_down() async.
(In reply to comment #24) > +PKG_CHECK_MODULES([DBUS_GLIB], [dbus-glib-1 dbus-1]) > > This makes me sad. We were trying to get rid of these dependencies! I guess > it’s unavoidable, though we should add a FIXME to remove it once our > dependencies get rid of it. Yeah, unfortunately, regression tests that want clean teardown need to be aware of this sort of implementation detail of their dependencies. I opened Bug #696177. (In reply to comment #33) > + * Return the path to a test file that is distributed in the source tarball > > Don’t you mean “built from the source tarball”? No, I mean what I said. This comment documents get_source_test_data(), which looks in the srcdir for source files that were unpacked from the source tarball, whereas get_built_test_data() looks in the builddir for built files that were generated by "make". get_source_test_data() is used to pick up tests/data/avatar-01.jpg, which is a source file, committed to git and distributed in source tarballs. get_built_test_data() isn't used anywhere yet (I added it for symmetry), but the intention is that it would be used for a file generated by a Makefile. For instance, if you added a tests/data/avatar-01.png which is generated from avatar-01.jpg at "make" time using ImageMagick or something, that would be get_built_test_data("data/avatar-01.png"). (In reply to comment #36) > Looks good, although I’m a little dubious as to why the set of signals to be > disconnected needs to be calculated at runtime rather than compile time? Taking the aggregation test as an example, every time we open a view (OpenViewCalled emitted), we connect to that view's StartCalled signal, with a lambda as the callback. When we clean up, we need to disconnect from StartCalled once per view. In C I'd consider using g_signal_handlers_disconnect_by_func(), but the Vala lambda doesn't have a user-available name (that I know of) so we can't do that, and in any case we'd still have to save a list of views so we can disconnect the signal from each of them in turn. Perhaps this test implicitly guarantees that OpenViewCalled is emitted exactly once per service (or something), in which case we'd be able to save the view and ID in a specific variable and then call view1.disconnect (id1) and view2.disconnect (id2) at the end... but I didn't want to assume too much about the semantics of specific tests, and I'd rather just know that every signal we connected is going to get disconnected eventually, so we don't leak the TestCase (and therefore everything it refs). Also, saving pairs of object and signal ID seems like the sort of boilerplate that obscures the test code. I could easily have disconnected aggregator.individuals_changed_detailed by saving its signal ID, but when I have the infrastructure already (for the more complicated cases), I thought I might as well use it and save a few lines of boilerplate. > Space before the ‘(’ (and several times below; sorry to be picky). I'll try to remember that you want whitespace fixed on lines I've touched, even when it was already like that :-) (In reply to comment #38) > A bit icky, but there’s not much between this approach and the “bung everything > in a stores_to_remove set” approach. For my reference when I get to the point when this is mergeable: are you saying "I'd prefer to iterate and put keys in a temporary list, then iterate that list and do the actual removals" or "the iter.remove() pattern is OK"? I realise it's probably a question of which is less icky.
Comment on attachment 239281 [details] [review] [01/13] Add infrastructure to run tests under GTestDBus 49bc027
Comment on attachment 239282 [details] [review] [02/13] LibsocialwebTest: don't try to keep ownership of name through tear_down() aa81aee
Comment on attachment 239283 [details] [review] [03/13] LibsocialwebTest.Backend: clean up object registrations in tear_down() cf5db86
Comment on attachment 239284 [details] [review] [04/13] Run libsocialweb tests under GTestDBus instead of with-session-bus.sh 40abe98
Comment on attachment 239285 [details] [review] [05/13] Run Telepathy test cases under GTestDBus instead of with-session-bus.sh 165e6ff
Comment on attachment 239286 [details] [review] [06/13] In tests that do not rely on with-session-bus*.sh, use a transient $HOME 67c7ae1
Comment on attachment 239287 [details] [review] [07/13] Add infrastructure to run helper test binaries 9bcb7f7
Comment on attachment 239288 [details] [review] [08/13] tools: use the right locale directory cc0f932, with 08a5276 for the NEWS update
(In reply to comment #40) > (In reply to comment #24) > > +PKG_CHECK_MODULES([DBUS_GLIB], [dbus-glib-1 dbus-1]) > > > > This makes me sad. We were trying to get rid of these dependencies! I guess > > it’s unavoidable, though we should add a FIXME to remove it once our > > dependencies get rid of it. > > Yeah, unfortunately, regression tests that want clean teardown need to be aware > of this sort of implementation detail of their dependencies. I opened Bug > #696177. > > (In reply to comment #33) > > + * Return the path to a test file that is distributed in the source tarball > > > > Don’t you mean “built from the source tarball”? > > No, I mean what I said. This comment documents get_source_test_data(), which > looks in the srcdir for source files that were unpacked from the source > tarball, whereas get_built_test_data() looks in the builddir for built files > that were generated by "make". My comment referred to the documentation for get_built_test_data(), which appears to be exactly the same as that for get_source_test_data(). I realise Splinter hasn’t put enough context around my comment to make this obvious though (sorry!). > (In reply to comment #36) > > Looks good, although I’m a little dubious as to why the set of signals to be > > disconnected needs to be calculated at runtime rather than compile time? > > Taking the aggregation test as an example, every time we open a view > (OpenViewCalled emitted), we connect to that view's StartCalled signal, with a > lambda as the callback. When we clean up, we need to disconnect from > StartCalled once per view. *snip* A very reasonable explanation. I was assuming there are a fixed number of signal connections per test, but this more general solution is definitely welcome. > I'll try to remember that you want whitespace fixed on lines I've touched, even > when it was already like that :-) Please! > (In reply to comment #38) > For my reference when I get to the point when this is mergeable: are you saying > "I'd prefer to iterate and put keys in a temporary list, then iterate that list > and do the actual removals" or "the iter.remove() pattern is OK"? I realise > it's probably a question of which is less icky. I think I prefer the “iterate and put keys in a temporary list” approach, but I don’t mind enough to require you to rewrite your patch. Both approaches are a bit icky, though passing the iter to _remove_address_book() seems (ever so) slightly more icky because it spreads the code out across more of the file. Whatever you prefer.
Comment on attachment 239289 [details] [review] [09/13] Simplify how we get PACKAGE_DATADIR 1f74d5d
Comment on attachment 239290 [details] [review] [10/13] Add infrastructure for finding test data files, and use it for avatars 850aff1
Comment on attachment 239291 [details] [review] [11/13] EdsTest.Backend: clear up more thoroughly after tests ae98b4f
(In reply to comment #49) > My comment referred to the documentation for get_built_test_data(), which > appears to be exactly the same as that for get_source_test_data(). Ah yes, sorry.
Created attachment 239405 [details] [review] Fix comment for get_built_test_data()
Review of attachment 239405 [details] [review]: ++
Comment on attachment 239405 [details] [review] Fix comment for get_built_test_data() 2f0521a5
Created attachment 239862 [details] [review] Set up tests' backend paths in Vala code One less thing in TESTS_ENVIRONMENT.
Created attachment 239863 [details] [review] Remove a remnant of the avatar coming from the environment
Created attachment 239886 [details] [review] Set up tests' backend paths in Vala code One less thing in TESTS_ENVIRONMENT. --- v2: rebased on 9064f78a3d, and with the backends listed in alphabetical order.
Review of attachment 239863 [details] [review]: ++
Review of attachment 239886 [details] [review]: ++
Comment on attachment 239863 [details] [review] Remove a remnant of the avatar coming from the environment 1e2fb274c
Comment on attachment 239886 [details] [review] Set up tests' backend paths in Vala code 13113b2f
(In reply to comment #35) > Could do with a bit of documentation (just to say why it’s necessary, really), > but looks good other than that. Sorry, I'd half-forgotten about this bug - I haven't touched Folks for a while (got diverted to unrelated work), and in fact I can't seem to build git master at the moment. I think out-of-tree may have regressed, I'll try in-tree. (In reply to comment #36) > Looks good, although I’m a little dubious as to why the set of signals to be > disconnected needs to be calculated at runtime rather than compile time? Addressed by comments above, I think. Is this a-c_n now? The other "reviewed" patches on this bug are untested, or never got beyond work-in-progress. I'm afraid there's more work to be done here before the bug can actually be closed - there's an entire herd of unshaven yaks.
Comment on attachment 239293 [details] [review] [13/13] libsocialweb tests: disconnect all signals (In reply to comment #64) > Addressed by comments above, I think. Is this a-c_n now? It is, modulo the whitespace fixes from comment #36.
Simon, are attachment#12 [details] and attachment#13 [details] still relevant? Should they be merged and this bug closed?
(In reply to comment #66) > Simon, are attachment#12 [details] and attachment#13 [details] still relevant? Should they be > merged As far as I can remember, they're still relevant and desirable. If you have a working Folks build environment and those patches don't make it regress, you're welcome to apply them; but last time I looked at Folks, lots of tests failed for me (possibly because I was trying to use an out-of-tree build), so I'd rather not spend too much time yak-shaving it. (In reply to comment #23) > WiP: unprepare all backends in tear_down() This bug isn't really finished until something like this can be applied, see Comment #21.
(In reply to comment #67) > (In reply to comment #66) > > Simon, are attachment#12 [details] and attachment#13 [details] still relevant? Should they be > > merged > > As far as I can remember, they're still relevant and desirable. If you have a > working Folks build environment and those patches don't make it regress, you're > welcome to apply them; but last time I looked at Folks, lots of tests failed > for me (possibly because I was trying to use an out-of-tree build), so I'd > rather not spend too much time yak-shaving it. This is hopefully fixed with my patch for bug#679826. EDS and Tracker tests were failing spectacularly (and inconsistently) when run in parallel and this patch forces them to be serialized.
(In reply to comment #68) > This is hopefully fixed with my patch for bug#679826. EDS and Tracker tests > were failing spectacularly (and inconsistently) when run in parallel and this > patch forces them to be serialized. Do these patches now work for you, Travis?
(In reply to comment #69) > (In reply to comment #68) > > This is hopefully fixed with my patch for bug#679826. EDS and Tracker tests > > were failing spectacularly (and inconsistently) when run in parallel and this > > patch forces them to be serialized. > > Do these patches now work for you, Travis? I applied attachment #239292 [details], had to refresh attachment #239293 [details] manually and applied attachment #239295 [details]. Everything builds and the tests pass. Try it out with this branch: http://cgit.collabora.com/git/user/treitter/folks.git/log/?h=bgo690830-clean-eds-test-state The tests run fine when run in aggregate via "make check" but if I run "make set-emails.gdb" and run it more than once, it's clear that state is being retained (the EDS contact IDs keep counting up). For some reason, it runs fine the first and second run but hits a timeout failure for subsequent runs.
(In reply to comment #70) > The tests run fine when run in aggregate via "make check" but if I run "make > set-emails.gdb" and run it more than once, it's clear that state is being > retained (the EDS contact IDs keep counting up). For some reason, it runs fine > the first and second run but hits a timeout failure for subsequent runs. Sounds like those patches are good then. They work fine according to my testing too, so I’ve pushed them. The bug can remain open for getting the *.gdb tests working nicely. (Patches coming!) commit ff3ebb6595730e7c1e7633369da53dfef3f161fa Author: Simon McVittie <simon.mcvittie@collabora.co.uk> Date: Tue Mar 19 15:22:23 2013 +0000 Eds.Backend: use a safe remove-while-iterating pattern Otherwise, if we unprepare with more than 0 address books, we'll remove from a hash map while iterating over it, resulting in a crash. backends/eds/eds-backend.vala | 42 +++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-) commit a9e15326b07b0c284d50013e220717b375521a50 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Thu Oct 10 16:23:32 2013 -0700 libsocialweb tests: disconnect all signals Patch by Simon McVittie tests/libsocialweb/aggregation.vala | 28 ++++++++++++++++++---------- tests/libsocialweb/dummy-lsw.vala | 12 ++++++++---- 2 files changed, 26 insertions(+), 14 deletions(-)
Aaand the third patch: commit 956c1d2aea563e6ef8cacafbb1edebb8b8ec7d7f Author: Simon McVittie <simon.mcvittie@collabora.co.uk> Date: Tue Mar 19 17:59:14 2013 +0000 Add DisconnectionQueue, which disconnects signals semi-automatically tests/lib/Makefile.am | 1 + tests/lib/disconnection-queue.vala | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+)
Created attachment 260186 [details] [review] eds: Move with-session-bus-eds.sh into test-case.vala Eliminate some more of the shell scripting which plagues our test cases. This wipes up to 2s off the startup time for the EDS tests (which was previously hard-coded to spend waiting for the EDS processes to start up; now the processes are started through D-Bus service activation, so we only wait as long as necessary).
Created attachment 260187 [details] [review] tests: Remove redundant execute-test.sh script It was used to redirect verbose logging output to a log file, but automake 1.11 does that automatically now with its parallel test framework (which folks has enabled).
Created attachment 260188 [details] [review] tracker: Move with-session-bus-tracker.sh into test-case.vala Eliminate the last bits of shell scripting from the test suites. Instead of starting and stopping the Tracker services through tracker-control, the Tracker services are now started through D-Bus activation, and die when the mock D-Bus bus is destroyed.
Created attachment 260189 [details] [review] tracker: Don’t warn if prepare() fails due to the bus disappearing This is not an uncommon occurrence during the unit tests. It doesn’t deserve a warning.
These patches eliminate the remaining Bash scripts in the test suites. They’re partially tested, but depend on some changes from bug #712274, and need further testing. I think there might be some issues with Backend instances being leaked, which prevent the TestDBus from shutting down cleanly. I need to investigate further. In the meantime, the patches can be reviewed, since the fixes will be in the backends not the test harnesses.
Review of attachment 260186 [details] [review]: Looks good. Please apply when bug #712274 has been fixed and all its dependencies widely available.
Review of attachment 260187 [details] [review]: Looks good. Please apply when bug #712274 has been fixed and all its dependencies widely available.
Review of attachment 260188 [details] [review]: Looks good. Please apply when bug #712274 has been fixed and all its dependencies widely available.
Review of attachment 260189 [details] [review]: Looks good. Please apply when bug #712274 has been fixed and all its dependencies widely available.
Fixed. There are a few test failures in the EDS and Tracker test suites, but those are due to races in killing the dbus-daemon and can be fixed separately. commit bdd4d235def0c2662e125aaa4ed4b77d2dad14e6 Author: Philip Withnall <philip@tecnocode.co.uk> Date: Tue Nov 19 00:35:49 2013 +0000 tracker: Don’t warn if prepare() fails due to the bus disappearing This is not an uncommon occurrence during the unit tests. It doesn’t deserve a warning. commit ecfe72ef925a9e561d9a6c94ba7bac12eede550e Author: Philip Withnall <philip@tecnocode.co.uk> Date: Tue Nov 19 00:33:29 2013 +0000 tracker: Move with-session-bus-tracker.sh into test-case.vala Eliminate the last bits of shell scripting from the test suites. Instead of starting and stopping the Tracker services through tracker-control, the Tracker services are now started through D-Bus activation, and die when the mock D-Bus bus is destroyed. commit 919a6dc73ed9717f5d2696dcf1d753cb5a789851 Author: Philip Withnall <philip@tecnocode.co.uk> Date: Mon Nov 18 23:41:42 2013 +0000 tests: Remove redundant execute-test.sh script It was used to redirect verbose logging output to a log file, but automake 1.11 does that automatically now with its parallel test framework (which folks has enabled). commit 3acaaeeb997f1240011806fabda95c17cbe0df1a Author: Philip Withnall <philip@tecnocode.co.uk> Date: Mon Nov 18 23:24:27 2013 +0000 eds: Move with-session-bus-eds.sh into test-case.vala Eliminate some more of the shell scripting which plagues our test cases. This wipes up to 2s off the startup time for the EDS tests (which was previously hard-coded to spend waiting for the EDS processes to start up; now the processes are started through D-Bus service activation, so we only wait as long as necessary).