Re: [Evolution-hackers] Isolated unit test status report
On Sun, 2013-02-24 at 23:01 -0500, Matthew Barnes wrote: I went ahead and committed it for 3.8. We'll see if this has any impact on the GDBusConnection leak, but it's an improvement regardless. Hi, easy to test it with Tristan's [1], isn't it? You'll realize it'll not help, just the opposite, as I expect that creating a factory connection for each created book/calendar client only adds more things to be done on various layers, thus makes opening generally slower. Just my untested opinion. The issue with [1] is also related to [2], if it'll help you to properly fix them both. If you do not like the already available tool for testing, then I can provide a simple test.c, which will exhibit the same issue - I only wanted to use something in place, to exhibit the regression from 3.6 code. Bye, Milan [1] https://bugzilla.gnome.org/show_bug.cgi?id=693461 [2] https://bugzilla.gnome.org/show_bug.cgi?id=693464 ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... https://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] Isolated unit test status report
On Mon, 2013-02-25 at 09:23 +0100, Milan Crha wrote: On Sun, 2013-02-24 at 23:01 -0500, Matthew Barnes wrote: I went ahead and committed it for 3.8. We'll see if this has any impact on the GDBusConnection leak, but it's an improvement regardless. Hi, easy to test it with Tristan's [1], isn't it? You'll realize it'll not help, just the opposite, as I expect that creating a factory connection for each created book/calendar client only adds more things to be done on various layers, thus makes opening generally slower. Milan, the factory connection is certainly something to consider here. While you have a point that, we still wont be able to test it until EBookClient/ECalClient can properly be finalized (i.e. the bug you refer to regarding imbalanced reference counts needs also to be fixed), we still need a way to release that factory connection. Perhaps, if performance at connection time is very slow, we should take an approach that the factory connection be released when all EBookClients are finalized (with a sort of internal ref-counting logic)... if of course, performance of opening the book is such a bottleneck. Cheers, -Tristan Just my untested opinion. The issue with [1] is also related to [2], if it'll help you to properly fix them both. If you do not like the already available tool for testing, then I can provide a simple test.c, which will exhibit the same issue - I only wanted to use something in place, to exhibit the regression from 3.6 code. Bye, Milan [1] https://bugzilla.gnome.org/show_bug.cgi?id=693461 [2] https://bugzilla.gnome.org/show_bug.cgi?id=693464 ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... https://mail.gnome.org/mailman/listinfo/evolution-hackers ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... https://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] Isolated unit test status report
On Sun, 2013-02-24 at 20:54 -0500, Matthew Barnes wrote:
On Sat, 2013-02-23 at 23:38 +0900, Tristan wrote:
This indicates that somewhere, in the client or server, we
are leaking references to the GDBusConnections. I've tested
this in raw GIO (and added a test case there) to ensure it
is indeed possible to stop/start the GTestDBus between tests,
the problem is clearly an ugly bug to be fixed in EDS.
I suspect this might be related to the factory proxy object that we
create in the background and leave lying around as a global variable.
We could try letting EBookClient create its own EDBusAddressBookFactory
proxy, call its OpenAddressBook method, and then immediately destroy it.
That would simplify the code, eliminate the global variable, and quite
possibly fix the GDBusConnection leak (if my guess is right).
I'll prototype this for 3.9. I think I've pulled enough D-Bus stunts
for one release already.
This is due to a race condition in e_client_remove_sync(),
this call is made at the end of a test case.. which results
in the ESource being removed from the ESourceRegistry server.
What happens when the cache-reaper module is loaded, is that
the 'test-address-book' source is removed /some time after/
e_client_remove_sync() completes... sometimes this happens
right in the middle of the following test case.
Probably another side effect of (A).
In the interim, it might help if the ESource's unique ID were actually
unique for each test case, instead of a static ID for all cases.
Instead of test-address-book use test-address-book-N where 'N' is
the test case number or some other increasing integer.
I had been trying to avoid that since the beginning ;-)
But... for now let's roll with that until we can fix the other issues.
I've made some fixes and now test cases are working much better
with the workarounds in place. I think the final touch will be
to run the module-prompter test relocated and then make check
will be completely passing again.
I've made four relevant commits, two of them touch outside
the test casing fixture and simply add environment variable
support to allow relocation of various loaded modules (and
this fixes the breakage that is only noticeable where EDS
has _never_ been installed, i.e. regarding missing module
directories in ${prefix}/lib/evolution-data-server).
commits listed below.
Cheers,
-Tristan
commit 04906b9b666b14b30667e7180ac1ae17e5258258
Author: Tristan Van Berkom trista...@openismus.com
Date: Mon Feb 25 19:35:20 2013 +0900
test-client-view-operations.c: Now test both direct access and indirect
access
This works properly now since we fixed (or worked around) issues in the
test fixtures. Since we use a separate ESource UID (sandbox) for each
test case this now works properly.
commit 79041f15c816c7f7b9ad90657c928da94e245e6c
Author: Tristan Van Berkom trista...@openismus.com
Date: Mon Feb 25 19:33:04 2013 +0900
Some changes to the isolated test fixture
a.) Setup EDS_REGISTRY_MODULES to use the local cache-reaper module
b.) Setup EDS_CAMEL_PROVIDER_DIR to use the local provider
c.) Added source_name and use separate ESource uids between tests,
each test in the suite uses a separate ESource name
Note that 'c' is a stop-gap solution to the problem of not being able to
properly shut-down the whole test D-Bus environment between tests, it
should be removed once we can address that problem.
commit 85afb581c3e5253e77c906c175a93dee5f7705b3
Author: Tristan Van Berkom trista...@openismus.com
Date: Mon Feb 25 19:28:04 2013 +0900
Make the Camel provider modules relocatable
This simply adds an environment variable allowing us to load the local
provider from a relocated location in the case we run 'make check'
without installing (this avoids some warnings and helps to isolate the
test environment).
commit 410a56cd3fd1793f6c523e416f2d73c4baf2fc05
Author: Tristan Van Berkom trista...@openismus.com
Date: Mon Feb 25 19:26:45 2013 +0900
Make the ESourceRegistryServer modules relocatable like addressbook
calendar
This simply adds an environment variable allowing us to load registry
modules from a relocated location in the case we run 'make check'
without installing (also ensuring that we test the not-yet-installed
environment properly).
___
evolution-hackers mailing list
evolution-hackers@gnome.org
To change your list options or unsubscribe, visit ...
https://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] Isolated unit test status report
On Mon, 2013-02-25 at 19:46 +0900, Tristan wrote: [...] I've made some fixes and now test cases are working much better with the workarounds in place. I think the final touch will be to run the module-prompter test relocated and then make check will be completely passing again. Correction, after successfully running the user prompter test I found that it is in fact a functional test and not a unit test and as such has no place in running under make check (as it involves user interaction and pops up a series of dialogs). Instead I'm just moving that from TESTS to noinst_PROGRAMS. Unfortunately make check still does not pass, due to various failures in the calendar tests. These same tests actually pass in our 3.6 based openismus-work branch. At least some of the failures are due to the regression reported in bug 692802. Cheers, -Tristan ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... https://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] Isolated unit test status report
On Mon, 2013-02-25 at 18:25 +0900, Tristan wrote: Perhaps, if performance at connection time is very slow, we should take an approach that the factory connection be released when all EBookClients are finalized (with a sort of internal ref-counting logic)... if of course, performance of opening the book is such a bottleneck. Hi, it was done exactly that way before Matthew's change. I do not see a point of changing it, except of the slowness. But let's see, I can be always wrong. Bye, Milan ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... https://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] Isolated unit test status report
On Sat, 2013-02-23 at 23:38 +0900, Tristan wrote: This indicates that somewhere, in the client or server, we are leaking references to the GDBusConnections. I've tested this in raw GIO (and added a test case there) to ensure it is indeed possible to stop/start the GTestDBus between tests, the problem is clearly an ugly bug to be fixed in EDS. I suspect this might be related to the factory proxy object that we create in the background and leave lying around as a global variable. We could try letting EBookClient create its own EDBusAddressBookFactory proxy, call its OpenAddressBook method, and then immediately destroy it. That would simplify the code, eliminate the global variable, and quite possibly fix the GDBusConnection leak (if my guess is right). I'll prototype this for 3.9. I think I've pulled enough D-Bus stunts for one release already. This is due to a race condition in e_client_remove_sync(), this call is made at the end of a test case.. which results in the ESource being removed from the ESourceRegistry server. What happens when the cache-reaper module is loaded, is that the 'test-address-book' source is removed /some time after/ e_client_remove_sync() completes... sometimes this happens right in the middle of the following test case. Probably another side effect of (A). In the interim, it might help if the ESource's unique ID were actually unique for each test case, instead of a static ID for all cases. Instead of test-address-book use test-address-book-N where 'N' is the test case number or some other increasing integer. Matthew Barnes ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... https://mail.gnome.org/mailman/listinfo/evolution-hackers
Re: [Evolution-hackers] Isolated unit test status report
On Sun, 2013-02-24 at 20:54 -0500, Matthew Barnes wrote: We could try letting EBookClient create its own EDBusAddressBookFactory proxy, call its OpenAddressBook method, and then immediately destroy it. That would simplify the code, eliminate the global variable, and quite possibly fix the GDBusConnection leak (if my guess is right). I'll prototype this for 3.9. I think I've pulled enough D-Bus stunts for one release already. Okay I lied. Prototyped code works fine, it eliminates a good amount of unnecessary complexity and is embarrassingly obvious in hindsight. I went ahead and committed it for 3.8. We'll see if this has any impact on the GDBusConnection leak, but it's an improvement regardless. Matthew Barnes ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... https://mail.gnome.org/mailman/listinfo/evolution-hackers
[Evolution-hackers] Isolated unit test status report
Since I spent some time today debugging the situation, I wanted to just share my findings so that we can hopefully fix up the remaining messy parts, piece by piece. There are a few remaining problems which cause the test cases to not consistently pass, so let's run through them in a random order (there may be more problems but these are the ones known to me at this time). A.) The registry and factory services do not properly shut-down between test cases. This is probably the single most difficult and important thing to fix regarding the test cases. If you take a quick look at the test fixture[0] used to test client APIs, you will notice a fat FIXME comment near the top. This refers to us being incapable to properly shutdown the GTestDBus object used for testing. To work around this we have the code #ifdef'd into GLOBAL_DBUS_DAEMON segments (so we test all tests in a given suite with the same D-Bus daemon instead of cleaning up between each case). This indicates that somewhere, in the client or server, we are leaking references to the GDBusConnections. I've tested this in raw GIO (and added a test case there) to ensure it is indeed possible to stop/start the GTestDBus between tests, the problem is clearly an ugly bug to be fixed in EDS. B.) Leaking ref counts on EBookClient This is rather a prerequisite of 'A', since the services wait for EClients to cleanly disconnect, there's no way we're going to get the service to cleanly shutdown between tests. There is a relevant bug report to this here[1]. C.) Race conditions when adding ESources This is discussed in the recent thread here[2]. Because we are still unsure whether the ESource has made it into the addressbook factory's local ESourceRegistry instance at the time we ask the factory to create an addressbook, we fall into a trap sometimes where the addressbook fails to be created. D.) Race conditions when removing ESources This might be another consequence of 'A', or perhaps that if 'A' were solved, we would not have ever noticed this race condition (so let's be thankful that 'A' is broken for now :D). Currently, test cases behave a little differently if EDS is installed or not installed. This is because the ESourceRegistryServer loads the cache-reaper registry module in the case that EDS is installed (I've created a patch today which ensures that the in tree cache-reaper module directory is used instead of the installed directory). What is interesting about this, is that if the cache-reaper is present, tests start to fail. This is due to a race condition in e_client_remove_sync(), this call is made at the end of a test case.. which results in the ESource being removed from the ESourceRegistry server. What happens when the cache-reaper module is loaded, is that the 'test-address-book' source is removed /some time after/ e_client_remove_sync() completes... sometimes this happens right in the middle of the following test case. The result is that we fail in a side-effect, sometimes we find ourselves in one of the test cases adding a contact to an addressbook. The test code will generally: - Add a contact with e_book_client_add_contact_sync() - Check the contact is there with e_book_client_get_contact_sync() What happens is that somewhere in between these two steps, the cache-reaper module steps in and finally gets the message (from the previous test case) that this addressbook needs to be moved to the trash... the result is that we fail to fetch the newly added contact. Note that we *never* fail to fetch a newly added contact if the cache-reaper module is not present in the testing environment. E.) A fresh build of EDS will not pass tests, after make install make uninstall tests pass This is because make uninstall does not remove the module directories created by make install. Tests fail because the registry service bails out when e_module_load_all_in_directory() fails for the registry modules. This is fixed with the patch (not committed) which I created today. This is not ideal because my patch redirects the registry modules to use the in-tree cache-reaper module directory. (meaning we invariably face the race condition issues described in 'D'). Cheers, -Tristan [0]:http://git.gnome.org/browse/evolution-data-server/tree/tests/test-server-utils/e-test-server-utils.c [1]:https://bugzilla.gnome.org/show_bug.cgi?id=693461 [2]:https://mail.gnome.org/archives/evolution-hackers/2013-January/msg00027.html ___ evolution-hackers mailing list evolution-hackers@gnome.org To change your list options or unsubscribe, visit ... https://mail.gnome.org/mailman/listinfo/evolution-hackers
