Tests for the Copresence API.

components/copresence/copresence_client.cc

Index: components/copresence/copresence_client.cc
diff --git a/components/copresence/copresence_client.cc b/components/copresence/copresence_client.cc
index 60e45e15d2156368eddf0e575f1812e6339e9dc6..aee95e3149414e8ef5c770ced34c345262dace15 100644
--- a/components/copresence/copresence_client.cc
+++ b/components/copresence/copresence_client.cc
@@ -22,42 +22,27 @@ PendingRequest::~PendingRequest() {
 
 // Public methods
 
-CopresenceClient::CopresenceClient(CopresenceClientDelegate* delegate)
-    : delegate_(delegate), init_failed_(false), pending_init_operations_(0) {
-  DVLOG(3) << "Initializing client.";
-  pending_init_operations_++;
-  rpc_handler_.reset(
-      new RpcHandler(delegate,
-                     base::Bind(&CopresenceClient::InitStepComplete,
-                                AsWeakPtr(),
-                                "Copresence device registration")));
-
-  pending_init_operations_++;
-  delegate_->GetWhispernetClient()->Initialize(
-      base::Bind(&CopresenceClient::InitStepComplete,
-                 AsWeakPtr(),
-                 "Whispernet proxy initialization"));
-}
-
-CopresenceClient::~CopresenceClient() {
-}
+CopresenceClient::~CopresenceClient() {}
 
 void CopresenceClient::Shutdown() {
   DVLOG(3) << "Shutting down client.";
-  delegate_->GetWhispernetClient()->Shutdown();
-  rpc_handler_->DisconnectFromWhispernet();
+  if (delegate_ && delegate_->GetWhispernetClient())

[not at google - send to devlin, 2014/08/07 23:21:12]

when would there not be a delegate? could you pass the delegate into the
constructor?

i guess that's 2 separate questions.

[Charlie, 2014/08/07 23:52:25]

I guess we only have the use case where there's no whispernet client (in tests).
Fixed.

[rkc, 2014/08/08 00:05:35]

We should rebase this code and have it tracked of the RPC handler CL. All of
this code is already gone there.

[Charlie, 2014/08/08 18:25:50]

Done.

+    delegate_->GetWhispernetClient()->Shutdown();
+  if (rpc_handler_.get())

[not at google - send to devlin, 2014/08/07 23:21:13]

likewise.

[Charlie, 2014/08/07 23:52:25]

MockCopresenceClient won't have an rpc_handler_.

[not at google - send to devlin, 2014/08/07 23:54:24]

this inheritance structure is confusing. inheritance should be a last resort,
this really seems like a case of an Interface with 2 separate implemetations.

[rkc, 2014/08/08 00:37:00]

Deriving a Mock class from a concrete class is a very common pattern used in
Chrome (in //src: git gs "Mock.*: public.*Impl" | wc -l = 19, and those are for
clear concrete classes). It helps us test existing functionality if we need to
while mocking out code we don't want to test.

Creating an interface just to be able to be able to mock a class is definitely
overkill.

[not at google - send to devlin, 2014/08/08 13:48:35]

git gs "Mock.*: public.*" | wc -l = 1115

what does that prove?
What are you trying to test then? If you're trying to test the
CopresenceService, do that. If you're trying to test the API implementation, do
that. There is no such thing as a class that you only need to test a part of,
because a class is encapsulation of a single concept. You're changing the code
of a *production implementation* simply to support the case when the class has a
*subclass only for testing*. That's not right.

If you want to pull some part of CopresenceService out then use a Delegate, but
that's overkill. Interfaces are the bread and butter of OO.
Disagree.

[Charlie, 2014/08/08 18:25:50]

IIUC the change to the production code that we're talking about simply involves
changing a few members to protected instead of private. And altering an
implementation to make it more testable is a standard practice. So I don't
follow your reasoning.

However, as with the Create() method, it seems your instinct are right. Stack
Overflow offers some help here:

http://stackoverflow.com/questions/1595166/why-is-it-so-bad-to-mock-classes

I think the thing we were missing is that the CopresenceClient is a system
boundary, and thus defining an interface for it, however small, makes sense.
After all, it's in the copresence/public directory with another header that
already describes an interface. Another useful point is that the mock can't
accidentally run some of the production code if it inherits from an interface.
And when I went back to read the gtest docs more carefully, they also
recommended inheriting from an interface:

https://code.google.com/p/googlemock/wiki/ForDummies#A_Case_for_Mock_Turtles

Feel free to pass some of these links on the next time mocks come up in a review
;-)

[not at google - send to devlin, 2014/08/08 19:42:04]

Thanks.

I think you're right that CopresenceClient isn't actually a natural place to cut
this, because it's part of the API. and it's weird to be mocking out part of the
API to test the API :\ CopresenceService is the right place.

[Charlie, 2014/08/13 19:13:15]

Done.

+    rpc_handler_->DisconnectFromWhispernet();
 }
 
 // Returns false if any operations were malformed.
 void CopresenceClient::ExecuteReportRequest(copresence::ReportRequest request,
                                             const std::string& app_id,
                                             const StatusCallback& callback) {
-  // Don't take on any more requests, we can't execute any, init failed.
+  // Don't take on any more requests. We can't execute them since init failed.
   if (init_failed_) {
     callback.Run(FAIL);
     return;
   }
 
+  DCHECK(rpc_handler_.get());
   if (pending_init_operations_) {
     pending_requests_queue_.push_back(
         PendingRequest(request, app_id, callback));
@@ -69,12 +54,46 @@ void CopresenceClient::ExecuteReportRequest(copresence::ReportRequest request,
   }
 }
 
+// static
+scoped_ptr<CopresenceClient> CopresenceClient::Create(
+    CopresenceClientDelegate* delegate) {
+  DVLOG(3) << "Initializing client.";
+  scoped_ptr<CopresenceClient> client(new CopresenceClient);
+  DCHECK(delegate);
+  client->delegate_ = delegate;
+
+  client->pending_init_operations_++;

[not at google - send to devlin, 2014/08/07 23:21:12]

should be ++x not x++

[Charlie, 2014/08/07 23:52:25]

Either is allowed for scalar types. With the -> syntax here I think
postincrement is clearer.

https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

[rkc, 2014/08/08 00:37:00]

prefixed is still better for consistency.

+  client->rpc_handler_.reset(
+      new RpcHandler(delegate,
+                     base::Bind(&CopresenceClient::InitStepComplete,
+                                client->AsWeakPtr(),
+                                "Copresence device registration")));
+
+  client->pending_init_operations_++;
+  DCHECK(delegate->GetWhispernetClient());
+  delegate->GetWhispernetClient()->Initialize(
+      base::Bind(&CopresenceClient::InitStepComplete,
+                 client->AsWeakPtr(),
+                 "Whispernet proxy initialization"));
+
+  return client.Pass();
+}
+
+// Protected methods
+
+CopresenceClient::CopresenceClient()
+    : delegate_(NULL),
+      init_failed_(false),
+      pending_init_operations_(0) {}
+
 // Private methods
 
 void CopresenceClient::CompleteInitialization() {
   if (pending_init_operations_)
     return;
 
+  DCHECK(delegate_);
+  DCHECK(rpc_handler_.get());
   if (!init_failed_)
     rpc_handler_->ConnectToWhispernet(delegate_->GetWhispernetClient());