Use FakeServer-based invalidations for Sync tests

Use FakeServer-based invalidations for Sync tests

This CL creates a new InvalidationService implementation,
FakeServerInvalidationService, to remove FakeServer-based tests'
dependency on the Python XMPP server.

Another major change is that FakeServer's HandleCommand is now executed
on the UI thread and locking has been removed from FakeServer itself.

BUG=323265
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267422

[pval...(no longer on Chromium), 2014-04-22 17:26:08]

@rlarocque no rush on this CL.

A couple points I'd especially like your feedback on:
1) Thoughts on delegating to FakeInvalidationService from
FakeServerInvalidationService?
2) What do you think about using unknown version invalidations in
FakeServerInvalidationService? Is this ok for a first version?

[rlarocque, 2014-04-22 18:22:11]

On 2014/04/22 17:26:08, pvalenzuela wrote:
> @rlarocque no rush on this CL.
> 
> A couple points I'd especially like your feedback on:
> 1) Thoughts on delegating to FakeInvalidationService from
> FakeServerInvalidationService?

It's a little strange, but I can see how it makes sense.

Personally, I'd prefer to re-implement each method in the
FakeServerInvalidationService without leveraging the FakeInvalidationService.  I
think it's likely those classes will head in different directions in the future,
so it would be best if there are no dependencies between them.

See also the P2PInvalidationService and P2PInvalidationForwarder.  You might
find it easier to structure this code if you split up the observing and
InvalidationService logic.

Come to think of it, maybe it would make more sense to base your invalidation
service on the P2PInvalidationService.  Being able to set notification targets
could be a useful feature, and the P2PInvalidationService is already used in
integration tests.  We might have to do something about the name and namespace,
though.

> 2) What do you think about using unknown version invalidations in
> FakeServerInvalidationService? Is this ok for a first version?

Yep, that's fine.  The only difference client-side is that these unnkown version
invalidations will be collapsed into each other.  The invalidations with hints
are tracked a bit more precisely.

[rlarocque, 2014-04-22 18:22:22]

https://codereview.chromium.org/234113002/diff/220001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc
(right):

https://codereview.chromium.org/234113002/diff/220001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc:1619: //
This test fails when run with FakeServer and FakeServerInvalidationService.
That's odd.  Do you know why it fails?

https://codereview.chromium.org/234113002/diff/220001/sync/sync_tests.gypi
File sync/sync_tests.gypi (right):

https://codereview.chromium.org/234113002/diff/220001/sync/sync_tests.gypi#ne...
sync/sync_tests.gypi:106: '../third_party/protobuf/protobuf.gyp:protobuf_lite',
Is that a tab character?

https://codereview.chromium.org/234113002/diff/220001/sync/test/DEPS
File sync/test/DEPS (right):

https://codereview.chromium.org/234113002/diff/220001/sync/test/DEPS#newcode2
sync/test/DEPS:2: "+content/public/browser/browser_thread.h",
I don't think you can do this.

https://codereview.chromium.org/234113002/diff/220001/sync/test/fake_server/f...
File sync/test/fake_server/fake_server.cc (right):

https://codereview.chromium.org/234113002/diff/220001/sync/test/fake_server/f...
sync/test/fake_server/fake_server.cc:336: ModelType* model_type,
This is starting to look a little awkward.  Could you just return the server ID
of the saved entity instead?  That way you could populate the
client_to_server_ids map and fetch the ModelType from the caller.

That may or may not be cleaner.  Let me know what you think.

https://codereview.chromium.org/234113002/diff/220001/sync/test/fake_server/f...
File sync/test/fake_server/fake_server.h (right):

https://codereview.chromium.org/234113002/diff/220001/sync/test/fake_server/f...
sync/test/fake_server/fake_server.h:45: const base::Closure& callback,
Rather than asking the caller to provide space for the returned values, I think
it would be more idiomatic to have the callback take arguments.  Could you make
it a base::Callback<void(int, int, const std::string&)>?

https://codereview.chromium.org/234113002/diff/220001/sync/test/fake_server/f...
sync/test/fake_server/fake_server.h:53: void AddObserver(Observer* observer);
This is a bit confusing.  Normally the observed object outlives the observer,
and has a RemoveObserver() method that must be called before its observers are
deleted.  

As it is, I think this will only work if the FakeServer is destroyed before its
observers.

I'd prefer the explicit unregister pattern.  It's a bit easier to DCHECK() when
it's used improperly.  See also the ScopedObserver class if you'd like some help
making sure the unregister method is called.

https://codereview.chromium.org/234113002/diff/220001/sync/test/fake_server/f...
File sync/test/fake_server/fake_server_http_post_provider.cc (right):

https://codereview.chromium.org/234113002/diff/220001/sync/test/fake_server/f...
sync/test/fake_server/fake_server_http_post_provider.cc:70:
BrowserThread::PostTask(BrowserThread::UI,
If this code is to live in sync/test, then I'm not sure if it's allowed to know
about the UI thread.  It should be OK to pass in a thread or
scoped_refptr<SequencedTaskRunner> that it could use, though.

[pval...(no longer on Chromium), 2014-04-24 01:08:53]

(Sorry about syncing/rebasing before uploading the new patchset)

Let's talk about a couple things tomorrow:
1) the MC_DeleteBookmark failing test, and
2) FakeServerInvalidationService's dependency on FakeInvalidationService

https://codereview.chromium.org/234113002/diff/220001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc
(right):

https://codereview.chromium.org/234113002/diff/220001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc:1619: //
This test fails when run with FakeServer and FakeServerInvalidationService.
On 2014/04/22 18:22:22, rlarocque wrote:
> That's odd.  Do you know why it fails?

IIRC, the progress markers don't match so AwaitQuiescence fails The interesting
part is that the test passes fine w/o the AwaitQuiescence check.

Let's debug this a little bit tomorrow before proceeding.

https://codereview.chromium.org/234113002/diff/220001/sync/sync_tests.gypi
File sync/sync_tests.gypi (right):

https://codereview.chromium.org/234113002/diff/220001/sync/sync_tests.gypi#ne...
sync/sync_tests.gypi:106: '../third_party/protobuf/protobuf.gyp:protobuf_lite',
On 2014/04/22 18:22:22, rlarocque wrote:
> Is that a tab character?

this was resolved in another CL that I committed

https://codereview.chromium.org/234113002/diff/220001/sync/test/DEPS
File sync/test/DEPS (right):

https://codereview.chromium.org/234113002/diff/220001/sync/test/DEPS#newcode2
sync/test/DEPS:2: "+content/public/browser/browser_thread.h",
On 2014/04/22 18:22:22, rlarocque wrote:
> I don't think you can do this.

removed

https://codereview.chromium.org/234113002/diff/220001/sync/test/fake_server/f...
File sync/test/fake_server/fake_server.cc (left):

https://codereview.chromium.org/234113002/diff/220001/sync/test/fake_server/f...
sync/test/fake_server/fake_server.cc:267: return net::HTTP_BAD_REQUEST;
this value was incorrect (it's the error code, not an HTTP response code)

https://codereview.chromium.org/234113002/diff/220001/sync/test/fake_server/f...
File sync/test/fake_server/fake_server.cc (right):

https://codereview.chromium.org/234113002/diff/220001/sync/test/fake_server/f...
sync/test/fake_server/fake_server.cc:336: ModelType* model_type,
On 2014/04/22 18:22:22, rlarocque wrote:
> This is starting to look a little awkward.  Could you just return the server
ID
> of the saved entity instead?  That way you could populate the
> client_to_server_ids map and fetch the ModelType from the caller.
> 
> That may or may not be cleaner.  Let me know what you think.

done / looks good to me

https://codereview.chromium.org/234113002/diff/220001/sync/test/fake_server/f...
File sync/test/fake_server/fake_server.h (right):

https://codereview.chromium.org/234113002/diff/220001/sync/test/fake_server/f...
sync/test/fake_server/fake_server.h:45: const base::Closure& callback,
On 2014/04/22 18:22:22, rlarocque wrote:
> Rather than asking the caller to provide space for the returned values, I
think
> it would be more idiomatic to have the callback take arguments.  Could you
make
> it a base::Callback<void(int, int, const std::string&)>?

Done.

https://codereview.chromium.org/234113002/diff/220001/sync/test/fake_server/f...
sync/test/fake_server/fake_server.h:53: void AddObserver(Observer* observer);
On 2014/04/22 18:22:22, rlarocque wrote:
> This is a bit confusing.  Normally the observed object outlives the observer,
> and has a RemoveObserver() method that must be called before its observers are
> deleted.  
> 
> As it is, I think this will only work if the FakeServer is destroyed before
its
> observers.
> 
> I'd prefer the explicit unregister pattern.  It's a bit easier to DCHECK()
when
> it's used improperly.  See also the ScopedObserver class if you'd like some
help
> making sure the unregister method is called.

I've added a RemoveAllObservers method to FakeServer and modified
FakeServer.observers_'s definition to check that it's empty on destruction.

Do you think this is sufficient? There's no point right now where we would
remove observers separately so I think one method makes sense.

https://codereview.chromium.org/234113002/diff/220001/sync/test/fake_server/f...
File sync/test/fake_server/fake_server_http_post_provider.cc (right):

https://codereview.chromium.org/234113002/diff/220001/sync/test/fake_server/f...
sync/test/fake_server/fake_server_http_post_provider.cc:70:
BrowserThread::PostTask(BrowserThread::UI,
On 2014/04/22 18:22:22, rlarocque wrote:
> If this code is to live in sync/test, then I'm not sure if it's allowed to
know
> about the UI thread.  It should be OK to pass in a thread or
> scoped_refptr<SequencedTaskRunner> that it could use, though.

Done.

[rlarocque, 2014-04-25 17:41:46]

https://codereview.chromium.org/234113002/diff/220001/sync/test/fake_server/f...
File sync/test/fake_server/fake_server.h (right):

https://codereview.chromium.org/234113002/diff/220001/sync/test/fake_server/f...
sync/test/fake_server/fake_server.h:53: void AddObserver(Observer* observer);
On 2014/04/24 01:08:53, pvalenzuela wrote:
> On 2014/04/22 18:22:22, rlarocque wrote:
> > This is a bit confusing.  Normally the observed object outlives the
observer,
> > and has a RemoveObserver() method that must be called before its observers
are
> > deleted.  
> > 
> > As it is, I think this will only work if the FakeServer is destroyed before
> its
> > observers.
> > 
> > I'd prefer the explicit unregister pattern.  It's a bit easier to DCHECK()
> when
> > it's used improperly.  See also the ScopedObserver class if you'd like some
> help
> > making sure the unregister method is called.
> 
> I've added a RemoveAllObservers method to FakeServer and modified
> FakeServer.observers_'s definition to check that it's empty on destruction.
> 
> Do you think this is sufficient? There's no point right now where we would
> remove observers separately so I think one method makes sense.

I'd prefer to remove them each individually.  Having a "remove all" might hide
bugs, and is likely to cause problems later on.

Come to think of it, don't you need to unregister an individual observer in
those tests where we disable one client?  I know most of them just disable one
type at a time, but I thought there might be one or two that fully shut down the
sync backend.

[pval...(no longer on Chromium), 2014-04-29 00:06:07]

Ok, I think this is ready to resume the review.

I still can't figure out the failing test but I'm almost convinced that it's
behaving closer to the real world scenario. The appropriate data is syncing, but
progress markers are not matching since the re-enabled client doesn't trigger a
GetUpdates for the first client.

https://codereview.chromium.org/234113002/diff/220001/sync/test/fake_server/f...
File sync/test/fake_server/fake_server.h (right):

https://codereview.chromium.org/234113002/diff/220001/sync/test/fake_server/f...
sync/test/fake_server/fake_server.h:53: void AddObserver(Observer* observer);
On 2014/04/25 17:41:46, rlarocque wrote:
> On 2014/04/24 01:08:53, pvalenzuela wrote:
> > On 2014/04/22 18:22:22, rlarocque wrote:
> > > This is a bit confusing.  Normally the observed object outlives the
> observer,
> > > and has a RemoveObserver() method that must be called before its observers
> are
> > > deleted.  
> > > 
> > > As it is, I think this will only work if the FakeServer is destroyed
before
> > its
> > > observers.
> > > 
> > > I'd prefer the explicit unregister pattern.  It's a bit easier to DCHECK()
> > when
> > > it's used improperly.  See also the ScopedObserver class if you'd like
some
> > help
> > > making sure the unregister method is called.
> > 
> > I've added a RemoveAllObservers method to FakeServer and modified
> > FakeServer.observers_'s definition to check that it's empty on destruction.
> > 
> > Do you think this is sufficient? There's no point right now where we would
> > remove observers separately so I think one method makes sense.
> 
> I'd prefer to remove them each individually.  Having a "remove all" might hide
> bugs, and is likely to cause problems later on.
> 
> Come to think of it, don't you need to unregister an individual observer in
> those tests where we disable one client?  I know most of them just disable one
> type at a time, but I thought there might be one or two that fully shut down
the
> sync backend.

I've added TODOs in SyncTest.{Enable,Disable}Network() as we will need to adjust
the observer lifetime in that case.

I believe the full disable case is handled in SyncBackendHost, so nothing needs
to be done on my end. See
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...

Are there any other cases you think we should be concerned about?

[rlarocque, 2014-04-29 00:13:02]

https://codereview.chromium.org/234113002/diff/220001/sync/test/fake_server/f...
File sync/test/fake_server/fake_server.h (right):

https://codereview.chromium.org/234113002/diff/220001/sync/test/fake_server/f...
sync/test/fake_server/fake_server.h:53: void AddObserver(Observer* observer);
On 2014/04/29 00:06:07, pvalenzuela wrote:
> On 2014/04/25 17:41:46, rlarocque wrote:
> > On 2014/04/24 01:08:53, pvalenzuela wrote:
> > > On 2014/04/22 18:22:22, rlarocque wrote:
> > > > This is a bit confusing.  Normally the observed object outlives the
> > observer,
> > > > and has a RemoveObserver() method that must be called before its
observers
> > are
> > > > deleted.  
> > > > 
> > > > As it is, I think this will only work if the FakeServer is destroyed
> before
> > > its
> > > > observers.
> > > > 
> > > > I'd prefer the explicit unregister pattern.  It's a bit easier to
DCHECK()
> > > when
> > > > it's used improperly.  See also the ScopedObserver class if you'd like
> some
> > > help
> > > > making sure the unregister method is called.
> > > 
> > > I've added a RemoveAllObservers method to FakeServer and modified
> > > FakeServer.observers_'s definition to check that it's empty on
destruction.
> > > 
> > > Do you think this is sufficient? There's no point right now where we would
> > > remove observers separately so I think one method makes sense.
> > 
> > I'd prefer to remove them each individually.  Having a "remove all" might
hide
> > bugs, and is likely to cause problems later on.
> > 
> > Come to think of it, don't you need to unregister an individual observer in
> > those tests where we disable one client?  I know most of them just disable
one
> > type at a time, but I thought there might be one or two that fully shut down
> the
> > sync backend.
> 
> I've added TODOs in SyncTest.{Enable,Disable}Network() as we will need to
adjust
> the observer lifetime in that case.
> 
> I believe the full disable case is handled in SyncBackendHost, so nothing
needs
> to be done on my end. See
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/syn...
> 
> Are there any other cases you think we should be concerned about?

That helps with some concerns, but I still worry about hiding bugs. 
RemoveAllObservers() is too easy to abuse.

[rlarocque, 2014-04-29 00:29:20]

lgtm

https://codereview.chromium.org/234113002/diff/300001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/fake_server_invalidation_service.h
(right):

https://codereview.chromium.org/234113002/diff/300001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/fake_server_invalidation_service.h:68: 
nit: Remove blank newline.

[pval...(no longer on Chromium), 2014-04-30 17:03:58]

https://codereview.chromium.org/234113002/diff/300001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/fake_server_invalidation_service.h
(right):

https://codereview.chromium.org/234113002/diff/300001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/fake_server_invalidation_service.h:68: 
On 2014/04/29 00:29:21, rlarocque wrote:
> nit: Remove blank newline.

Done.

[pval...(no longer on Chromium), 2014-04-30 17:07:37]

The CQ bit was checked by pvalenzuela@chromium.org

[commit-bot: I haz the power, 2014-04-30 17:11:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pvalenzuela@chromium.org/234113002/320001

[commit-bot: I haz the power, 2014-04-30 18:57:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-30 18:57:56]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium

[pval...(no longer on Chromium), 2014-05-01 00:46:12]

The CQ bit was checked by pvalenzuela@chromium.org

[commit-bot: I haz the power, 2014-05-01 00:47:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pvalenzuela@chromium.org/234113002/320001

[commit-bot: I haz the power, 2014-05-01 04:21:45]

Change committed as 267422

[sadrul, 2014-05-01 04:47:20]

On 2014/05/01 04:21:45, I haz the power (commit-bot) wrote:
> Change committed as 267422

Hi. This seems to break linux-clang build with the following compile error:

FAILED: /b/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/sync/test/fake_server/run_sync_fake_server.fake_sync_server_http_handler.o.d
...  -o
obj/sync/test/fake_server/run_sync_fake_server.fake_sync_server_http_handler.o
../../sync/test/fake_server/fake_sync_server_http_handler.cc:51:61: error: too
many arguments to function call, expected 2, have 3
                                                            &response);
                                                            ^~~~~~~~~
../../sync/test/fake_server/fake_server.h:44:3: note: 'HandleCommand' declared
here
  void HandleCommand(const std::string& request,
  ^
1 error generated.
ninja: build stopped: subcommand failed.

Build link:
http://build.chromium.org/p/chromium.linux/builders/Linux%20Clang%20%28dbg%29...