This bug blocks https://bugs.meego.com/show_bug.cgi?id=16787

Punting bugs that won't be fixed by Folks 0.6.0.

I've started a very basic dummy backend here: http://cgit.collabora.com/git/user/jwhiting/folks.git/log/?h=dummybackend and will continue working on it there.  If you have any feedback, let me know.

(In reply to comment #3)
> I've started a very basic dummy backend here:
> http://cgit.collabora.com/git/user/jwhiting/folks.git/log/?h=dummybackend and
> will continue working on it there.  If you have any feedback, let me know.

I spotted this:
> /* FIXME: load personas from a file or something. */
which means we should probably expand on the use case for the dummy backend. I think one of the big requirements for it is to allow Personas to be added/removed/have their properties changed in the backend at runtime, so that we can easily write unit tests for folks’ dynamic behaviour. (e.g. Testing how libfolks re-links personas from the dummy backend in response to some of their linkable properties changing at runtime.)

I once started writing a dummy backend which implemented a D-Bus interface to allow these kinds of changes (e.g. AddPersona, AddPersonaStore, ChangePersonaProperty methods), and that felt like a reasonable approach. Unit tests would then call the D-Bus methods and watch for the expected changes coming back through libfolks. I didn’t get too far with my branch, though, so there may be problems with this approach that I didn’t run into.

This looks like a good start though!

I added small unit tests for ImFieldDetails and WebServiceFieldDetails.  Next will start adding support for the other *Details interfaces along with unit tests for those interfaces.  Are the unit tests I've created for alias, imfield, and webservice detailed enough? or do they need to have more added to them?  If so, what needs to be added?

(In reply to comment #5)
> I added small unit tests for ImFieldDetails and WebServiceFieldDetails.  Next
> will start adding support for the other *Details interfaces along with unit
> tests for those interfaces.  Are the unit tests I've created for alias,
> imfield, and webservice detailed enough? or do they need to have more added to
> them?  If so, what needs to be added?

They're a good starting point. See the EDS tests for an idea of the coverage we want for the various interfaces.

I think for each member, we try to check:

* non-existence of the final value
* (sometimes: checking for the existence of an expected initial value)
* set the final value
* after appropriate async functions return/signals emit: final value meets expectations

Here are my current thoughts so far:

Required for new files which contain translations:

diff --git a/po/POTFILES.in b/po/POTFILES.in
index 20b9f9c..e43fa09 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -1,4 +1,5 @@
 [encoding: UTF-8]
+backends/dummy/lib/dummy-persona.vala
 backends/eds/lib/edsf-persona-store.vala
 backends/key-file/kf-backend-factory.vala
 backends/key-file/kf-persona-store.vala
diff --git a/po/POTFILES.skip b/po/POTFILES.skip
index a27b962..885042c 100644
--- a/po/POTFILES.skip
+++ b/po/POTFILES.skip
@@ -1,3 +1,4 @@
+backends/dummy/lib/dummy-persona.c
 backends/eds/lib/edsf-persona-store.c
 backends/key-file/kf-backend-factory.c
 backends/key-file/kf-persona-store.c


backends/dummy/dummy-backend.vala:

+          if (!this._is_prepared || this._prepare_pending == true)
+            {
+              return;
+            }

The second half of this branch seems like a bad idea -- in case we're part-way into preparing, we just give up the client's request to unprepare?

Similarly, this code later in the function seems wrong, since we're trying to tear down, not build up:

+              this._prepare_pending = true;

I see the point of these two snippets taken together, but it seems like this requires a new _unprepare_pending in both cases and to factor that into the code that uses _prepare_pending. This all sounds pretty ugly though :-/

Did you copy this code from elsewhere? We might need to fix that as well.


add_persona_from_details() needs to persist the changes to disk


backends/dummy/dummy-persona.vala:

The change_* functions also need to persist to disk


tests/folks/specific-field-details.vala:

+  private Dummy.Persona? _test_persona;
+  

Unnecessary whitespace on the second line

+      this._persona_store.prepare();
+      

Same here

+              this._test_persona.alias = new_alias;
+              

Same here


tests/folks/specific-field-details.vala



+              this._test_persona.alias = new_alias;
+              
+              /* Check the persona's alias has changed */
+              assert(this._test_persona.alias == new_alias);

This just checks that alias.set() function doesn't do something really silly. It'd be better to yield (or equivalent) upon the change_alias() function, since its changes should be written to disk by the time it's done.

This also applies to all the others tests, upon brief glimpse.


You might want to split test_alias_replacement() into a sync and async function (with most of the code here) to take advantage of the yield for all the called async functions. See some of the other tests.


+              this._test_persona = 
+                (Dummy.Persona) this._persona_store.add_persona_from_details.en

Trailing whitespace on both lines

+              assert(this._persona_store.personas.size == 0);
+      

Trailing whitespace on the second line


+      string path = "/usr/share/pixmaps/nobody.png";

We should use the avatar we have bundled, which we can guarantee exists: tests/data/avatar-01.jpg

Travis,

Yes, the _prepare_pending is coming straight from the keyfile backend I created the dummy backend from.  Should the _prepare_pending stuff just be removed and if we are already prepared we return, otherwise we try to prepare?  I can fix in both dummy and keyfile backend if that's the case.

Thanks for the feedback.  I'll fix the other bits also.

For what it’s worth, here’s my current WIP on this:

https://www.gitorious.org/folks/folks/commits/648811-dummy-backend-attempt3

It’s almost there. As far as I remember, the code is complete and most things work. It just needs testing (and test cases)…and rebasing against master.

I hope to get time to do this at some point. Maybe once I finish this darned degree of mine.

this link is broken could you paste a new link, I want to try it and if possible implement the missing tests.

(In reply to comment #10)
> this link is broken could you paste a new link, I want to try it and if
> possible implement the missing tests.

Whoops. I have an automatic git mirror script on a cronjob which deleted the branch. I’ve pushed it here instead:
https://git.gnome.org/browse/folks/log/?h=648811-dummy-backend-attempt3

Let me know if you have any questions. Thanks for taking a look at this!

(In reply to comment #11)
> I’ve pushed it here instead:
> https://git.gnome.org/browse/folks/log/?h=648811-dummy-backend-attempt3

Or even here:
https://git.gnome.org/browse/folks/log/?h=648811-dummy-backend-non-dbus

(In reply to comment #12)
> (In reply to comment #11)
> > I’ve pushed it here instead:
> > https://git.gnome.org/browse/folks/log/?h=648811-dummy-backend-attempt3
> 
> Or even here:
> https://git.gnome.org/browse/folks/log/?h=648811-dummy-backend-non-dbus

Apparently there is a missing directory in the branch. You did some references to it on "root/tests/lib/Makefile.am". The directory is "root/tests/lib/dummy"

Any special reason to use the namespace Dummy"f"?? or is that a typo??

(In reply to comment #13)
> Apparently there is a missing directory in the branch. You did some references
> to it on "root/tests/lib/Makefile.am". The directory is "root/tests/lib/dummy"

I guess I forgot to commit it to git. Unfortunately, I’ve done a `git clean -fxd` since then, so the directory’s lost. :-(

(In reply to comment #14)
> Any special reason to use the namespace Dummy"f"?? or is that a typo??

It follows the same naming scheme as the rest of the public folks backend libraries (e.g. Kf, Tpf, Trf, …). I think that scheme (‘f’ for ‘folks’) was originally chosen so that we didn’t collide with the Tp namespace. I’m not particularly attached to it, though it would do no harm to leave it.

(In reply to comment #15)
> It follows the same naming scheme as the rest of the public folks backend
> libraries (e.g. Kf, Tpf, Trf, …). I think that scheme (‘f’ for ‘folks’) was
> originally chosen so that we didn’t collide with the Tp namespace. I’m not
> particularly attached to it, though it would do no harm to leave it.

Ok, understood. I will rename it back to Dummyf. Thanks

BTW, I am working on this repository: https://github.com/renatofilho/folks/tree/dummy

Created attachment 257866 [details] [review]
Update code to works with the source code from mainline

Created attachment 258072 [details] [review]
Add pc file for dummy backend library

Created attachment 258073 [details] [review]
Initial unit test for dummy backend

The test is not working fine I am getting the following message:
tests/dummy/.libs/lt-individual-retrieval:15762): folks-WARNING **: Failed to find primary PersonaStore with type ID 'dummy-store' and ID ''.
Individuals will not be linked properly and creating new links between Personas will not work.
The configured primary PersonaStore's backend may not be installed. If you are unsure, check with your distribution.
Trace/breakpoint trap
cleaning up pid 15777


I am not sure how to fix that, could you help me?

Created attachment 258129 [details] [review]
New tests for dummy backend

(In reply to comment #17)
> Created an attachment (id=257866) [details] [review]
> Update code to works with the source code from mainline

This doesn’t apply for me on the gitorious branch I gave in comment #12, or on the rebased version of it.

Could you please rebase your git branch on this branch:
    https://git.gnome.org/browse/folks/log/?h=648811-dummy-backend-non-dbus-rebase1
which I’ve just rebased on master (as of today). Could you then please push your branch (with various fixup changes applied on top of my branch) to github or somewhere else? Then I can try it out and start reviewing.

Thanks. :-)

(In reply to comment #21)
> (In reply to comment #17)
> > Created an attachment (id=257866) [details] [review] [details] [review]
> > Update code to works with the source code from mainline
> 
> This doesn’t apply for me on the gitorious branch I gave in comment #12, or on
> the rebased version of it.
> 
> Could you please rebase your git branch on this branch:
>    
> https://git.gnome.org/browse/folks/log/?h=648811-dummy-backend-non-dbus-rebase1
> which I’ve just rebased on master (as of today). Could you then please push
> your branch (with various fixup changes applied on top of my branch) to github
> or somewhere else? Then I can try it out and start reviewing.
> 
> Thanks. :-)


Done: https://github.com/renatofilho/folks2/tree/dummy

Created attachment 259172 [details] [review]
dummy: Add a dummy backend

This is a new backend targeted at making libfolks more testable. It is
designed to be used in internal libfolks unit tests, allowing the test
driver code to manipulate the backing store state to test how that
affects front-end state in libfolks. For example, it will be useful in
more thoroughly testing the IndividualAggregator.

It includes work by Renato Araujo Oliveira Filho <renatox@gmail.com>.

The backend may also be useful for testing libfolks clients,
such as contacts UIs, by providing an easy way to create well-known
personas to appear in the the UI. Hence, it is an installed backend, and
is loaded by default (although creates no Backend or PersonaStore instances
unless API calls are made).

It is designed to allow delay and error injection into all major
PersonaStore operations, and to allow any kind of Persona implementation
to be returned from the store.

It includes a few small test cases to sanity-check that the backend
works, but no thorough self-test-coverage.

Full API documentation is included and installed by default. The API is
not currently stable, so while external projects may start to consume
it, they should be prepared for breakage in the future as the API
stabilises. No timescale is given for such stabilisation.

This commit does not include any changes to the core libfolks unit
tests, but future changes could be made to port them to use the dummy
backend.

New API:
 • libfolks-dummy.la mocking library and all its symbols

Here we go. This patch is all updated, rebased on master as of today, and ready to be pushed (as far as I’m concerned).

I dispensed with porting the existing core folks tests to the dummy backend, and will work on them separately. I don’t want that work to block on getting the backend itself committed.

Thanks for your work on this Renato!

I should mention that this is available as a series of fixup patches here: https://git.gnome.org/browse/folks/log/?h=648811-dummy-backend-rebase1 which might make reviewing a little easier (or perhaps not).

Created attachment 259176 [details] [review]
dummy: Add a dummy backend

This is a new backend targeted at making libfolks more testable. It is
designed to be used in internal libfolks unit tests, allowing the test
driver code to manipulate the backing store state to test how that
affects front-end state in libfolks. For example, it will be useful in
more thoroughly testing the IndividualAggregator.

It includes work by Renato Araujo Oliveira Filho <renatox@gmail.com>.

The backend may also be useful for testing libfolks clients,
such as contacts UIs, by providing an easy way to create well-known
personas to appear in the the UI. Hence, it is an installed backend, and
is loaded by default (although creates no Backend or PersonaStore instances
unless API calls are made).

It is designed to allow delay and error injection into all major
PersonaStore operations, and to allow any kind of Persona implementation
to be returned from the store.

It includes a few small test cases to sanity-check that the backend
works, but no thorough self-test-coverage.

Full API documentation is included and installed by default. The API is
not currently stable, so while external projects may start to consume
it, they should be prepared for breakage in the future as the API
stabilises. No timescale is given for such stabilisation.

This commit does not include any changes to the core libfolks unit
tests, but future changes could be made to port them to use the dummy
backend.

New API:
 • libfolks-dummy.la mocking library and all its symbols

Updated with some fixes from Renato.

Review of attachment 259172 [details] [review]:

::: backends/dummy/Makefile.am
@@ +1,1 @@
+SUBDIRS = lib

Doesn't this need to be "SUBDIRS = lib ." so lib gets compiled first?

::: backends/dummy/lib/Makefile.am
@@ +80,3 @@
+	$(AM_LDFLAGS) \
+	$(CODE_COVERAGE_LDFLAGS) \
+	-version-info "$(LT_CURRENT)":"$(LT_REVISION)":"$(LT_AGE)" \

This gives it the same SONAME as the main library, which seems premature if it isn't stable.

Maybe use "-release $(VERSION)" for now, so it deliberately breaks SONAME in every release?

::: backends/dummy/lib/dummy-fat-persona.vala
@@ +204,3 @@
+    }
+
+  private HashSet<PhoneFieldDetails> _phone_numbers =

Might be worth trying SmallSet once you have a regression test to profile? Not a blocker, obviously.

@@ +714,3 @@
+      foreach (var k in input_multi_map.get_keys ())
+        {
+          var values = input_multi_map.get (k);

Not a merge blocker, but it would be good to get this using a MapIter after merge - realistically, people are going to copy/paste the dummy backend all over the place, and iterating a MultiMap in this style has pessimal performance. Iterating a SmallMultiMap like this is particularly bad.

@@ +1093,3 @@
+    {
+      /* Make sure it includes our local ID. */
+      local_ids.add (this.iid);

It seems pretty strange that this alters its parameter. Since this is only test code, I think it'd be OK to just document it.

::: backends/dummy/lib/dummy-persona-store.vala
@@ +837,3 @@
+   * Negative delays result in the call completing synchronously, zero delays
+   * result in completion in an idle callback, and positive delays result in
+   * completion after that many milliseconds.

I assume this is necessary for historical reasons, because Folks doesn't specify whether prepare can be synchronous? With hindsight, it'd be good for prepare to be GAsyncResult-based.

I would be unhappy to see real regression tests that need to delay for a positive time.

I'd be tempted to have an API more like this (pseudocode):

    void mock_prepare (PrepareCall prepare_call)
    {
#if 0
      prepare_call.succeed ();
#elif 0
      prepare_call.fail (error);
#else
      idle_add (lambda: prepare_call.succeed());
#endif
    }

but that can wait til a subsequent commit, if it's needed at all.

I'd say "use GTask", but GTask always returns async, which it seems is exactly what you're trying to avoid here.

::: tests/Makefile.am
@@ +32,3 @@
 	tools \
 	folks \
+    dummy \

Indentation is weird here

Created attachment 259191 [details] [review]
dummy: Add a dummy backend

This is a new backend targeted at making libfolks more testable. It is
designed to be used in internal libfolks unit tests, allowing the test
driver code to manipulate the backing store state to test how that
affects front-end state in libfolks. For example, it will be useful in
more thoroughly testing the IndividualAggregator.

It includes work by Renato Araujo Oliveira Filho <renatox@gmail.com>.

The backend may also be useful for testing libfolks clients,
such as contacts UIs, by providing an easy way to create well-known
personas to appear in the the UI. Hence, it is an installed backend, and
is loaded by default (although creates no Backend or PersonaStore instances
unless API calls are made).

It is designed to allow delay and error injection into all major
PersonaStore operations, and to allow any kind of Persona implementation
to be returned from the store.

It includes a few small test cases to sanity-check that the backend
works, but no thorough self-test-coverage.

Full API documentation is included and installed by default. The API is
not currently stable, so while external projects may start to consume
it, they should be prepared for breakage in the future as the API
stabilises. No timescale is given for such stabilisation.

This commit does not include any changes to the core libfolks unit
tests, but future changes could be made to port them to use the dummy
backend.

New API:
 • libfolks-dummy.la mocking library and all its symbols

Thanks for the fast review!

(In reply to comment #28)
> ::: backends/dummy/Makefile.am
> @@ +1,1 @@
> +SUBDIRS = lib
> 
> Doesn't this need to be "SUBDIRS = lib ." so lib gets compiled first?

True, although it was working before. Fixed.

> ::: backends/dummy/lib/Makefile.am
> @@ +80,3 @@
> +    $(AM_LDFLAGS) \
> +    $(CODE_COVERAGE_LDFLAGS) \
> +    -version-info "$(LT_CURRENT)":"$(LT_REVISION)":"$(LT_AGE)" \
> 
> This gives it the same SONAME as the main library, which seems premature if it
> isn't stable.
> 
> Maybe use "-release $(VERSION)" for now, so it deliberately breaks SONAME in
> every release?

I’ve attached a patch to bug #711544 to add separate LT versioning for each backend library. Might as well fix this properly.

> ::: backends/dummy/lib/dummy-fat-persona.vala
> @@ +204,3 @@
> +    }
> +
> +  private HashSet<PhoneFieldDetails> _phone_numbers =
> 
> Might be worth trying SmallSet once you have a regression test to profile? Not
> a blocker, obviously.

I’ll punt this for now.

> @@ +714,3 @@
> +      foreach (var k in input_multi_map.get_keys ())
> +        {
> +          var values = input_multi_map.get (k);
> 
> Not a merge blocker, but it would be good to get this using a MapIter after
> merge - realistically, people are going to copy/paste the dummy backend all
> over the place, and iterating a MultiMap in this style has pessimal
> performance. Iterating a SmallMultiMap like this is particularly bad.

Fixed. Good catch.

> @@ +1093,3 @@
> +    {
> +      /* Make sure it includes our local ID. */
> +      local_ids.add (this.iid);
> 
> It seems pretty strange that this alters its parameter. Since this is only test
> code, I think it'd be OK to just document it.

I dropped it instead — it doesn’t make sense anyway, since local-ids is for linking to other personas, so including the current persona’s IID in there creates a pointless reflexive link.

> ::: backends/dummy/lib/dummy-persona-store.vala
> @@ +837,3 @@
> +   * Negative delays result in the call completing synchronously, zero delays
> +   * result in completion in an idle callback, and positive delays result in
> +   * completion after that many milliseconds.
> 
> I assume this is necessary for historical reasons, because Folks doesn't
> specify whether prepare can be synchronous? With hindsight, it'd be good for
> prepare to be GAsyncResult-based.

prepare() _is_ GAsyncResult-based, so it should never complete synchronously, I think (although I haven’t actually checked what code Vala generates). A negative delay results in a single idle callback, and a zero delay results in two then, I suppose.

> I would be unhappy to see real regression tests that need to delay for a
> positive time.

True, but I think it’s necessary. The use case is testing fallback timeouts in folks.

> I'd be tempted to have an API more like this (pseudocode):

Not quite sure what you mean by this.

> I'd say "use GTask", but GTask always returns async, which it seems is exactly
> what you're trying to avoid here.

Yes, and it would break API. :-)

> ::: tests/Makefile.am
> @@ +32,3 @@
>      tools \
>      folks \
> +    dummy \
> 
> Indentation is weird here

Fixed.

Does this need further review before committing?

(In reply to comment #31)
> Does this need further review before committing?

Please. I’d appreciate one final round of API sanity checking and high-level thought before it goes in.

Created attachment 262960 [details] [review]
dummy: Add a dummy backend

This is a new backend targeted at making libfolks more testable. It is
designed to be used in internal libfolks unit tests, allowing the test
driver code to manipulate the backing store state to test how that
affects front-end state in libfolks. For example, it will be useful in
more thoroughly testing the IndividualAggregator.

It includes work by Renato Araujo Oliveira Filho <renatox@gmail.com>.

The backend may also be useful for testing libfolks clients,
such as contacts UIs, by providing an easy way to create well-known
personas to appear in the the UI. Hence, it is an installed backend, and
is loaded by default (although creates no Backend or PersonaStore instances
unless API calls are made).

It is designed to allow delay and error injection into all major
PersonaStore operations, and to allow any kind of Persona implementation
to be returned from the store.

It includes a few small test cases to sanity-check that the backend
works, but no thorough self-test-coverage.

Full API documentation is included and installed by default. The API is
not currently stable, so while external projects may start to consume
it, they should be prepared for breakage in the future as the API
stabilises. No timescale is given for such stabilisation.

This commit does not include any changes to the core libfolks unit
tests, but future changes could be made to port them to use the dummy
backend.

New API:
 • libfolks-dummy.la mocking library and all its symbols

Created attachment 262961 [details] [review]
tests: Add a standalone-individuals test program

This contains a few test cases which check folks’ behaviour when
manually creating Individuals *outside* an IndividualAggregator. These
should catch the problems found in the following two bugs:
 • https://bugzilla.redhat.com/show_bug.cgi?id=1031252
 • https://bugzilla.gnome.org/show_bug.cgi?id=712839
and hopefully fixed by commits:
 • f00534294d7d52ac7e37dfaa075e3465b7755483
 • 1ec050efc4f7135e9958c74da2028daf669077a0

Created attachment 262962 [details] [review]
individual: Add default values for PresenceDetails properties

If an Individual is created with no Personas (as in the
standalone-individuals test cases), the PresenceDetails properties
should be initialised to empty strings. They were previously defaulting
to null.

Created attachment 262963 [details] [review]
individual: Allow Individual.id to be an empty string with no personas

If an Individual has no Personas (as can happen when it’s just been
constructed), document the ID as being the empty string. This is a
special case.

Previously, the ID defaulted to being null, which violated its specified
type.

Review of attachment 262960 [details] [review]:

General
-------
First, thanks for all the work on this. It's a giant effort and it will really
be useful to have in place.

* could we set a different version number for the dummy backend/library (eg, 0.1
  for the time being) and use clear messaging in releases to indicate that any
  API stability announcements do not apply to it? Though, I would really like to
  even get the dummy API stable by 1.0, so maybe it's OK as long as we've got
  separate soname numbering for it until 1.0 (at which point I think it'd be
  obnoxious /not/ to have them all unified, as that would lead to really ugly
  package numbering for at least Debian and derivatives)?

* update the NEWS changes to be in the 0.9.7 section

* as far as the broken persona-store test, you can merge this code then fix
  that. It must be fixed before the next release though. And we should do that
  relatively soon after since this is a big change worth getting out.

* in general, we don't have a common convention on the new symbols to say "these
  are special for the Dummy backend", eg, for changing the state of the Personas
  in the fake backing store. Maybe it's not necessary, but I know that it would
  even take me a little while to pick out exactly which of the new members are
  specifically for unique Dummy Backend functionality. We are certainly the
  primary audience for it, but it seems like it would be extra-difficult for
  anyone not already very familiar with the Folks API to use the dummy backend.
  I don't think every symbol needs a common prefix, but maybe a handful that we
  use consistently so any tutorial or we helping out other people could just say
  "you're looking for the symbols that start with "foo_", "bar_", or "baz_".

* please file a bug for "rebase tests/folks/* on dummy backend to remove EDS and
  key-file dependencies" and one to test a number of delays for properties and
  errors with persona adds/removes (using the mock functions) to make sure we
  don't break anything in the core with those types of issues. Having some tests
  with very long delays would also be a good time to split out long-running
  tests as a config option (to at least make sure they work before releases but
  not usually run during regular development).

* longer-term, we might want to remove the key-file backend before 1.0 since
  it's not terribly useful at this point

dummy/lib/dummy-backend.vala
----------------------------
set_persona_stores(), prepare(), unprepare():
* you use {freeze, thaw}_notify() to avoid extra notifications here. Is there
  any reason we shouldn't do this in all the other backends? If not, please open
  a bug for that

register_persona_stores():
* the docs claim that all stores must be a FolksDummy.PersonaStore (or subclass)
  but we don't actually enforce it. At the least, we should warn if this isn't
  met, maybe throw an Error

dummy/lib/dummy-fat-persona.vala
--------------------------------
* I think it would make more sense to call this something like SuperPersona,
  MaxPersona, or FullPersona to suggest it implements all interfaces. "Fat"
  makes me think it just implements "many" or always has a lot of preloaded
  attributes or something.

* would it make sense to use a different verb for the update_*() functions? It
  wasn't immediately obvious based on name that they were intended to make
  changes as if they happened in the backing store. What if we prepend
  "backend_" or "store_" to these functions to make it clear that they're
  simulating changes which would normally happen outside of our control? I
  wonder if it'd be cleaner to make them functions on the parent PersonaStore,
  but I don't have a strong opinion either way

dummy/lib/dummy-persona-store.vala
----------------------------------
* all the MaybeBool properties seem to default to FALSE. Would it make sense for
  them to default to UNSET, leaving it up to the caller to set up the details as
  it cares about them? I guess FALSE is a reasonable default for most cases
  though.

* It would be a little nice to wrap operations on _personas_changed_frozen in
  functions, so it can assert if we ever try to decrease it while it's <= 0
  (particularly since we mostly check whether it exactly equals 0)

+              else if (k == Folks.PersonaStore.detail_key (
... // repeat 30 times or more
There's so much repetition in this block. It's probably not worth changing but I
guess there isn't a great way to factor that pattern.

* as far as I can tell, we don't actually call the *_mock() functions from any
  tests. The code all looks right but I'd be a lot more confident if we
  exercised that code (at least one of each ranges of timeouts and with/without
  throwing an Error for a mock function)

add_persona_from_details_mock()
* the docs say that a negative return value in the delegate will have it return
  "synchronously". Is that actually true? It seems to just return immediately,
  and it's from an async function. I guess that's technically true but it makes
  it sound like it's somehow blocking for some amount of time.

* for all the other *_mock() functions, just have their docs refer to the first
  one's because otherwise we've got a lot of aliased text. Same for their
  delegates.

set_capabilities():
* I think we should probably have this accept a Map of well-known keys and their
  new values. Otherwise we could paint ourselves into a corner.

freeze_personas_changed():
* this functionality seems fairly useful for testing purposes. Is there any
  reason we don't have an equivalent for Persona details themselves (other than
  it being a lot hairier to implement)?

reach_quiescence():
* the docs should emphasize that it can only achieve quiescence after this
  function has been called, since that's normally something that clients can
  just expect to happen

update_is_user_set_default()
* Jonny would have a field day with this name but I think it's fine (other than
  the note above about update_*() functions)

dummy/lib/folks-dummy-uninstalled.pc.in
---------------------------------------
* this depends upon libebook and libedataserver — should it?

Review of attachment 262961 [details] [review]:

This mostly looks good except for a trivial issue:

+      /* FIXME: assert (individual.display_name == "");*/

Just resolve that and push after the dummy backend is merged.

Review of attachment 262962 [details] [review]:

Looks good. No need to wait to merge.

Review of attachment 262963 [details] [review]:

Looks good. No need to wait to merge.

Attachment 262962 [details] pushed as 1cff44a - individual: Add default values for PresenceDetails properties
Attachment 262963 [details] pushed as c3ace8c - individual: Allow Individual.id to be an empty string with no personas

Created attachment 263048 [details] [review]
dummy: Add a dummy backend

This is a new backend targeted at making libfolks more testable. It is
designed to be used in internal libfolks unit tests, allowing the test
driver code to manipulate the backing store state to test how that
affects front-end state in libfolks. For example, it will be useful in
more thoroughly testing the IndividualAggregator.

It includes work by Renato Araujo Oliveira Filho <renatox@gmail.com>.

The backend may also be useful for testing libfolks clients,
such as contacts UIs, by providing an easy way to create well-known
personas to appear in the the UI. Hence, it is an installed backend, and
is loaded by default (although creates no Backend or PersonaStore instances
unless API calls are made).

It is designed to allow delay and error injection into all major
PersonaStore operations, and to allow any kind of Persona implementation
to be returned from the store.

It includes a few small test cases to sanity-check that the backend
works, but no thorough self-test-coverage.

Full API documentation is included and installed by default. The API is
not currently stable, so while external projects may start to consume
it, they should be prepared for breakage in the future as the API
stabilises. No timescale is given for such stabilisation.

This commit does not include any changes to the core libfolks unit
tests, but future changes could be made to port them to use the dummy
backend.

New API:
 • libfolks-dummy.la mocking library and all its symbols

Created attachment 263050 [details] [review]
tests: Add a standalone-individuals test program

This contains a few test cases which check folks’ behaviour when
manually creating Individuals *outside* an IndividualAggregator. These
should catch the problems found in the following two bugs:
 • https://bugzilla.redhat.com/show_bug.cgi?id=1031252
 • https://bugzilla.gnome.org/show_bug.cgi?id=712839
and hopefully fixed by commits:
 • f00534294d7d52ac7e37dfaa075e3465b7755483
 • 1ec050efc4f7135e9958c74da2028daf669077a0

Created attachment 263051 [details] [review]
backends: Add [freeze|thaw]_notify() calls to [un]prepare() in backends

Reduce signal duplication. Inspired by the dummy backend.

Updated branch here with fixup commits: http://cgit.collabora.com/git/user/pwith/folks.git/log/?h=648811-dummy-backend-final

Note it’s now based on the branch for bug #711544 (http://cgit.collabora.com/git/user/pwith/folks.git/log/?h=backend-lt-versioning).

I can’t upload any updated patches at the moment because Bugzilla doesn’t seem to be behaving properly.

(In reply to comment #37)
> * could we set a different version number for the dummy backend/library (eg,
> 0.1
>   for the time being) and use clear messaging in releases to indicate that any
>   API stability announcements do not apply to it? Though, I would really like
> to
>   even get the dummy API stable by 1.0, so maybe it's OK as long as we've got
>   separate soname numbering for it until 1.0 (at which point I think it'd be
>   obnoxious /not/ to have them all unified, as that would lead to really ugly
>   package numbering for at least Debian and derivatives)?

That’s bug #711544. I agree it’s definitely necessary to have different soname versioning, but I think the release versioning should  be kept uniform across the backends, or chaos will ensue. They’re all tied together by being the same tarball anyway.

I’ve updated that bug and made it a prerequisite of this one. The dummy backend branch is now rebased on it.

> * update the NEWS changes to be in the 0.9.7 section

Doesn’t seem to be necessary? May have fixed itself in the rebase.

> * as far as the broken persona-store test, you can merge this code then fix
>   that. It must be fixed before the next release though. And we should do that
>   relatively soon after since this is a big change worth getting out.

Agreed.

> * in general, we don't have a common convention on the new symbols to say
> "these
>   are special for the Dummy backend", *snip*

Actually, we do: ‘update’, ‘[un]register’ or ‘mock’ are used in almost all the backing store API. I’ve tweaked the documentation to better state this.

> * please file a bug for "rebase tests/folks/* on dummy backend to remove EDS
> and
>   key-file dependencies" *snip*

Bug #719502 and bug #719503.

> * longer-term, we might want to remove the key-file backend before 1.0 since
>   it's not terribly useful at this point

It’s useful as a fallback primary store if the user doesn’t have EDS. I would be opposed to removing it.

> dummy/lib/dummy-backend.vala
> ----------------------------
> set_persona_stores(), prepare(), unprepare():
> * you use {freeze, thaw}_notify() to avoid extra notifications here. Is there
>   any reason we shouldn't do this in all the other backends? If not, please
> open
>   a bug for that

Done as a new patch on this bug.

> register_persona_stores():
> * the docs claim that all stores must be a FolksDummy.PersonaStore (or
> subclass)
>   but we don't actually enforce it. At the least, we should warn if this isn't
>   met, maybe throw an Error

Added an assertion.

> dummy/lib/dummy-fat-persona.vala
> --------------------------------
> * I think it would make more sense to call this something like SuperPersona,
>   MaxPersona, or FullPersona to suggest it implements all interfaces. "Fat"
>   makes me think it just implements "many" or always has a lot of preloaded
>   attributes or something.

OK. Changed to FullPersona.

> * would it make sense to use a different verb for the update_*() functions? It
>   wasn't immediately obvious based on name that they were intended to make
>   changes as if they happened in the backing store. What if we prepend
>   "backend_" or "store_" to these functions to make it clear that they're
>   simulating changes which would normally happen outside of our control? I
>   wonder if it'd be cleaner to make them functions on the parent PersonaStore,
>   but I don't have a strong opinion either way

The method names are already pretty long and I don’t want to make them longer. I can’t think of anything better than ‘update’, so I’ll leave them as-is.

> dummy/lib/dummy-persona-store.vala
> ----------------------------------
> * all the MaybeBool properties seem to default to FALSE. Would it make sense
> for
>   them to default to UNSET, leaving it up to the caller to set up the details
> as
>   it cares about them? I guess FALSE is a reasonable default for most cases
>   though.

I think FALSE is a reasonable default and will leave them as-is.

> * It would be a little nice to wrap operations on _personas_changed_frozen in
>   functions, so it can assert if we ever try to decrease it while it's <= 0
>   (particularly since we mostly check whether it exactly equals 0)

The only way _personas_changed_frozen can be decremented is in thaw_personas_changed(), and that asserts it’s > 0 beforehand.

> +              else if (k == Folks.PersonaStore.detail_key (
> ... // repeat 30 times or more
> There's so much repetition in this block. It's probably not worth changing but
> I
> guess there isn't a great way to factor that pattern.

Not worth changing.

> * as far as I can tell, we don't actually call the *_mock() functions from any
>   tests. The code all looks right but I'd be a lot more confident if we
>   exercised that code (at least one of each ranges of timeouts and with/without
>   throwing an Error for a mock function)

I was planning on doing that as part of bug #719502.

> add_persona_from_details_mock()
> * the docs say that a negative return value in the delegate will have it return
>   "synchronously". Is that actually true? It seems to just return immediately,
>   and it's from an async function. I guess that's technically true but it makes
>   it sound like it's somehow blocking for some amount of time.

Whoops, looks like Vala does generate code which completes in an idle callback, rather than completely synchronously. Updated the documentation.

> * for all the other *_mock() functions, just have their docs refer to the first
>   one's because otherwise we've got a lot of aliased text. Same for their
>   delegates.

Fixed.

> set_capabilities():
> * I think we should probably have this accept a Map of well-known keys and
> their
>   new values. Otherwise we could paint ourselves into a corner.

I disagree. Those three capabilities have been fairly stable and sufficient for quite a while; I don’t foresee new ones being added. I dislike maps of well-known keys because they’re a faff to construct and call with, and not at all type-safe.

> freeze_personas_changed():
> * this functionality seems fairly useful for testing purposes. Is there any
>   reason we don't have an equivalent for Persona details themselves (other than
>   it being a lot hairier to implement)?

An equivalent for what? In the core libfolks or in the dummy backend? Sounds like bug #652659.

> reach_quiescence():
> * the docs should emphasize that it can only achieve quiescence after this
>   function has been called, since that's normally something that clients can
>   just expect to happen

Done.

> update_is_user_set_default()
> * Jonny would have a field day with this name but I think it's fine (other than
>   the note above about update_*() functions)

Hehe. Agreed.

> dummy/lib/folks-dummy-uninstalled.pc.in
> ---------------------------------------
> * this depends upon libebook and libedataserver — should it?

Good catch. Fixed.

Review of attachment 263048 [details] [review]:

This mostly looks fine - but I'm now hitting a crasher with the dummy/add-persona test. And it's somehow missing debug symbols (and I can't tell why with just some quick examination):

Reading symbols from /home/treitter/collabora/folks/tests/dummy/.libs/lt-add-persona...(no debugging symbols found)...done.
(gdb) r
Starting program: /home/treitter/checkout/gnome/folks/tests/dummy/.libs/lt-add-persona 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
Detaching after fork from child process 25240.
Detaching after fork from child process 25242.
/AddPersonaTests/adding a persona: [New Thread 0x7ffff0179700 (LWP 25253)]

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7b2eac2 in folks_dummy_persona_store_real_add_persona_from_details_co ()
   from /home/treitter/checkout/gnome/folks/backends/dummy/lib/.libs/libfolks-dummy.so.25
Missing separate debuginfos, use: debuginfo-install gmp-5.1.1-2.fc19.x86_64 libmodman-2.0.1-6.fc19.x86_64 nettle-2.6-2.fc19.x86_64 systemd-libs-204-17.fc19.x86_64
(gdb) bt

+ Trace 232891

So, please see if you can reproduce or figure out what might be going wrong. The implementation of the notes functionality doesn't seem obviously wrong, but I haven't tried debugging in detail.

Other than that, I think this patch is ready.

Review of attachment 263050 [details] [review]:

Looks good. Merge after deps.

Review of attachment 263051 [details] [review]:

Looks good.

Fixed the crasher in comment #46 (was a missing NULL check). Everything pushed to master! Yay!

Attachment 263048 [details] pushed as 3a63de9 - dummy: Add a dummy backend
Attachment 263050 [details] pushed as e0e7675 - tests: Add a standalone-individuals test program
Attachment 263051 [details] pushed as 3eb474f - backends: Add [freeze|thaw]_notify() calls to [un]prepare() in backends