GNOME Bugzilla – Bug 690876
eds: fix various test failures
Last modified: 2013-01-06 01:37:16 UTC
I'm trying to get the tests back into working shape. Here are a few which had related problems: * set-emails * set-im-addresses (mostly clean-up) * link-personas Set-emails failed because it was expecting no Individuals be removed at any point (since it literally just set an email address for a Persona not linked with any others). So, I figured out the core problem (we needed to special-case this for linkable property and anti-link changes). In the process, I split this check for excess Individual removal into its own test (linkable-properties) to avoid duplicating that in each set-* test.
Created attachment 232383 [details] [review] Fix linkable-property-related issues in EDS tests Branch here: http://cgit.collabora.com/git/user/treitter/folks.git/log/?h=build-fixes-clean-4-rebase
After this patch, the create-remove-stores test still fails some portion of the time (~30%), but I think it's an unrelated issue.
Review of attachment 232383 [details] [review]: ::: folks/individual-aggregator.vala @@ +1246,3 @@ + debug ("Ignoring linkable property change because persona has no siblings"); + return; + } I’m not sure this is correct. What if the linkable property is changed to a value which would link that persona with another one? @@ +1273,3 @@ + debug ("Ignoring anti-link changes because persona has no siblings"); + return; + } I’m not sure this is correct either. What if the anti-links are changed so that the persona can be linked to another persona it was previously anti-linked with? ::: tests/eds/link-personas.vala @@ +72,3 @@ + * inherit state from the last test */ + this._test_num++; + this._eds_backend.set_up (false, @"test$_test_num"); I don’t think this is the right fix for (what I assume is) bug #690830. I think we should move the setup/cleanup performed by the shell scripts into the test programs themselves. ::: tests/eds/linkable-properties.vala @@ +1,2 @@ +/* + * Copyright (C) 2011 Collabora Ltd. Copyright’s incorrect. @@ +15,3 @@ + * along with this library. If not, see <http://www.gnu.org/licenses/>. + * + * Authors: Raul Gutierrez Segales <raul.gutierrez.segales@collabora.co.uk> You’re not Raúl! @@ +53,3 @@ + } + + void test_linkable_properties () It would be useful to have a brief comment above this method explaining what the test is and how it works. @@ +74,3 @@ + this._main_loop.quit (); + assert_not_reached (); + }); Indentation’s a little messy here.
(In reply to comment #3) > Review of attachment 232383 [details] [review]: > > ::: folks/individual-aggregator.vala > @@ +1246,3 @@ > + debug ("Ignoring linkable property change because persona has no > siblings"); > + return; > + } > > I’m not sure this is correct. What if the linkable property is changed to a > value which would link that persona with another one? > > @@ +1273,3 @@ > + debug ("Ignoring anti-link changes because persona has no > siblings"); > + return; > + } > > I’m not sure this is correct either. What if the anti-links are changed so that > the persona can be linked to another persona it was previously anti-linked > with? Hrm, I knew this seemed too easy. I originally tried something a lot more invasive which might be the only option (and might not be worth it). I guess it's a matter of how big of an optimization this would be in the real world. It's also messy for a client to (definitively) match up a replacement Individual to its replacee with individuals-changed (as these tests use). Of course, that's why we added individuals-changed-detailed, so maybe we should use the latter signal for these tests and add regression tests for the deprecated API. At any rate, we didn't have any tests which caught the cases you listed above, so I'll add those to this branch. > ::: tests/eds/link-personas.vala > @@ +72,3 @@ > + * inherit state from the last test */ > + this._test_num++; > + this._eds_backend.set_up (false, @"test$_test_num"); > > I don’t think this is the right fix for (what I assume is) bug #690830. I think > we should move the setup/cleanup performed by the shell scripts into the test > programs themselves. I thought it was a slightly different issue (since gdb isn't supposed to terminate in between these internal test cases) but I'll see if I can solve both problems at once. > @@ +15,3 @@ > + * along with this library. If not, see <http://www.gnu.org/licenses/>. > + * > + * Authors: Raul Gutierrez Segales <raul.gutierrez.segales@collabora.co.uk> > > You’re not Raúl! But I try so hard! Added my name locally. I'll have to figure out better solutions for the non-trivial issues above. Everything else fixed locally.
Created attachment 232799 [details] [review] Fix various EDS tests (try 2) Patch from branch: http://cgit.collabora.com/git/user/treitter/folks.git/log/?h=build-fixes-clean-4-rebase ========= This addresses the issues raised for the last patch except for the work-around for bgo#690830. I think we should fix the two instances of that work-around in this branch afterward since they're not too heinous and this is already a borderline catch-all bug. With this branch, all the EDS, Telepathy, core, and key-file tests pass (haven't tested the others, but they should be unaffected and, eg, the Tracker backend needs to be updated to the latest Tracker to build in a modern jhbuild system anyhow)
Review of attachment 232799 [details] [review]: Looks good to me. Add a NEWS entry and push! \o/ ::: tests/eds/anti-linking.vala @@ +54,3 @@ + + /* Create a new backend (by name) each set up to guarantee we don't + * inherit state from the last test */ Perhaps add a ‘FIXME: bgo#690830’. ::: tests/eds/link-personas.vala @@ +70,3 @@ + + /* Create a new backend (by name) each set up to guarantee we don't + * inherit state from the last test */ Perhaps add a ‘FIXME: bgo#690830’. ::: tests/eds/linkable-properties.vala @@ +32,3 @@ + private bool _found_after_update; + + /* NOTE: each full name must be remain unique. Likewise for email. */ “must be remain”
commit ff6bec4f823587f9821827f396492a3fefd823f9 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Sat Jan 5 17:11:42 2013 -0800 Bug 690876 — eds: fix various test failures Note bug fixed by the last several commits Closes: https://bugzilla.gnome.org/show_bug.cgi?id=690876 NEWS | 1 + 1 file changed, 1 insertion(+) commit 9fea7b641ac1d470200677c11baf0e10d6c4ca4d Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Fri Jan 4 15:15:57 2013 -0800 eds test: don't assume Individuals remain after changing a linkable property tests/eds/set-emails.vala | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) commit 0e8f8f23b9502c0582d5fa1a4d815d4da0c5a43f Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Thu Jan 3 22:23:04 2013 -0800 eds test: create a new address book for each test to avoid inheriting state This fixes a bug where the second anti-linking test would fail if it ran after the first. tests/eds/anti-linking.vala | 18 +++++++++++++----- tests/eds/link-personas.vala | 3 ++- 2 files changed, 15 insertions(+), 6 deletions(-) commit 9de55174d7a7b4fa9dfbddc60e5490e27542acf0 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Thu Jan 3 21:55:19 2013 -0800 eds test: ensure basic anti-link removal works tests/eds/anti-linking.vala | 184 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 184 insertions(+) commit 3c269c3f4c10734b0f848f32f40074417d6cac78 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Thu Jan 3 09:42:57 2013 -0800 eds test: add basic anti-linking test tests/eds/Makefile.am | 5 ++ tests/eds/anti-linking.vala | 202 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 207 insertions(+) commit 9d6f5c89444c511c3c5e7f15089ab76d07d0b00f Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Wed Jan 2 16:42:38 2013 -0800 eds test: remove unnecessarily-strict test for extra Individual add/remove Ideally, we should be able to fulfill this test, but it's probably not worth the extra code complexity. tests/eds/linkable-properties.vala | 109 ------------------------------------- 1 file changed, 109 deletions(-) commit 8feed754cad27ad751fc1c6e569ec7de697443bc Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Sun Dec 30 12:17:21 2012 -0800 eds test: support setting email addresses in test backend tests/lib/eds/backend.vala | 10 ++++++++++ 1 file changed, 10 insertions(+) commit 597c93ff6bc5202c10ab6a543f272edd323ba840 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Sun Dec 30 12:10:47 2012 -0800 eds test: rename _parse_addrs to _parse_im_addrs for specificity. tests/lib/eds/backend.vala | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) commit de4dc1a33468ff5e0a9b0a5fbd88495452806932 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Sun Dec 30 12:08:49 2012 -0800 eds test: ensure changes to linkable properties aggregate Personas together tests/eds/linkable-properties.vala | 177 +++++++++++++++++++++++++++++++++++++ 1 file changed, 177 insertions(+) commit 01263791d2f827dd7f07cd3451441a26f01b36b2 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Sat Dec 29 18:43:22 2012 -0800 eds test: create a new address book for each test to avoid inheriting state This fixes a bug where the web services linking test would fail if it ran after the im address linking test. tests/eds/link-personas.vala | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) commit 7638419cc279f36886434344f391ca39b374a6a4 Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Fri Dec 28 09:31:57 2012 -0800 eds test: add test for specific behavior of linkable properties This splits out a subtle and complicated linkable-properties bug discovered by the set-emails test where changing the emails (which happen to be a linkable property in the EDS backend) causes an unnecessary Edsf.Persona creation and removal cycle. The set-* tests are meant to confirm that reading and writing the given property works, so seperating this should let us simplify those tests slightly yet focus on the secondary details we care about in a specific test case. tests/eds/Makefile.am | 5 ++ tests/eds/linkable-properties.vala | 175 +++++++++++++++++++++++++++++++++++++ tests/eds/set-emails.vala | 8 -- 3 files changed, 180 insertions(+), 8 deletions(-)