GNOME Bugzilla – Bug 655008
Tidy up error handling in eds backend
Last modified: 2011-08-03 23:58:58 UTC
Branch coming up to do that, now that we've got some shiny new error codes (bug #652425).
Created attachment 192339 [details] [review] Tidy up error handling in eds backend https://www.gitorious.org/folks/folks/commits/655008-eds-error-handling The following two tests fail with this branch, but I believe they were failing before anyway: • /PersonaStoreTests/persona store tests • /AddContactsStressTestTests/stress testing adding (150) contacts to e-d-s
(In reply to comment #1) > Created an attachment (id=192339) [details] [review] > Tidy up error handling in eds backend > > https://www.gitorious.org/folks/folks/commits/655008-eds-error-handling > > The following two tests fail with this branch, but I believe they were failing > before anyway: > • /PersonaStoreTests/persona store tests Could you provide a paste for the failing error? > • /AddContactsStressTestTests/stress testing adding (150) contacts to e-d-s The number 150 is absolutely nagical for this test, I settled down to that number after trying what was actually achievable <= 5 seconds :) We reaaaally need a more rigorous approach to for testing pushing contacts into e-d-s. Ideas?
(In reply to comment #2) > (In reply to comment #1) > > Created an attachment (id=192339) [details] [review] [details] [review] > > Tidy up error handling in eds backend > > > > https://www.gitorious.org/folks/folks/commits/655008-eds-error-handling > > > > The following two tests fail with this branch, but I believe they were failing > > before anyway: > > • /PersonaStoreTests/persona store tests > > Could you provide a paste for the failing error? It was due to me breaking the test suite with my groups changes, which was fixed by Travis a few days ago: http://git.gnome.org/browse/folks/commit/?id=dcc8a4b39ee357fbbf42b98bffb2d6dc848d5f51 > > • /AddContactsStressTestTests/stress testing adding (150) contacts to e-d-s > > The number 150 is absolutely nagical for this test, I settled down to that > number after trying what was actually achievable <= 5 seconds :) We reaaaally > need a more rigorous approach to for testing pushing contacts into e-d-s. > Ideas? How about resetting the timeout every time an added contact is successfully found in add-contacts-stress-test.vala:_individuals_changed_cb()? As you say, that test is a little temperamental: I ran it a few more times and it passed roughly half of the time.
(In reply to comment #3) > (In reply to comment #2) > > (In reply to comment #1) > > > Created an attachment (id=192339) [details] [review] [details] [review] [details] [review] > > > Tidy up error handling in eds backend > > > > > > https://www.gitorious.org/folks/folks/commits/655008-eds-error-handling > > > > > > The following two tests fail with this branch, but I believe they were failing > > > before anyway: > > > • /PersonaStoreTests/persona store tests > > > > Could you provide a paste for the failing error? > > It was due to me breaking the test suite with my groups changes, which was > fixed by Travis a few days ago: > http://git.gnome.org/browse/folks/commit/?id=dcc8a4b39ee357fbbf42b98bffb2d6dc848d5f51 > > > > • /AddContactsStressTestTests/stress testing adding (150) contacts to e-d-s > > > > The number 150 is absolutely nagical for this test, I settled down to that > > number after trying what was actually achievable <= 5 seconds :) We reaaaally > > need a more rigorous approach to for testing pushing contacts into e-d-s. > > Ideas? > > How about resetting the timeout every time an added contact is successfully > found in add-contacts-stress-test.vala:_individuals_changed_cb()? > Sounds like a way out. But tbh, I am not even sure what we are testing (measuring) here: how fast can we pump stuff into e-d-s? How longs does it take for created contacts to bounce back to folks? If the response time is constant or does it spike every once in a while?
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > (In reply to comment #1) > > > > Created an attachment (id=192339) [details] [review] [details] [review] [details] [review] [details] [review] > > > > Tidy up error handling in eds backend > > > > > > > > https://www.gitorious.org/folks/folks/commits/655008-eds-error-handling > > > > > > > > The following two tests fail with this branch, but I believe they were failing > > > > before anyway: > > > > • /PersonaStoreTests/persona store tests > > > > > > Could you provide a paste for the failing error? > > > > It was due to me breaking the test suite with my groups changes, which was > > fixed by Travis a few days ago: > > http://git.gnome.org/browse/folks/commit/?id=dcc8a4b39ee357fbbf42b98bffb2d6dc848d5f51 > > > > > > • /AddContactsStressTestTests/stress testing adding (150) contacts to e-d-s > > > > > > The number 150 is absolutely nagical for this test, I settled down to that > > > number after trying what was actually achievable <= 5 seconds :) We reaaaally > > > need a more rigorous approach to for testing pushing contacts into e-d-s. > > > Ideas? > > > > How about resetting the timeout every time an added contact is successfully > > found in add-contacts-stress-test.vala:_individuals_changed_cb()? > > > > Sounds like a way out. But tbh, I am not even sure what we are testing > (measuring) here: how fast can we pump stuff into e-d-s? How longs does it take > for created contacts to bounce back to folks? If the response time is constant > or does it spike every once in a while? I don't know; you wrote the test! I'm not sure that timing it is particularly useful at all. I was operating on the understanding that the test was designed to load eds up with requests as much as possible, and make sure that nothing broke (e.g. we handled any errors properly).
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (In reply to comment #2) > > > > (In reply to comment #1) > > > > > Created an attachment (id=192339) [details] [review] [details] [review] [details] [review] [details] [review] [details] [review] > > > > > Tidy up error handling in eds backend > > > > > > > > > > https://www.gitorious.org/folks/folks/commits/655008-eds-error-handling > > > > > > > > > > The following two tests fail with this branch, but I believe they were failing > > > > > before anyway: > > > > > • /PersonaStoreTests/persona store tests > > > > > > > > Could you provide a paste for the failing error? > > > > > > It was due to me breaking the test suite with my groups changes, which was > > > fixed by Travis a few days ago: > > > http://git.gnome.org/browse/folks/commit/?id=dcc8a4b39ee357fbbf42b98bffb2d6dc848d5f51 > > > > > > > > • /AddContactsStressTestTests/stress testing adding (150) contacts to e-d-s > > > > > > > > The number 150 is absolutely nagical for this test, I settled down to that > > > > number after trying what was actually achievable <= 5 seconds :) We reaaaally > > > > need a more rigorous approach to for testing pushing contacts into e-d-s. > > > > Ideas? > > > > > > How about resetting the timeout every time an added contact is successfully > > > found in add-contacts-stress-test.vala:_individuals_changed_cb()? > > > > > > > Sounds like a way out. But tbh, I am not even sure what we are testing > > (measuring) here: how fast can we pump stuff into e-d-s? How longs does it take > > for created contacts to bounce back to folks? If the response time is constant > > or does it spike every once in a while? > > I don't know; you wrote the test! I'm not sure that timing it is particularly > useful at all. I was operating on the understanding that the test was designed > to load eds up with requests as much as possible, and make sure that nothing > broke (e.g. we handled any errors properly). Right - that was part of original intend. I think we could just drop the timeout for this test since we can't expect it to finish in a given amount of time and we want to avoid guessing numbers. As long as each e-d-s contact that is created triggers the appearance of a Persona, we should be good to go.
Created attachment 193145 [details] [review] Don't set a timeout for add contacts stress test
Comment on attachment 192339 [details] [review] Tidy up error handling in eds backend >+ throw new PersonaStoreError.STORE_OFFLINE ( >+ "Address book '%s' is offline, " + >+ "so contact '%s' cannot be removed.", >+ this.id, persona.uid); I am guessing this error message is translatable? >+ case ClientError.PERMISSION_DENIED: >+ throw new PersonaStoreError.PERMISSION_DENIED ( >+ "Permission denied to remove contact '%s': %s", >+ persona.uid, e.message); Ditto. >+ case ClientError.NOT_SUPPORTED: >+ throw new PersonaStoreError.READ_ONLY ( >+ "Removing contacts isn't supported by this " + >+ "persona store: %s", e.message); And the same. >+ /* Fallback error. */ >+ throw new PersonaStoreError.REMOVE_FAILED ( >+ "Can't remove contact '%s': %s", persona.uid, e.message); Make it translatable. >+ case ClientError.REPOSITORY_OFFLINE: >+ throw new PersonaStoreError.STORE_OFFLINE ( >+ "Address book '%s' is offline.", this.id); Make it translatable. >+ case ClientError.PERMISSION_DENIED: >+ throw new PersonaStoreError.PERMISSION_DENIED ( >+ "Permission denied to open address book '%s': %s", >+ this.id, e1.message); Make it translatable. >+ if (got_view == false) >+ { >+ throw new PersonaStoreError.INVALID_ARGUMENT ( >+ "Couldn't get book view\n"); >+ } Spurious \n.
Created attachment 193205 [details] [review] Tidy up error handling in eds backend (updated) https://www.gitorious.org/folks/folks/commits/655008-eds-error-handling Good catches. Made all the strings translatable, added translator comments, added the file to POTFILES.in and, most importantly, Unicode-ified the quotation marks in the strings.
Review of attachment 193145 [details] [review]: Looks OK to me apart from this minor style point. ::: tests/eds/add-contacts-stress-test.vala @@ +59,3 @@ + this._contacts_found = new HashTable<string, bool> (str_hash, str_equal); + for (var i=0; i<this._contacts_cnt; i++) Need spaces around the ‘=’ and ‘<’.
(In reply to comment #9) > Created an attachment (id=193205) [details] [review] > Tidy up error handling in eds backend (updated) > > https://www.gitorious.org/folks/folks/commits/655008-eds-error-handling > > Good catches. Made all the strings translatable, added translator comments, > added the file to POTFILES.in and, most importantly, Unicode-ified the > quotation marks in the strings. After what we've discussed on IRC and you've subsequently fixed, this looks good to be committed. Thanks!
Comment on attachment 193205 [details] [review] Tidy up error handling in eds backend (updated) Committed, thanks. commit d0e2285d8b82bdd8c314751a6a294c1ac457f208 Author: Philip Withnall <philip@tecnocode.co.uk> Date: Wed Jul 20 22:08:00 2011 +0100 Bug 655008 — Tidy up error handling in eds backend Return more specific error codes from Edsf.PersonaStore methods where possible, and in all cases make sure we've got a switch statement on the incoming error code. Closes: bgo#655008 NEWS | 1 + backends/eds/lib/edsf-persona-store.vala | 251 +++++++++++++++++++++++++----- po/POTFILES.in | 1 + 3 files changed, 212 insertions(+), 41 deletions(-)
Review of attachment 193145 [details] [review]: Committed: commit 5beccf217070b20abccb8485b44ca6b4fcc02af4 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Wed Aug 3 10:13:42 2011 +0100 Don't set a timeout for contacts adding stress test Helps: https://bugzilla.gnome.org/show_bug.cgi?id=655008 tests/eds/add-contacts-stress-test.vala | 29 ++++++++++++++++------------- 1 files changed, 16 insertions(+), 13 deletions(-)
Committed: commit 5beccf217070b20abccb8485b44ca6b4fcc02af4 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Wed Aug 3 10:13:42 2011 +0100 Don't set a timeout for contacts adding stress test Helps: https://bugzilla.gnome.org/show_bug.cgi?id=655008 tests/eds/add-contacts-stress-test.vala | 29 ++++++++++++++++------------- 1 files changed, 16 insertions(+), 13 deletions(-)