Tests for the Copresence API.

chrome/browser/extensions/api/copresence/copresence_apitest.cc

Index: chrome/browser/extensions/api/copresence/copresence_apitest.cc
diff --git a/chrome/browser/extensions/api/copresence/copresence_apitest.cc b/chrome/browser/extensions/api/copresence/copresence_apitest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..3f68b20a6d17e428a9e7d26b0e7313a8abf4d126
--- /dev/null
+++ b/chrome/browser/extensions/api/copresence/copresence_apitest.cc
@@ -0,0 +1,103 @@
+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license
+// that can be found in the LICENSE file.
+
+#include "chrome/browser/extensions/api/copresence/copresence_api.h"
+#include "chrome/browser/extensions/extension_apitest.h"
+#include "components/copresence/public/copresence_client.h"
+
+using copresence::CopresenceClientDelegate;
+using copresence::FAIL;
+using copresence::Message;
+using google::protobuf::RepeatedPtrField;
+
+namespace extensions {
+
+class CopresenceApiTest : public ExtensionApiTest {

[not at google - send to devlin, 2014/08/06 21:45:26]

a pattern I've seen that saves creating this vacuous class is to typedef
CopresenceApiTest to ExtensionApiTest.

[Charlie, 2014/08/07 22:24:15]

Interesting. Done.

+ public:
+  CopresenceApiTest() {}
+  virtual ~CopresenceApiTest() {}
+};
+
+class MockCopresenceClient : public copresence::CopresenceClient {
+ public:
+  explicit MockCopresenceClient(CopresenceClientDelegate* delegate)
+      : delegate_(delegate) {}
+  virtual ~MockCopresenceClient() {}
+
+  virtual void Initialize(CopresenceClientDelegate*) OVERRIDE {}
+
+  virtual void ExecuteReportRequest(
+      copresence::ReportRequest request,
+      const std::string& app_id,
+      const copresence::StatusCallback& status_callback) OVERRIDE {
+

[not at google - send to devlin, 2014/08/06 21:45:27]

if possible a nicer pattern is to have the Mock just record the operations that
were made on it, then have the test do the test assertions. this is quite hard
to read.

[rkc, 2014/08/07 08:56:57]

I am not sure if that would work with this test. It seems that this code
verifies that the operations it sees are of the type that it expects, returning
a failure to the test app otherwise. We need to know whether the operations that
the Execute call got were of the type we expected, before this call can return
an error to the API.

We could have assertions in the main test body, but then it would be hard to
fail individual JS tests since those tests have no way of knowing whether they
passed or not except via the status code we return.

I've given another suggestion below though that should help make this code more
readable.

[not at google - send to devlin, 2014/08/07 15:34:25]

could you just hold off on calling |status_callback| until you've verified the
assertions?

[Charlie, 2014/08/07 22:24:15]

Not sure how that would look. IIUC RunExtensionTest() won't return until the js
completes, which won't happen until the callbacks are called. But I did pull the
verification details out into separate functions. How's this?

+    const RepeatedPtrField<copresence::PublishedMessage>& publishes =
+        request.manage_messages_request().message_to_publish();
+    const RepeatedPtrField<std::string>& unpublishes =
+        request.manage_messages_request().id_to_unpublish();
+    const RepeatedPtrField<copresence::Subscription>& subscriptions =
+        request.manage_subscriptions_request().subscription();
+    const RepeatedPtrField<std::string>& unsubscribes =
+        request.manage_subscriptions_request().id_to_unsubscribe();
+
+    // Expecting either a publish, a subscribe, or unpublish *and* unsubscribe.
+    if (publishes.size() > 0) {
+      if (!(publishes.size() == 1 &&
+          publishes.Get(0).id() == "call" &&

[rkc, 2014/08/07 08:56:57]

Instead of checking every single operation and restricting yourself to only one
publish/subscribe operation each, you should use the id to identify the
operation and then run your verification on it based of that name. That way, you
can test multiple different types of operation structures instead of just one
per type. This also allows you to just have calls to, "VerifyCallPublish" or
"VerifyOnlyBroadcastOnlyScanStrategySubscribe", etc, making this code more
readable.

[Charlie, 2014/08/07 22:24:15]

To get the ids you still have to drill down into the sub-requests. But I like
the idea of separate VerifyX() functions.

+          publishes.Get(0).access_policy().acl().acl_type()
+              == copresence::NO_ACL_CHECK &&
+          publishes.Get(0).message().type().type() == "joke" &&
+          publishes.Get(0).message().payload() == "Knock Knock!" &&
+          publishes.Get(0).token_exchange_strategy()
+              .broadcast_scan_configuration() == copresence::BROADCAST_ONLY)) {
+        LOG(ERROR) << "Found invalid publish";
+        status_callback.Run(FAIL);
+        return;
+      }
+    } else if (subscriptions.size() > 0) {
+      if (!(subscriptions.size() == 1 &&
+          subscriptions.Get(0).id() == "response" &&
+          subscriptions.Get(0).ttl_millis() == 1000 &&
+          subscriptions.Get(0).message_type().type() == "joke" &&
+          subscriptions.Get(0).token_exchange_strategy()
+              .broadcast_scan_configuration() == copresence::SCAN_ONLY)) {
+        LOG(ERROR) << "Found invalid subscribe";
+        status_callback.Run(FAIL);
+        return;
+      } else {
+        // For subscriptions, deliver messages.
+        Message returned_message;
+        returned_message.mutable_type()->set_type("joke");
+        returned_message.set_payload("Who's there?");
+        delegate_->HandleMessages(
+            app_id, "response", std::vector<Message>(2, returned_message));
+
+      }
+    } else {
+      if (!(unpublishes.size() == 1 && unsubscribes.size() == 1 &&
+          unpublishes.Get(0) == "call" && unsubscribes.Get(0) == "response")) {
+        LOG(ERROR) << "Found invalid unpublish/unsubscribe";
+        status_callback.Run(FAIL);
+        return;
+      }
+    }
+
+    status_callback.Run(copresence::SUCCESS);
+  }
+
+ private:
+  CopresenceClientDelegate* delegate_;
+};
+
+IN_PROC_BROWSER_TEST_F(CopresenceApiTest, Basic) {
+  CopresenceService* service =
+      CopresenceService::GetFactoryInstance()->Get(browser()->profile());
+  scoped_ptr<copresence::CopresenceClient> mockClient(

[not at google - send to devlin, 2014/08/06 21:45:27]

mock_client

[Charlie, 2014/08/07 22:24:15]

Done.

+      new MockCopresenceClient(service));
+  service->set_client_for_testing(mockClient.Pass());

[rkc, 2014/08/07 08:56:57]

nit: service->set_client_for_testing(make_scoped_ptr(new
MockCopresenceClient(service)));

[Charlie, 2014/08/07 22:24:15]

It's a little wordier than that since you have to pass a
scoped_ptr<CopresenceClient> instead of scoped_ptr<MockCopresenceClient>. But I
was able to fit it by adding a "using" statement above.

+
+  ASSERT_TRUE(RunExtensionTestNoFileAccess("copresence")) << message_;
+}
+
+}  // namespace extensions