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

Review URL: https://codereview.chromium.org/234113002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@267422 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 146 insertions, 62 deletions

diff --git a/sync/test/fake_server/fake_server.cc b/sync/test/fake_server/fake_server.cc
index d758e5d..798797e 100644
--- a/sync/test/fake_server/fake_server.cc
+++ b/sync/test/fake_server/fake_server.cc
@@ -4,6 +4,7 @@
 
 #include "sync/test/fake_server/fake_server.h"
 
+#include <algorithm>
 #include <limits>
 #include <string>
 #include <vector>
@@ -29,7 +30,6 @@
 using std::string;
 using std::vector;
 
-using base::AutoLock;
 using syncer::GetModelType;
 using syncer::ModelType;
 using syncer::ModelTypeSet;
@@ -238,11 +238,8 @@ void FakeServer::SaveEntity(FakeServerEntity* entity) {
   entities_[entity->GetId()] = entity;
 }
 
-int FakeServer::HandleCommand(const string& request,
-                              int* response_code,
-                              string* response) {
-  AutoLock lock(lock_);
-
+void FakeServer::HandleCommand(const string& request,
+                               const HandleCommandCallback& callback) {
   sync_pb::ClientToServerMessage message;
   bool parsed = message.ParseFromString(request);
   DCHECK(parsed);
@@ -259,20 +256,20 @@ int FakeServer::HandleCommand(const string& request,
                                     response_proto.mutable_commit());
       break;
     default:
-      return net::ERR_NOT_IMPLEMENTED;
+      callback.Run(net::ERR_NOT_IMPLEMENTED, 0, string());;
+      return;
   }
 
   if (!success) {
     // TODO(pvalenzuela): Add logging here so that tests have more info about
     // the failure.
-    return net::HTTP_BAD_REQUEST;
+    callback.Run(net::ERR_FAILED, 0, string());
+    return;
   }
 
   response_proto.set_error_code(sync_pb::SyncEnums::SUCCESS);
   response_proto.set_store_birthday(birthday_);
-  *response_code = net::HTTP_OK;
-  *response = response_proto.SerializeAsString();
-  return 0;
+  callback.Run(0, net::HTTP_OK, response_proto.SerializeAsString());
 }
 
 bool FakeServer::HandleGetUpdatesRequest(
@@ -324,13 +321,13 @@ bool FakeServer::HandleGetUpdatesRequest(
   return true;
 }
 
-bool FakeServer::CommitEntity(
+string FakeServer::CommitEntity(
     const sync_pb::SyncEntity& client_entity,
     sync_pb::CommitResponse_EntryResponse* entry_response,
     string client_guid,
-    std::map<string, string>* client_to_server_ids) {
+    string parent_id) {
   if (client_entity.version() == 0 && client_entity.deleted()) {
-    return false;
+    return string();
   }
 
   FakeServerEntity* entity;
@@ -339,7 +336,7 @@ bool FakeServer::CommitEntity(
     // TODO(pvalenzuela): Change the behavior of DeleteChilden so that it does
     // not modify server data if it fails.
     if (!DeleteChildren(client_entity.id_string())) {
-      return false;
+      return string();
     }
   } else if (GetModelType(client_entity) == syncer::NIGORI) {
     // NIGORI is the only permanent item type that should be updated by the
@@ -356,11 +353,6 @@ bool FakeServer::CommitEntity(
       entity = UniqueClientEntity::CreateNew(client_entity);
     }
   } else {
-    string parent_id = client_entity.parent_id_string();
-    if (client_to_server_ids->find(parent_id) !=
-        client_to_server_ids->end()) {
-      parent_id = (*client_to_server_ids)[parent_id];
-    }
     // TODO(pvalenzuela): Validate entity's parent ID.
     if (entities_.find(client_entity.id_string()) != entities_.end()) {
       entity = BookmarkEntity::CreateUpdatedVersion(
@@ -375,17 +367,12 @@ bool FakeServer::CommitEntity(
   if (entity == NULL) {
     // TODO(pvalenzuela): Add logging so that it is easier to determine why
     // creation failed.
-    return false;
-  }
-
-  // Record the ID if it was renamed.
-  if (client_entity.id_string() != entity->GetId()) {
-    (*client_to_server_ids)[client_entity.id_string()] = entity->GetId();
+    return string();
   }
 
   SaveEntity(entity);
   BuildEntryResponseForSuccessfulCommit(entry_response, entity);
-  return true;
+  return entity->GetId();
 }
 
 void FakeServer::BuildEntryResponseForSuccessfulCommit(
@@ -442,6 +429,7 @@ bool FakeServer::HandleCommitRequest(
     sync_pb::CommitResponse* response) {
   std::map<string, string> client_to_server_ids;
   string guid = commit.cache_guid();
+  ModelTypeSet committed_model_types;
 
   // TODO(pvalenzuela): Add validation of CommitMessage.entries.
   ::google::protobuf::RepeatedPtrField<sync_pb::SyncEntity>::const_iterator it;
@@ -449,11 +437,30 @@ bool FakeServer::HandleCommitRequest(
     sync_pb::CommitResponse_EntryResponse* entry_response =
         response->add_entryresponse();
 
-    if (!CommitEntity(*it, entry_response, guid, &client_to_server_ids)) {
+    sync_pb::SyncEntity client_entity = *it;
+    string parent_id = client_entity.parent_id_string();
+    if (client_to_server_ids.find(parent_id) !=
+        client_to_server_ids.end()) {
+      parent_id = client_to_server_ids[parent_id];
+    }
+
+    string entity_id = CommitEntity(client_entity,
+                                    entry_response,
+                                    guid,
+                                    parent_id);
+    if (entity_id.empty()) {
       return false;
     }
+
+    // Record the ID if it was renamed.
+    if (entity_id != client_entity.id_string()) {
+      client_to_server_ids[client_entity.id_string()] = entity_id;
+    }
+    FakeServerEntity* entity = entities_[entity_id];
+    committed_model_types.Put(entity->GetModelType());
   }
 
+  FOR_EACH_OBSERVER(Observer, observers_, OnCommit(committed_model_types));
   return true;
 }
 
@@ -489,4 +496,12 @@ scoped_ptr<base::DictionaryValue> FakeServer::GetEntitiesAsDictionaryValue() {
   return dictionary.Pass();
 }
 
+void FakeServer::AddObserver(Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void FakeServer::RemoveObserver(Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
 }  // namespace fake_server
diff --git a/sync/test/fake_server/fake_server.h b/sync/test/fake_server/fake_server.h
index 0621bbc..6af77ab 100644
--- a/sync/test/fake_server/fake_server.h
+++ b/sync/test/fake_server/fake_server.h
@@ -7,10 +7,12 @@
 
 #include <map>
 #include <string>
+#include <vector>
 
 #include "base/basictypes.h"
+#include "base/callback.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/synchronization/lock.h"
+#include "base/observer_list.h"
 #include "base/values.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/protocol/sync.pb.h"
@@ -21,15 +23,26 @@ namespace fake_server {
 // A fake version of the Sync server used for testing.
 class FakeServer {
  public:
+  typedef base::Callback<void(int, int, const std::string&)>
+      HandleCommandCallback;
+
+  class Observer {
+   public:
+    virtual ~Observer() {}
+
+    // Called after FakeServer has processed a successful commit. The types
+    // updated as part of the commit are passed in |committed_model_types|.
+    virtual void OnCommit(syncer::ModelTypeSet committed_model_types) = 0;
+  };
+
   FakeServer();
   virtual ~FakeServer();
 
-  // Handles a /command POST to the server. If the return value is 0 (success),
-  // |response_code| and |response| will be set. Otherwise, the return value
-  // will be a network error code.
-  int HandleCommand(const std::string& request,
-                    int* response_code,
-                    std::string* response);
+  // Asynchronously handles a /command POST to the server. If the error_code is
+  // passed to |callback| is 0 (success), the POST's response code and content
+  // will also be passed.
+  void HandleCommand(const std::string& request,
+                     const HandleCommandCallback& callback);
 
   // Creates a DicionaryValue representation of all entities present in the
   // server. The dictionary keys are the strings generated by ModelTypeToString
@@ -37,6 +50,14 @@ class FakeServer {
   // names.
   scoped_ptr<base::DictionaryValue> GetEntitiesAsDictionaryValue();
 
+  // Adds |observer| to FakeServer's observer list. This should be called
+  // before the Profile associated with |observer| is connected to the server.
+  void AddObserver(Observer* observer);
+
+  // Removes |observer| from the FakeServer's observer list. This method
+  // must be called if AddObserver was ever called with |observer|.
+  void RemoveObserver(Observer* observer);
+
  private:
   typedef std::map<std::string, FakeServerEntity*> EntityMap;
 
@@ -59,14 +80,15 @@ class FakeServer {
   void SaveEntity(FakeServerEntity* entity);
 
   // Commits a client-side SyncEntity to the server as a FakeServerEntity.
-  // The client that sent the commit is identified via |client_guid| and all
-  // entity ID renaming is tracked with |client_to_server_ids|. If the commit
-  // is successful, true is returned and the server's version of the SyncEntity
-  // is stored in |server_entity|.
-  bool CommitEntity(const sync_pb::SyncEntity& client_entity,
-                    sync_pb::CommitResponse_EntryResponse* entry_response,
-                    std::string client_guid,
-                    std::map<std::string, std::string>* client_to_server_ids);
+  // The client that sent the commit is identified via |client_guid|. The
+  // parent ID string present in |client_entity| should be ignored in favor
+  // of |parent_id|. If the commit is successful, the entity's server ID string
+  // is returned and a new FakeServerEntity is saved in |entities_|.
+  std::string CommitEntity(
+      const sync_pb::SyncEntity& client_entity,
+      sync_pb::CommitResponse_EntryResponse* entry_response,
+      std::string client_guid,
+      std::string parent_id);
 
   // Populates |entry_response| based on |entity|. It is assumed that
   // SaveEntity has already been called on |entity|.
@@ -82,14 +104,6 @@ class FakeServer {
   // |id|. A tombstone is not created for the entity itself.
   bool DeleteChildren(const std::string& id);
 
-  // The lock used to ensure that only one client is communicating with server
-  // at any given time.
-  //
-  // It is probably preferable to have FakeServer operate on its own thread and
-  // communicate with it via PostTask, but clients would still need to wait for
-  // requests to finish before proceeding.
-  base::Lock lock_;
-
   // This is the last version number assigned to an entity. The next entity will
   // have a version number of version_ + 1.
   int64 version_;
@@ -108,6 +122,9 @@ class FakeServer {
   // are kept track of so that permanent entities are not recreated for new
   // clients.
   syncer::ModelTypeSet created_permanent_entity_types_;
+
+  // FakeServer's observers.
+  ObserverList<Observer, true> observers_;
 };
 
 }  // namespace fake_server
diff --git a/sync/test/fake_server/fake_server_http_post_provider.cc b/sync/test/fake_server/fake_server_http_post_provider.cc
index e0b3436..5dd45c2 100644
--- a/sync/test/fake_server/fake_server_http_post_provider.cc
+++ b/sync/test/fake_server/fake_server_http_post_provider.cc
@@ -6,6 +6,11 @@
 
 #include <string>
 
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/memory/ref_counted.h"
+#include "base/sequenced_task_runner.h"
+#include "base/synchronization/waitable_event.h"
 #include "sync/test/fake_server/fake_server.h"
 
 using syncer::HttpPostProviderInterface;
@@ -13,7 +18,9 @@ using syncer::HttpPostProviderInterface;
 namespace fake_server {
 
 FakeServerHttpPostProviderFactory::FakeServerHttpPostProviderFactory(
-    FakeServer* fake_server) : fake_server_(fake_server) { }
+    FakeServer* fake_server,
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
+        : fake_server_(fake_server), task_runner_(task_runner) { }
 
 FakeServerHttpPostProviderFactory::~FakeServerHttpPostProviderFactory() { }
 
@@ -21,7 +28,7 @@ void FakeServerHttpPostProviderFactory::Init(const std::string& user_agent) { }
 
 HttpPostProviderInterface* FakeServerHttpPostProviderFactory::Create() {
   FakeServerHttpPostProvider* http =
-      new FakeServerHttpPostProvider(fake_server_);
+      new FakeServerHttpPostProvider(fake_server_, task_runner_);
   http->AddRef();
   return http;
 }
@@ -32,7 +39,11 @@ void FakeServerHttpPostProviderFactory::Destroy(
 }
 
 FakeServerHttpPostProvider::FakeServerHttpPostProvider(
-    FakeServer* fake_server) : fake_server_(fake_server) { }
+    FakeServer* fake_server,
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
+        : fake_server_(fake_server),
+          task_runner_(task_runner),
+          post_complete_(false, false) { }
 
 FakeServerHttpPostProvider::~FakeServerHttpPostProvider() { }
 
@@ -54,13 +65,35 @@ void FakeServerHttpPostProvider::SetPostPayload(const char* content_type,
   request_content_.assign(content, content_length);
 }
 
+void FakeServerHttpPostProvider::OnPostComplete(int error_code,
+                                                int response_code,
+                                                const std::string& response) {
+  post_error_code_ = error_code;
+  post_response_code_ = response_code;
+  response_ = response;
+  post_complete_.Signal();
+}
+
 bool FakeServerHttpPostProvider::MakeSynchronousPost(int* error_code,
                                                      int* response_code) {
-  // This assumes that a POST is being made to /command.
-  *error_code = fake_server_->HandleCommand(request_content_,
-                                            response_code,
-                                            &response_);
-  return (*error_code == 0);
+  // It is assumed that a POST is being made to /command.
+  FakeServer::HandleCommandCallback callback = base::Bind(
+      &FakeServerHttpPostProvider::OnPostComplete, base::Unretained(this));
+  task_runner_->PostNonNestableTask(FROM_HERE,
+                                    base::Bind(&FakeServer::HandleCommand,
+                                               base::Unretained(fake_server_),
+                                               base::ConstRef(request_content_),
+                                               base::ConstRef(callback)));
+  const int kTimeoutSecs = 5;
+  bool signaled = post_complete_.TimedWait(
+      base::TimeDelta::FromSeconds(kTimeoutSecs));
+  if (!signaled || *error_code != 0) {
+    return false;
+  }
+
+  *error_code = post_error_code_;
+  *response_code = post_response_code_;
+  return true;
 }
 
 int FakeServerHttpPostProvider::GetResponseContentLength() const {
diff --git a/sync/test/fake_server/fake_server_http_post_provider.h b/sync/test/fake_server/fake_server_http_post_provider.h
index 63f1eda..9e2aa0e 100644
--- a/sync/test/fake_server/fake_server_http_post_provider.h
+++ b/sync/test/fake_server/fake_server_http_post_provider.h
@@ -8,6 +8,8 @@
 #include <string>
 
 #include "base/memory/ref_counted.h"
+#include "base/sequenced_task_runner.h"
+#include "base/synchronization/waitable_event.h"
 #include "sync/internal_api/public/http_post_provider_factory.h"
 #include "sync/internal_api/public/http_post_provider_interface.h"
 
@@ -19,7 +21,9 @@ class FakeServerHttpPostProvider
     : public syncer::HttpPostProviderInterface,
       public base::RefCountedThreadSafe<FakeServerHttpPostProvider> {
  public:
-  explicit FakeServerHttpPostProvider(FakeServer* fake_server);
+  explicit FakeServerHttpPostProvider(
+      FakeServer* fake_server,
+      scoped_refptr<base::SequencedTaskRunner> task_runner);
 
   // HttpPostProviderInterface implementation.
   virtual void SetExtraRequestHeaders(const char* headers) OVERRIDE;
@@ -39,13 +43,22 @@ class FakeServerHttpPostProvider
   virtual ~FakeServerHttpPostProvider();
 
  private:
+  void OnPostComplete(int error_code,
+                      int response_code,
+                      const std::string& response);
+
   FakeServer* const fake_server_;
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+
   std::string response_;
   std::string request_url_;
   int request_port_;
   std::string request_content_;
   std::string request_content_type_;
   std::string extra_request_headers_;
+  int post_error_code_;
+  int post_response_code_;
+  base::WaitableEvent post_complete_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeServerHttpPostProvider);
 };
@@ -53,7 +66,9 @@ class FakeServerHttpPostProvider
 class FakeServerHttpPostProviderFactory
     : public syncer::HttpPostProviderFactory {
  public:
-  explicit FakeServerHttpPostProviderFactory(FakeServer* fake_server);
+  explicit FakeServerHttpPostProviderFactory(
+      FakeServer* fake_server,
+      scoped_refptr<base::SequencedTaskRunner> task_runner);
   virtual ~FakeServerHttpPostProviderFactory();
 
   // HttpPostProviderFactory:
@@ -63,6 +78,7 @@ class FakeServerHttpPostProviderFactory
 
  private:
   FakeServer* const fake_server_;
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeServerHttpPostProviderFactory);
 };
diff --git a/sync/test/fake_server/fake_server_network_resources.cc b/sync/test/fake_server/fake_server_network_resources.cc
index 56d5ea2..c3bedb0 100644
--- a/sync/test/fake_server/fake_server_network_resources.cc
+++ b/sync/test/fake_server/fake_server_network_resources.cc
@@ -5,6 +5,7 @@
 #include "sync/test/fake_server/fake_server_network_resources.h"
 
 #include "base/memory/scoped_ptr.h"
+#include "base/message_loop/message_loop.h"
 #include "sync/internal_api/public/base/cancelation_signal.h"
 #include "sync/internal_api/public/http_post_provider_factory.h"
 #include "sync/internal_api/public/network_time_update_callback.h"
@@ -28,7 +29,9 @@ scoped_ptr<syncer::HttpPostProviderFactory>
         const NetworkTimeUpdateCallback& network_time_update_callback,
         CancelationSignal* cancelation_signal) {
   return make_scoped_ptr<syncer::HttpPostProviderFactory>(
-      new FakeServerHttpPostProviderFactory(fake_server_));
+      new FakeServerHttpPostProviderFactory(
+          fake_server_,
+          base::MessageLoop::current()->message_loop_proxy()));
 }
 
 }  // namespace fake_server