GNOME Bugzilla – Bug 695381
enabling/disabling backends via environment is needlessly complicated
Last modified: 2013-03-18 21:03:32 UTC
You can't just say "FOLKS_BACKENDS_ALLOWED=eds" to knock out all other backends for testing, or "FOLKS_BACKENDS_DISABLED=tracker" to knock out that one backend; you have to write a .ini file and point Folks to it. I propose adding those two variables. :-)
Created attachment 238331 [details] [review] BackendStore: provide an easier way to limit backends via environment If FOLKS_BACKENDS_ALLOWED is set, it's a space-, comma- or colon-separated list of backends to allow, or "all". If unset, the default is "all". Backends not in the list are disabled, even if the keyfile says they ought to be enabled. If FOLKS_BACKENDS_DISABLED is set, it's a space-, comma- or colon-separated list of additional backends to disable. --- Accepting any of space, comma, colon is consistent with g_parse_debug_string().
Created attachment 238332 [details] [review] tests: limit backends via the environment a bit more directly This makes it more obvious what's enabled and what isn't. We want to make sure that only the intended backends are enabled, because those are the ones for which we've done enough environmental setup (D-Bus, etc.) to make sure they don't "leak out" into the user's real data. --- As per Bug #690830, it would be even better for the setup to move into the tests, so you can run the executable and the omission of the TESTS_ENVIRONMENT won't make it trash your home directory - e.g. using g_test_dbus_up(), g_test_dbus_down() - but for now, I'm setting the environment variables in the same place where we set everything else.
Review of attachment 238331 [details] [review]: Please add documentation to HACKING for these environment variables. The documentation for BackendStore.[enable|disable]_backend() also needs amending to mention that it won’t have any (immediate) effect on backends disabled by the environment variables. Also, please make sure to add an entry to NEWS for every bug you fix (and backdate them for the couple of bugs you’ve already committed fixes for). ::: folks/backend-store.vala @@ +156,3 @@ this._prepared_backends_ro = this._prepared_backends.read_only_view; + + /* Which backends do we want? */ I think this whole block of code might better live in BackendStore._load_disabled_backend_names(), so that it’s in the same place as the rest of the code handling backend enabling/disabling. Could you also please expand this comment with some of the documentation from comment #1. @@ +161,3 @@ + if (envvar != null) + { + var tokens = envvar.split_set(" ,:"); Missing space before ‘(’. @@ +182,3 @@ + if (envvar != null) + { + var tokens = envvar.split_set(" ,:"); And here. @@ +191,3 @@ + this._backends_disabled.add (s); + } + } It would be useful if a summary of which backends are explicitly disabled/enabled was printed out (using debug()), so that it’s present in debug logs for when we can’t work out why a specific backend is (not) being loaded.
Created attachment 238584 [details] [review] BackendStore: provide an easier way to limit backends via environment If FOLKS_BACKENDS_ALLOWED is set, it's a space-, comma- or colon-separated list of backends to allow, or "all". If unset, the default is "all". Backends not in the list are disabled, even if the keyfile says they ought to be enabled. If FOLKS_BACKENDS_DISABLED is set, it's a space-, comma- or colon-separated list of additional backends to disable. --- Revised as requested. (Do I assume correctly from your review that you approve of this feature in general principle?)
Created attachment 238585 [details] [review] tests: limit backends via the environment a bit more directly This makes it more obvious what's enabled and what isn't. We want to make sure that only the intended backends are enabled, because those are the ones for which we've done enough environmental setup (D-Bus, etc.) to make sure they don't "leak out" into the user's real data.
Review of attachment 238584 [details] [review]: I do approve of this in principle; and in practice. Please commit to master!
Review of attachment 238585 [details] [review]: While it’s good to get rid of the .ini files, I think that replacing them with another environment variable in the Makefiles isn’t the best solution. I think it would be cleaner to add a subclass of Folks.TestCase for each backend (e.g. Folks.EdsTestCase, etc.) which set the environment variables as appropriate in the set_up() and tear_down() methods.
Created attachment 238732 [details] [review] TestCase: fix indentation --- Cosmetic.
Created attachment 238733 [details] [review] Add subclasses for each category of TestCase Move small amounts of initialization (just the low-hanging fruit) into each subclass. Folks.TestCase is now reserved for tests that don't need any backends or additional services at all. In the process, make TpTestsBackend.tear_down() idempotent, so it's OK if we call it repeatedly. --- This only goes a short distance towards Bug #690830: a significant number of tests currently assume that some of the (un)initialization will be process-global rather than per-test, so some of the setup/teardown currently needs to be in the TestCase constructor/destructor, not in set_up()/tear_down().
Created attachment 238734 [details] [review] tests: limit backends via the environment a bit more directly This makes it more obvious what's enabled and what isn't. We want to make sure that only the intended backends are enabled, because those are the ones for which we've done enough environmental setup (D-Bus, etc.) to make sure they don't "leak out" into the user's real data. --- Now in the executable like you suggested. This could, in fact, now be a library call... but I think I prefer using the environment anyway, because "FOLKS_BACKENDS_ALLOWED=eds ./my-benchmark" is still pretty useful.
Created attachment 238735 [details] [review] All tests except e-d-s and tracker: use GTestDBus These tests don't actually need any .service files, so we don't need to use anything beyond basic GTestDBus functionality. Insulate them from the user's environment via GTestDBus instead of using telepathy-glib's equivalent. We keep the execute-test.sh bit, because that just does the "silence tests unless they succeed" logic - it's useful on a buildd or whatever, and it's harmless that it doesn't run if you run the test directly. Eventually, the e-d-s and tracker tests should pull in the functionality of their respective shell scripts, but that's a job for another day.
Created attachment 238737 [details] [review] All tests except e-d-s and tracker: use GTestDBus These tests don't actually need any .service files, so we don't need to use anything beyond basic GTestDBus functionality. Insulate them from the user's environment via GTestDBus instead of using telepathy-glib's equivalent. We keep the execute-test.sh bit, because that just does the "silence tests unless they succeed" logic - it's useful on a buildd or whatever, and it's harmless that it doesn't run if you run the test directly. Eventually, the e-d-s and tracker tests should pull in the functionality of their respective shell scripts, but that's a job for another day.
Review of attachment 238732 [details] [review]: ♥
Comment on attachment 238732 [details] [review] TestCase: fix indentation 431930d15
Comment on attachment 238584 [details] [review] BackendStore: provide an easier way to limit backends via environment 1fd8b55222
Review of attachment 238733 [details] [review]: I bet that was less fun to write than it was to review; thanks for coming up with this. Presumably all the tests still pass reliably after applying this? ::: tests/eds/avatar-details.vala @@ +46,3 @@ Value? v; + this.eds_backend.reset(); Missing space before the ‘()’. ::: tests/eds/im-details.vala @@ +49,3 @@ Value? v; + this.eds_backend.reset(); Missing space before the ‘()’. ::: tests/eds/individual-retrieval.vala @@ +44,3 @@ Value? v; + this.eds_backend.reset(); Space. ::: tests/eds/link-personas-diff-stores.vala @@ +51,2 @@ Environment.set_variable ("FOLKS_BACKEND_EDS_USE_ADDRESS_BOOKS", "test:other", true); s/test/this.eds_backend.address_book_uid/ for correctness? ::: tests/eds/persona-store-tests.vala @@ +38,1 @@ this._capabilities_received = new HashSet<string> (); I guess technically we should nullify _capabilities_received in tear_down(). ::: tests/eds/phone-details.vala @@ +48,3 @@ Value? v; + this.eds_backend.reset(); Space. ::: tests/lib/eds/test-case.vala @@ +23,3 @@ + */ + +public class EdsTest.TestCase : Folks.TestCase This file could do with some brief documentation of the class and its methods and properties. @@ +36,3 @@ + base.set_up (); + create_backend (); + configure_primary_store (); Please use ‘this.’ explicitly when calling methods, to make it clear where the method comes from. ::: tests/lib/libsocialweb/test-case.vala @@ +25,3 @@ +public class LibsocialwebTest.TestCase : Folks.TestCase +{ + public LibsocialwebTest.Backend? lsw_backend = null; Can lsw_backend ever actually be null? It’s assigned in the constructor and never nullified as far as I can see. @@ +44,3 @@ + var main_loop = new GLib.MainLoop (null, false); + + this.lsw_backend.ready.connect(() => Missing space before the ‘(’. ::: tests/lib/telepathy/test-case.vala @@ +23,3 @@ + */ + +public class TpfTest.TestCase : Folks.TestCase This could do with some (brief) documentation of the class and its methods and properties. @@ +42,3 @@ + + create_kf_backend (); + create_tp_backend (); Please use ‘this.’ explicitly when calling local methods, to make it more obvious where the method comes from. @@ +60,3 @@ + base.set_up (); + set_up_tp (); + set_up_kf (); And here. ::: tests/lib/test-case.vala @@ +61,3 @@ + * will be called once, the last time only. It should undo the + * constructor, and must be idempotent (i.e. OK to call more than once). */ + public virtual void final_tear_down () Why is it necessary to have this in a separate method? Can’t the final tear down be done in chained-up destructors? ::: tests/lib/tracker/test-case.vala @@ +21,3 @@ + */ + +public class TrackerTest.TestCase : Folks.TestCase This file could also do with some (brief!) documentation, e.g. of what it sets up. @@ +23,3 @@ +public class TrackerTest.TestCase : Folks.TestCase +{ + public TrackerTest.Backend? tracker_backend = null; Can this ever be null? It’s assigned to in the constructor and never nullified as far as I can see. ::: tests/telepathy/individual-properties.vala @@ +53,1 @@ + this._changes_pending = new HashSet<string> (); Perhaps _changes_pending should be nullified in tear_down()? ::: tests/telepathy/persona-store-capabilities.vala @@ +47,1 @@ + this._capabilities_received = new HashSet<string> (); Should _capabilities_received be nullified in tear_down()?
Review of attachment 238734 [details] [review]: Looks great.
Review of attachment 238737 [details] [review]: Does this lose us the functionality dbus-session.sh had to allow automatic spawning of a dbus-monitor for the private bus? I guess that’s not a great loss. ::: tests/lib/test-case.vala @@ +46,3 @@ + /* This is per-process, not per-test, because some of the tests assume + * that that will be the case (and also to make tests faster). + * It can be overridden to do-nothing if necessary. */ Which tests assume it’s the case? They might need to be rewritten. I guess that also implies we can’t put a check in tear_down() to see if there are no well-known names still registered on the bus.
(In reply to comment #18) > Does this lose us the functionality dbus-session.sh had to allow automatic > spawning of a dbus-monitor for the private bus? I guess that’s not a great > loss. Actually, reviewing your patch from bug #690830 reminded me that I’ve made use of the dbus-monitor functionality before, and it was quite useful. Keeping it would be nice.
(In reply to comment #16) > I bet that was less fun to write than it was to review; thanks for coming up > with this. Presumably all the tests still pass reliably after applying this? They seem to. I also used a dbus-monitor on my real session bus to confirm that nothing "leaks". > + this.eds_backend.reset(); > > Missing space before the ‘()’. Not a regression; I mostly just did a search/replace, so anywhere that previously had insufficient whitespace will still have insufficient whitespace. I can fix this in lines I touched if you want. > Environment.set_variable ("FOLKS_BACKEND_EDS_USE_ADDRESS_BOOKS", > "test:other", true); > > s/test/this.eds_backend.address_book_uid/ for correctness? Not a regression, but yeah, I suppose so. > ::: tests/eds/persona-store-tests.vala > @@ +38,1 @@ > this._capabilities_received = new HashSet<string> (); > > I guess technically we should nullify _capabilities_received in tear_down(). The Vala destructor will free it anyway, and set_up() replaces it with a new empty one; but if you want tear_down() to be particularly thorough, I could clear() it. > +public class EdsTest.TestCase : Folks.TestCase > > This file could do with some brief documentation of the class and its methods > and properties. Sure. > + public LibsocialwebTest.Backend? lsw_backend = null; > > Can lsw_backend ever actually be null? It’s assigned in the constructor and > never nullified as far as I can see. I don't really know how thorough Vala is about verifying non-nullness. If you say this is OK, then it's OK, and I can make this non-nullable. If you want its life-cycle to eventually change to "set_up() to tear_down()" instead of "ctor to final_tear_down()", then it does need to be nullable anyway, because it will be null between ctor and set_up(), and between tear_down() and final_tear_down() (or destruction). > ::: tests/lib/test-case.vala > @@ +61,3 @@ > + * will be called once, the last time only. It should undo the > + * constructor, and must be idempotent (i.e. OK to call more than once). */ > + public virtual void final_tear_down () > > Why is it necessary to have this in a separate method? Can’t the final tear > down be done in chained-up destructors? It could, but some language communities consider too much reliance on destructors to be a bad thing. I'm not sure to what extent Vala is one of those, so I erred on the side of making it possible to dismantle the object even if refs are leaked. I didn't actually patch any of the tests to do so yet, because life's too short; I plan to do this in new tests, though. > ::: tests/telepathy/individual-properties.vala > @@ +53,1 @@ > + this._changes_pending = new HashSet<string> (); > > Perhaps _changes_pending should be nullified in tear_down()? As above, Vala ensures that it isn't leaked, and set_up() ensures that it's replaced with a new empty one before the next test; but I could clear() it if you like. (In reply to comment #18) > Does this lose us the functionality dbus-session.sh had to allow automatic > spawning of a dbus-monitor for the private bus? No, GTestDBus has this too. Set the environment variable G_DBUS_MONITOR (to any value) and it'll share the test's std(out|err). (In reply to comment #18) > ::: tests/lib/test-case.vala > @@ +46,3 @@ > + /* This is per-process, not per-test, because some of the tests assume > + * that that will be the case (and also to make tests faster). > + * It can be overridden to do-nothing if necessary. */ > > Which tests assume it’s the case? They might need to be rewritten. From the point of view of having or not having stale data, I don't know of any that assume this, but any TestCase containing more than one test *could* have this assumption at the moment. To avoid having to have too much test churn, I mostly kept the current semantics in terms of what happens in ctor/dtor and what happens in set_up/tear_down, even if it didn't make perfect sense (some backends are set up in the ctor and some in set_up). I'd rather not get too far into redesigning this right now. Because environment variables are process-global state and libraries frequently assume that that means their values will remain constant, I don't think we can go as far as having one dbus-daemon or one temporary $HOME-equivalent per set_up() - GLib caches XDG_CONFIG_HOME etc., and libdbus will cache the DBusConnection for the session bus, ignoring subsequent changes to DBUS_SESSION_BUS_ADDRESS. In principle we could kill and restart the Evolution/Tracker services per-test and empty their ~/.local, ~/.config, ~/.cache... but doing too much in set_up() and tear_down() slows us down, and Folks' tests take too long already :-) > I guess that > also implies we can’t put a check in tear_down() to see if there are no > well-known names still registered on the bus. I don't think we can put that check in tear_down() anyway, as long as we're using the standard session service directories (which we do for e-d-s and Tracker, but not for the others): as soon as any backend uses dconf, GConf or any similar service, they'll be autolaunched. I'm not so sure that it matters anyway. When the temporary D-Bus session is killed off by final_tear_down(), its services will all lose their D-Bus connections and exit (gracefully or otherwise). GTestDBus arranges for the dbus-daemon to be killed even if the test crashes (it forks a subprocess on Unix, or uses Windows APIs to tie its lifetime to the test's) so this killing is reliable - more so than with-session-bus.sh, in fact.
Created attachment 238894 [details] [review] fixup "Add subclasses...": correct whitespace --- To be squashed into Attachment #238733 [details], but it's probably easier to review like this.
Created attachment 238895 [details] [review] fixup "Add subclasses...": be clearer about "this.foo" --- To be squashed into Attachment #238733 [details], but it's probably easier to review like this.
Created attachment 238898 [details] [review] tests: limit backends via the environment a bit more directly This makes it more obvious what's enabled and what isn't. We want to make sure that only the intended backends are enabled, because those are the ones for which we've done enough environmental setup (D-Bus, etc.) to make sure they don't "leak out" into the user's real data. --- Just adjusted for conflicts with Attachment #238895 [details].
Created attachment 238899 [details] [review] Reduce hard-coding of e-d-s address book names --- As per review.
Created attachment 238900 [details] [review] Make EdsTest.TestCase.eds_backend nullable It's null outside the period from set_up() to tear_down().
Created attachment 238901 [details] [review] LibsocialwebTest.TestCase: defer creation of lsw_backend until set_up() Also destroy it in tear_down(), for symmetry. Since there are only two LSW test cases and neither of them has more than one test, we don't need to worry much about compatibility - it's easy to check.
Created attachment 238902 [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. --- Also addresses Bug #690830. Attachment #238737 [details] moves to just before this one in sequence.
Created attachment 238903 [details] [review] Document recently-added test infrastructure
Sorry, didn't spot your review of Bug #690830 until just now. I'll look into addressing that too.
Created attachment 238909 [details] [review] fixup "Replace with-session-bus*.sh": s/message/debug/ To be squashed into Attachment #238902 [details] as per pwithnall's review over on Bug #690830, but it's probably easier to review this way.
Created attachment 238910 [details] [review] fixup "Replace with-session-bus*.sh": explain shell-replacement code --- To be squashed into Attachment #238902 [details] as per pwithnall's review over on Bug #690830, but it's probably easier to review this way.
Created attachment 238911 [details] [review] fixup "Replace with-session-bus*.sh": one more missing "this." --- To be squashed into Attachment #238902 [details] as per pwithnall's review over on Bug #690830, but it's probably easier to review this way.
Created attachment 238914 [details] [review] fixup "Replace with-session-bus*.sh": use .local/share, not .local, for more realism --- Likewise.
Created attachment 238915 [details] [review] Wait for e-d-s bus names instead of sleeping for 2 seconds --- This assumes that e-d-s' bus names will remain somewhat stable, i.e. that it won't break D-Bus API ridiculously often. If it does break D-Bus API and we want to remain compatible with both, we do have the option of adding a watch for (e.g.) both Sources1 and Sources2, and setting have_sources = true when *either* becomes available.
Created attachment 238920 [details] [review] Set more environment variables to insulate tests from the real user In some tests we currently allow arbitrary session services to run if service-activated (e.g. gvfs, dconf, GConf), and we don't want those services to use the user's real home directory. --- This mostly addresses my concerns about allowing arbitrary session services to be activated - at least on systems with either GNOME 3.8, or Debian's patched version of earlier GNOME releases.
Unaddressed points from the review on Bug #690830: * I'm not yet explicitly killing the Evolution D-Bus services (but they'll die with the bus anyway, unless they have serious bugs, as will any other services that were activated). * I'm still trying twice to apply the equivalent of rm -rf to the transient directory, then giving up and assuming everything's fine. * The FIXME about allowing arbitrary session services is still present, unless/until someone comes to a conclusion whether it's OK or not. GTestDBus runs dbus-daemon without redirecting its stdout/stderr away, so we can see a log message every time a service gets activated (these are suppressed by execute-test.sh, unless either the test fails or CHECK_VERBOSE is set). Similarly, the activated service's stdout and stderr get passed-through, then redirected to files by execute-test.sh. On my system, the eds tests activate gvfs and GNOME Online Accounts, and the Tracker tests activate Tracker1, Tracker1.Miner.Files and Tracker1.Miner.Flickr.
Review of attachment 238894 [details] [review]: ++
Review of attachment 238895 [details] [review]: ++
Review of attachment 238898 [details] [review]: ++
Review of attachment 238899 [details] [review]: ++
Review of attachment 238900 [details] [review]: ++
Review of attachment 238901 [details] [review]: ++
Review of attachment 238902 [details] [review]: ++
Review of attachment 238903 [details] [review]: Nice! Thanks.
Review of attachment 238909 [details] [review]: ++
Review of attachment 238910 [details] [review]: ++
Review of attachment 238911 [details] [review]: ++
Review of attachment 238914 [details] [review]: ++
Review of attachment 238915 [details] [review]: ::: tests/lib/eds/test-case.vala @@ +156,3 @@ + MainContext.default().iteration (true); + + Source.remove (timer_id); Shouldn’t you also call Bus.unwatch_name() here, otherwise you’re leaking the two closures (since the callbacks will remain callable throughout the test).
Review of attachment 238920 [details] [review]: Please commit with the minor change below. ::: tests/lib/test-case.vala @@ +84,3 @@ + /* GLib < 2.36 in Debian obeyed this (although upstream GLib < 2.36 + * used the home directory from passwd), so set it too, until + * we depend on 2.36. */ If this can be removed when we depend on GLib 2.36, it should probably be a FIXME so we don’t forget about it.
One remaining issue that I have noticed is that final_tear_down() is actually never called, presumably because there's a refleak or circular reference somewhere. :-( Unfortunately, when I force it, we get a warning from libebook, which we have made fatal: > (/home/smcv/build/folks/opt/tests/eds/.libs/lt-anti-linking:32143): libebook-WARNING **: book_client_close_cb: The connection is closed ... so I think I'll have to turn off fatality of warnings during final_tear_down() for now, patch e-d-s so it doesn't consider "I tried to disconnect but I was already disconnected!" to be a problem, and either find the refleak or explicitly do a final_tear_down() in each test. This nicely illustrates why final_tear_down() isn't just a destructor :-) (In reply to comment #9) > Attachment #238733 [details] > Add subclasses for each category of TestCase Is this ready for commit in conjunction with the other changes I've made? Unaddressed points from your review: you suggested clear()ing the data structures, but they're thrown away and replaced in set_up() anyway, and Vala will free them when the object is destroyed, so I don't think this is necessary. (In reply to comment #12) > Attachment #238737 [details] > All tests except e-d-s and tracker: use GTestDBus Is this ready for commit, given that dbus-monitor is still available? (In reply to comment #49) > Shouldn’t you also call Bus.unwatch_name() here, otherwise you’re leaking the > two closures (since the callbacks will remain callable throughout the test). Yes, I should; I'll do that. (In reply to comment #50) > If this can be removed when we depend on GLib 2.36, it should probably be a > FIXME so we don’t forget about it. Sure.
(In reply to comment #51) > One remaining issue that I have noticed is that final_tear_down() is actually > never called, presumably because there's a refleak or circular reference > somewhere. :-( This turns out to be a rather substantial amount of yak shaving. I tried converting one test from each subdirectory to do an explicit final_tear_down(), but: * 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 Meanwhile, I don't think relying on a destructor for final teardown is going to work either, because the way TestCase adds its tests results in circular references and it's never freed.
(In reply to comment #51) > ... so I think I'll have to turn off fatality of warnings during > final_tear_down() for now, patch e-d-s so it doesn't consider "I tried to > disconnect but I was already disconnected!" to be a problem, and either find > the refleak or explicitly do a final_tear_down() in each test. OK. Make sure to put a FIXME comment in referencing the EDS bug. > (In reply to comment #9) > > Attachment #238733 [details] > > Add subclasses for each category of TestCase > > Is this ready for commit in conjunction with the other changes I've made? Yes. I think. It’s hard to keep track of all the patches. > Unaddressed points from your review: you suggested clear()ing the data > structures, but they're thrown away and replaced in set_up() anyway, and Vala > will free them when the object is destroyed, so I don't think this is > necessary. I realise that nullifying the data structures in tear_down() is basically equivalent to re-constructing them in set_up(); the tidyness and symmetry appealed to me. It also reduces the possibility of them getting referenced by something in set_up() and never actually being destroyed. > (In reply to comment #12) > > Attachment #238737 [details] > > All tests except e-d-s and tracker: use GTestDBus > > Is this ready for commit, given that dbus-monitor is still available? Yes. (In reply to comment #52) > This turns out to be a rather substantial amount of yak shaving. I tried > converting one test from each subdirectory to do an explicit final_tear_down(), > but: > > * 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 I guess this all needs fixing, though to make patch management easier you could commit all your existing patches then work on this yak shaving in a new patch set. Thanks for doing this; sorry it’s so painful. Hopefully a generic, clean, re-usable approach to testing code which uses D-Bus services can come out of this.
Created attachment 239131 [details] [review] [1/10] Add subclasses for each category of TestCase Move small amounts of initialization (just the low-hanging fruit) into each subclass. Folks.TestCase is now reserved for tests that don't need any backends or additional services at all. In the process, make TpTestsBackend.tear_down() idempotent, so it's OK if we call it repeatedly. --- Basically Attachment #238733 [details], Attachment #238894 [details] and Attachment #238895 [details] squashed together. "Mostly reviewed".
Created attachment 239132 [details] [review] [2/10] Reduce hard-coding of e-d-s address book names --- Same as Attachment #238899 [details], effectively already reviewed.
Created attachment 239133 [details] [review] [03/10] Make EdsTest.TestCase.eds_backend nullable It's null outside the period from set_up() to tear_down(). --- Same as Attachment #238900 [details] so already basically reviewed.
Created attachment 239134 [details] [review] [4/10] tests: limit backends via the environment a bit more directly --- Effectively Attachment #238900 [details], so basically already reviewed.
Created attachment 239135 [details] [review] [5/10] LibsocialwebTest.TestCase: defer creation of lsw_backend until set_up( --- Effectively the same as Attachment #238901 [details] so basically already reviewed.
Created attachment 239136 [details] [review] [6/10] Document recently-added TestCase subclasses They currently assume that they will be wrapped in with-session-bus*.sh. --- This is about half of Attachment #238903 [details]; I've left out the "use GTestDBus" bits.
Created attachment 239137 [details] [review] [7/10] Assert that each D-Bus test case is run in its intended environment This is a safety check so we don't trash the user's real home directory. --- New. I deliberately set the environment variables in with-*.sh, because that's the location that does the sandboxing. This check can mostly disappear when we switch to GTestDBus, but I want to set the environment variables anyway, because for performance testing I'm planning to add some helper binaries that do things like clearing the e-d-s address books in a separate process (so it's easier to separate profiling the setup from profiling the thing we actually wanted to profile), and those helper binaries should definitely check the environment variables!
Created attachment 239138 [details] [review] [08/10] For symmetry, nullify sets in tear_down() if created in set_up() --- New.
Created attachment 239139 [details] [review] [09/10] All tests: switch to a calling convention that does not rely on leaking The standard main() used in each test relied on the fact that get_suite() kept a ref to the TestCase subclass instance, which would be stored in the global variables of the GTest infrastructure, resulting in the TestCase never being freed and its final_teardown() never being executed. That's undesirable if final_teardown() is going to do something significant, like terminating a temporary session bus. As a first step towards that, run the tests with the TestCase subclass instance globally referenced (so that the version passed to GTest can eventually be a weak reference), and explicitly run final_tear_down() so that even if there is a ref leak (as there is now), it gets executed. --- New. The part in tests/lib/test-case.vala is the only interesting bit. Most of the rest was done using perl - please skim-read.
Created attachment 239140 [details] [review] [10/10] TestCase: avoid circular refs between GTest and this Instead of running tests via the Adaptor inner class, run them via a C helper that takes a weak ref to the TestCase. This relies on the caller keeping a ref to the TestCase until after Test.run() - but I recently converted the tests to do exactly that, so that's OK. Adding a debug message which prints this.ref_count to final_tear_down() reveals that some of our regression tests still leak the TestCase, but many don't now. --- New. This is a step towards being able to do the GTestDBus stuff properly, later.
(In reply to comment #53) > I guess this all needs fixing, though to make patch management easier you could > commit all your existing patches then work on this yak shaving in a new patch > set. Thanks for doing this; sorry it’s so painful. Hopefully a generic, clean, > re-usable approach to testing code which uses D-Bus services can come out of > this. I think I'd prefer to commit something in which final_tear_down() is actually called, so I don't have to say "this is probably right but has never actually been run" :-) ... so please try [1/10] - [10/10], and when those are OK, I'll rebase the replacement of with-session-bus*.sh with Vala onto them. (Probably better done over on Bug #690830.)
Review of attachment 239131 [details] [review]: ++
Review of attachment 239136 [details] [review]: ++
Review of attachment 239137 [details] [review]: ++ ::: tests/lib/eds/test-case.vala @@ +43,3 @@ public TestCase (string name) { + if (Environment.get_variable ("FOLKS_TESTS_SANDBOXED_DBUS") Could do with a brief comment mentioning that FOLKS_TESTS_SANDBOXED_DBUS is set in with-session-bus-*.sh, along with the envvars we care about.
Review of attachment 239138 [details] [review]: ++
Review of attachment 239139 [details] [review]: ++
Review of attachment 239140 [details] [review]: Missing test-case-helper.c? The Vala changes look OK.
Created attachment 239179 [details] [review] TestCase: avoid circular refs between GTest and this Instead of running tests via the Adaptor inner class, run them via a C helper that takes a weak ref to the TestCase. This relies on the caller keeping a ref to the TestCase until after Test.run() - but I recently converted the tests to do exactly that, so that's OK. Adding a debug message which prints this.ref_count to final_tear_down() reveals that some of our regression tests still leak the TestCase, but many don't now. --- Now with the C part included, oops.
Created attachment 239180 [details] [review] all Makefiles: don't ignore or clean .c files not corresponding to Vala This will become more important if we use C to implement fast-paths and faster data structures. --- This specifically requires GNU make (BSD make, or whatever, is not enough), but I suspect GNU make is considerably more portable than Folks anyway, and you were already using the $(add-prefix ) pseudo-function which I think is also GNU-specific.
Comment on attachment 239131 [details] [review] [1/10] Add subclasses for each category of TestCase 63cc94f
Comment on attachment 239132 [details] [review] [2/10] Reduce hard-coding of e-d-s address book names 6bed888
Comment on attachment 239133 [details] [review] [03/10] Make EdsTest.TestCase.eds_backend nullable 039a7cc
Comment on attachment 239134 [details] [review] [4/10] tests: limit backends via the environment a bit more directly e4de6c5
Comment on attachment 239135 [details] [review] [5/10] LibsocialwebTest.TestCase: defer creation of lsw_backend until set_up( fb2abc0
Comment on attachment 239136 [details] [review] [6/10] Document recently-added TestCase subclasses ed87765
Comment on attachment 239137 [details] [review] [7/10] Assert that each D-Bus test case is run in its intended environment 19a44b0
Comment on attachment 239138 [details] [review] [08/10] For symmetry, nullify sets in tear_down() if created in set_up() 4520355
Comment on attachment 239139 [details] [review] [09/10] All tests: switch to a calling convention that does not rely on leaking afb97ca
Review of attachment 239179 [details] [review]: ++
Review of attachment 239180 [details] [review]: It’s sad that this is necessary (automake really should clean Vala-generated C files itself), but it’s a good catch. Thanks.
Comment on attachment 239179 [details] [review] TestCase: avoid circular refs between GTest and this 8268c6e
Comment on attachment 239180 [details] [review] all Makefiles: don't ignore or clean .c files not corresponding to Vala bcec3e3
I'm going to call this FIXED, and go over to Bug #690830 for the D-Bus bits.