Add test for ServiceManager

mojo/shell/service_manager_unittest.cc

+#include "base/message_loop/message_loop.h"

[viettrungluu, 2014/01/03 17:44:49]

nit: Missing lame copyright boilerplate.

[DaveMoore, 2014/01/03 18:05:49]

Done.

+#include "mojo/common/bindings_support_impl.h"
+#include "mojo/public/bindings/lib/remote_ptr.h"
+#include "mojo/shell/service_manager.h"
+#include "mojom/shell.h"
+#include "mojom/test.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace mojo {
+namespace shell {
+namespace {
+
+class TestApp : public ShellClientStub {
+ public:
+  TestApp(ScopedMessagePipeHandle shell_handle)
+      : shell_(shell_handle.Pass()) {
+    shell_.SetPeer(this);
+  }
+  virtual ~TestApp() {
+  }
+  virtual void AcceptConnection(ScopedMessagePipeHandle client_handle)
+      MOJO_OVERRIDE {

[viettrungluu, 2014/01/03 17:44:49]

Typically, I think our pattern is to use the regular base macros, etc. (e.g.,
OVERRIDE, from base/compiler_specific.h) if we have base available.

[DaveMoore, 2014/01/03 18:05:49]

Done.

+    service_.reset(new TestServiceImpl(this, client_handle.Pass()));
+  }
+  std::string GetLastTestString() {
+    return service_->last_test_string_;
+  }
+
+ private:
+  class TestServiceImpl : public TestServiceStub {
+   public:
+    TestServiceImpl(TestApp* service, ScopedMessagePipeHandle client_handle)
+        : service_(service),
+          client_(client_handle.Pass()) {
+      client_.SetPeer(this);
+    }
+    virtual ~TestServiceImpl() {
+    }
+    virtual void Test(const mojo::String& test_string) MOJO_OVERRIDE {

[viettrungluu, 2014/01/03 17:44:49]

...

[DaveMoore, 2014/01/03 18:05:49]

Done.

+      last_test_string_ = test_string.To<std::string>();
+      client_->AckTest();
+    }
+    TestApp* service_;
+    RemotePtr<TestClient> client_;
+    std::string last_test_string_;
+  };
+  RemotePtr<Shell> shell_;
+  scoped_ptr<TestServiceImpl> service_;
+};
+
+class TestClientImpl : public TestClientStub {
+ public:
+  explicit TestClientImpl(ScopedMessagePipeHandle service_handle)
+      : service_(service_handle.Pass()),
+        quit_after_ack_(false) {
+    service_.SetPeer(this);
+  }
+  virtual ~TestClientImpl() {
+  }
+  virtual void AckTest() MOJO_OVERRIDE {

[viettrungluu, 2014/01/03 17:44:49]

...

[DaveMoore, 2014/01/03 18:05:49]

Done.

+    if (quit_after_ack_)
+      base::MessageLoop::current()->QuitNow();
+  }
+  void Test(std::string test_string) {
+    AllocationScope scope;
+    quit_after_ack_ = true;
+    service_->Test(mojo::String(test_string));
+  }
+  RemotePtr<TestService> service_;
+  bool quit_after_ack_;
+};
+
+class ServiceManagerTest : public testing::Test,
+                           public ServiceManager::Loader {
+ public:
+  ServiceManagerTest()
+      : test_app_(NULL),
+        test_client_(NULL),
+        service_manager_(NULL) {
+  }
+
+  virtual ~ServiceManagerTest() {
+  }
+
+  virtual void SetUp() OVERRIDE {
+    mojo::BindingsSupport::Set(&support_);
+    GURL test_url("test:testService");
+    service_manager_ = new ServiceManager();
+    service_manager_->SetLoaderForURL(this, test_url);
+    MessagePipe pipe;
+    test_client_ = new TestClientImpl(pipe.handle0.Pass());
+    service_manager_->Connect(test_url, pipe.handle1.Pass());
+  }
+
+  virtual void TearDown() OVERRIDE {
+    delete test_client_;
+    delete test_app_;
+    delete service_manager_;
+    mojo::BindingsSupport::Set(NULL);
+  }
+
+  virtual void Load(const GURL& url,
+                    ScopedMessagePipeHandle shell_handle) MOJO_OVERRIDE {

[viettrungluu, 2014/01/03 17:44:49]

...

[DaveMoore, 2014/01/03 18:05:49]

Done.

+    test_app_ = new TestApp(shell_handle.Pass());
+  }
+
+ protected:
+  common::BindingsSupportImpl support_;
+  base::MessageLoop loop_;
+  TestApp* test_app_;

[viettrungluu, 2014/01/03 17:44:49]

I think you should use scoped_ptrs here (and .reset() in TearDown()).

[DaveMoore, 2014/01/03 18:05:49]

I went back and forth on this, but didn't see any value in doing so, and this
was more direct. Why do you think the scoped_ptrs are better? Is there some
chance we could destroy the Test w/out calling TearDown()? Not clearing the
BindingsSupport is worse than leaking.

On 2014/01/03 17:44:49, viettrungluu wrote:

[viettrungluu, 2014/01/03 18:23:14]

a) It's more idiomatic (in Chromium), and makes ownership more clear.
scoped_ptrs are self-documenting; dumb pointers aren't.
b) It eliminates the risk of accidentally using the objects in question after
destruction. Or, you could reset the pointers to null after the deletes (but
then see c)).
c) It's shorter. No need to explicitly initialize to null, etc.

+  TestClientImpl* test_client_;
+  ServiceManager* service_manager_;
+  DISALLOW_COPY_AND_ASSIGN(ServiceManagerTest);
+};
+
+TEST_F(ServiceManagerTest, Basic) {
+  test_client_->Test("test");
+  loop_.Run();
+  EXPECT_EQ(std::string("test"), test_app_->GetLastTestString());
+}
+
+}  // namespace
+}  // namespace shell
+}  // namespace mojo