GNOME Bugzilla – Bug 655374
Un-break avatar tests
Last modified: 2011-08-17 18:17:59 UTC
These tests were recently broken with commit 3862f876, which added the avatar cache. The tests compare avatars with LoadableIcon.equal(), which eventually returns a URI (so the Individual's avatar in the cache isn't "equal" to the original avatar path). Sorry I didn't catch this in the first place.
Created attachment 192712 [details] [review] Un-break Tracker avatar tests Patches from branch: http://cgit.collabora.com/git/user/treitter/folks.git/log/?h=bgo655374-un-break-tracker-avatar-tests
Review of attachment 192712 [details] [review]: Looks fine apart from the one comment below. ::: folks/utils.vala @@ +86,3 @@ + public static async bool loadable_icons_content_equal (LoadableIcon a, + LoadableIcon b, + int icon_size) There's already a function, _avatars_equal(), in tests/folks/avatar-cache.vala which does this.
Created attachment 192828 [details] [review] Un-break avatar tests Patches from branch: http://cgit.collabora.com/git/user/treitter/folks.git/log/?h=bgo655374-un-break-tracker-avatar-tests-1b Note that the commits in the original branch are in-tact, but it's been rebased upon master (which necessitates some of the later commits).
*** Bug 655431 has been marked as a duplicate of this bug. ***
As Raúl pointed out on IRC, we should only manipulate our avatar cache for Edsf.Personas in response to changes from EDS (not immediately when a client sets the avatar on an Edsf.Persona). I'm working on a fix for this issue as well as the one raised in bug #655431 comment #1 and #655431 comment #2.
One comment about Utils.loadable_icons_content_equal: to compare avatars' content we read 2048-bytes chunks, compute a hash for each chunk and then compare the hashes... Isn't it cheaper to just do a byte-per-byte comparison of those two chunks? What is the point of computing a hash we'll immediately be throwing away when all we care about is if they are equal? Sorry if I am overlooking something, haven't had enough coffee yet :)
(In reply to comment #6) > One comment about Utils.loadable_icons_content_equal: to compare avatars' > content we read 2048-bytes chunks, compute a hash for each chunk and then > compare the hashes... Isn't it cheaper to just do a byte-per-byte comparison of > those two chunks? What is the point of computing a hash we'll immediately be > throwing away when all we care about is if they are equal? > > Sorry if I am overlooking something, haven't had enough coffee yet :) I should have reviewed all the branch before commenting.. this was addressed two commits later :)
Here is a branch that follows up on the issues previously discussed: http://cgit.collabora.com/git/user/rgs/folks/log/?h=unbreak-avatar
Created attachment 193344 [details] [review] Travis changes + new stuff according to the last comments
Review of attachment 193344 [details] [review]: Looking good. ::: backends/eds/lib/memory-icon.vala @@ +48,2 @@ /** + * The memory icon's data. Should probably add a comment saying that we don't care what format the data's in; it's just a bag of bytes as far as MemoryIcon's concerned. ::: folks/utils.vala @@ +95,3 @@ + * @since UNRELEASED + */ + public static async bool loadable_icons_content_equal (LoadableIcon a, Could this be internal rather than public? @@ +99,3 @@ + int icon_size) + { + bool retval = true; If (a == null && b != null), the function will return true. I think retval should be set to false by default. ::: tests/Makefile.am @@ +20,3 @@ +if ENABLE_TRACKER +SUBDIRS += tracker +endif Cunning. ::: tests/folks/avatar-cache.vala @@ +175,3 @@ } + protected void _assert_avatars_equal (LoadableIcon a, LoadableIcon b) Might be a good idea to move this into the folks-wide test library, so that it can be used by the eds tests as well.
Created attachment 193862 [details] [review] Updated patch to address Philip's comments I've rebased on top of master and also addressed a couple of review comments. The un-squashed branch is here: http://cgit.collabora.com/git/user/rgs/folks/log/?h=unbreak-avatar
Review of attachment 193862 [details] [review]: ::: backends/eds/lib/edsf-persona-store.vala @@ +872,3 @@ + throw new PersonaStoreError.INVALID_ARGUMENT ( + _("Unknown avatar type")); + } There should be no need to do a case split on the type of avatar. Avatar implements LoadableIcon, so just call avatar.load_async() to get an InputStream which you can read to get the inline data for the avatar. ::: backends/eds/lib/memory-icon.vala @@ +56,3 @@ + * @since UNRELEASED + */ + public uint8[] get_image_data () This shouldn't be necessary; use LoadableIcon.load_async() instead. ::: folks/utils.vala @@ +184,3 @@ + * @since UNRELEASED + */ + public static async bool loadable_icons_content_equal (LoadableIcon a, It might be tidier to have this in the (private) test utility library. I'm not sure it's useful as a general purpose function. ::: tests/tracker/add-persona.vala @@ +36,3 @@ private string _persona_iid; private LoadableIcon _avatar; + private string _avatar_path; This doesn't need to be a class member variable. ::: tests/tracker/set-avatar.vala @@ +30,3 @@ private IndividualAggregator _aggregator; private string _persona_fullname; + private string _avatar_path; This doesn't need to be a class member variable either.
Created attachment 194048 [details] [review] Squashed patch I've addressed what has been pointed out by Sir Philip and also made sure we ignore image types when comparing MemoryIcons. The non-squashed commits live here: http://cgit.collabora.com/git/user/rgs/folks/log/?h=unbreak-avatar
Review of attachment 194048 [details] [review]: ::: backends/eds/lib/edsf-persona-store.vala @@ +844,3 @@ + if (avatar == null) + { + var attr = contact.get_attribute ("PHOTO"); As per commit 4ed30316663e913ef1bbf293482b3d093c46aa4d, we should use “unowned VCardAttribute” instead of “var” for vCard attributes. @@ +873,3 @@ + } + + yield input_s.read_async (image_data); Why re-read the image? Unless I'm missing something, it should already be in image_data due to the memcpy()s above. ::: tests/lib/Makefile.am @@ +26,3 @@ noinst_LTLIBRARIES = libfolks-test.la +libfolks_test_la_SOURCES = test-case.vala test-utils.vala test-utils.vala is missing from the patch. :-(
(In reply to comment #14) > Review of attachment 194048 [details] [review]: > > ::: backends/eds/lib/edsf-persona-store.vala > @@ +844,3 @@ > + if (avatar == null) > + { > + var attr = contact.get_attribute ("PHOTO"); > > As per commit 4ed30316663e913ef1bbf293482b3d093c46aa4d, we should use “unowned > VCardAttribute” instead of “var” for vCard attributes. > Right, updated. > @@ +873,3 @@ > + } > + > + yield input_s.read_async (image_data); > > Why re-read the image? Unless I'm missing something, it should already be in > image_data due to the memcpy()s above. Oops - vestige of the previous incarnation of that code. > > ::: tests/lib/Makefile.am > @@ +26,3 @@ > noinst_LTLIBRARIES = libfolks-test.la > > +libfolks_test_la_SOURCES = test-case.vala test-utils.vala > > test-utils.vala is missing from the patch. :-( Oops.
Merged: commit b9bb49d6b032d930b666f42bb0f73ad8708d15e6 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> commit b9bb49d6b032d930b666f42bb0f73ad8708d15e6 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Wed Aug 17 15:48:39 2011 +0100 e-d-s: ignore the type when comparing images commit 7b595b5f4ae9326680866be204e5d15193b909a7 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Mon Aug 15 13:30:42 2011 +0100 e-d-s: update the Avatar Details test Now that we've reworked avatar handling and we've introduced an Avatar cache, the avatars associated to a newly created e-d-s contact might not be immediately available after the corresponding Edsf.Persona is created. Hence we need to listen to notifications for the avatar property. commit e3017a4bfc6a6940e2fb91dd8022ab8f817e9cbb Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Thu Aug 4 16:09:12 2011 +0100 e-d-s: Only set avatar if store_avatar () was successful Helps: https://bugzilla.gnome.org/show_bug.cgi?id=655374 commit 8f747c7361bea1eefbae5358df44b161c2f432c4 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Thu Aug 4 16:08:06 2011 +0100 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Thu Aug 4 16:09:12 2011 +0100 e-d-s: Only set avatar if store_avatar () was successful Helps: https://bugzilla.gnome.org/show_bug.cgi?id=655374 commit 8f747c7361bea1eefbae5358df44b161c2f432c4 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Thu Aug 4 16:08:06 2011 +0100 e-d-s: When an avatar has been removed, set it to null Though new_avatar was null, I prefer to even more obvious about what is going on. Helps: https://bugzilla.gnome.org/show_bug.cgi?id=655374 commit 5752726049d9b27af3083f9358a74a29a78e8061 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Thu Aug 4 15:55:09 2011 +0100 e-d-s: Don't infer we already have an avatar in our cache by its URI From libfolks we should discourage using local URIs to set an EContactPhoto so even if we get inlined EContactPhoto just do a full content comparison. Helps: https://bugzilla.gnome.org/show_bug.cgi?id=655374 commit 19ae7a8f60aa04dc0548a41e6c976c3654709c22 Author: Raul Gutierrez Segales <rgs@collabora.co.uk> Date: Thu Aug 4 15:44:28 2011 +0100 Don't manipulate the Avatar Cache before e-d-s acks changes Also, lets set EContactPhotos as INLINED because it doesn't make sense to use URIs for local files. In the future, when we support gravatars, we'll have to handle the case of setting ContactPhotos as URIs for those. Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=655374 commit d7797088fbb5dd1a480e5ffd3b51b406a2409d3d Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Thu Jul 28 10:08:09 2011 -0700 Do Tracker tests last, since they're most likely to fail uncontrollably. Helps: bgo#655374 - Un-break Tracker avatar tests commit 22a72f0579e37a6ddb113846eb373a5c8995cfed Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Tue Jul 26 15:53:02 2011 -0700 Add utility function to compare LoadableIcon content. Unlike the various "equal()" functions for LoadableIcon and related classes, this function actually compares image content (which can be necessary when testing vs. images in the avatar cache, which may have a different path than an otherwise-identical image). Helps: bgo#655374 - Un-break Tracker avatar tests