Further refine mojo_example_apptests.

mojo/examples/apptest/example_apptest.cc

Index: mojo/examples/apptest/example_apptest.cc
diff --git a/mojo/examples/apptest/example_apptest.cc b/mojo/examples/apptest/example_apptest.cc
index c754c85e3b7b56dc959a8a357fe54dc7531593e7..af7ad53db5aa813fad8ce20eac07fa123b9fce3a 100644
--- a/mojo/examples/apptest/example_apptest.cc
+++ b/mojo/examples/apptest/example_apptest.cc
@@ -20,8 +20,9 @@
 
 namespace {
 
-// TODO(msw): Remove this once we can get ApplicationImpl from TLS.
-mojo::ApplicationImpl* g_application_impl_hack = NULL;
+// A shell handle and command line args used for numerous application instances.
+mojo::ScopedMessagePipeHandle g_shell_scoped_message_pipe_handle_hack;
+mojo::Array<mojo::String> g_args_hack;

[viettrungluu, 2014/10/15 20:15:50]

I don't think the individual tests actually need the real args.

E.g., a test may want to provide its own args.

[msw, 2014/10/16 00:34:09]

Okay, I've removed this for now and added TODOs to support (and exemplify) using
actual command line arguments.

 
 }  // namespace
 
@@ -29,21 +30,37 @@ namespace mojo {
 
 namespace {
 
+// These GTESTs exemplify application testing executed within mojo_shell.
+// GTEST command line arguments are supported amid application arguments:
+//   mojo_shell 'mojo:mojo_example_apptest arg1 --gtest_filter=foo arg2'
 class ExampleApptest : public testing::Test {

[viettrungluu, 2014/10/15 20:15:50]

Maybe provide this as a test fixture base class. The constructor could take a
vector of arguments.

[msw, 2014/10/16 00:34:09]

Done.

  public:
-  ExampleApptest() {
-    g_application_impl_hack->ConnectToService("mojo:mojo_example_service",
-                                              &example_service_);
+  ExampleApptest() : application_impl_(NULL) {
+    // New applications are constructed for each test to avoid persisting state.
+    application_impl_ = new mojo::ApplicationImpl(

[viettrungluu, 2014/10/15 20:15:50]

You may as well initialize this in the initializer list, no?

[msw, 2014/10/16 00:34:09]

I'm not sure that's easily possible. ApplicationImpl construction requires a
RunLoop afaict; that's only a member of the ExampleApptest subclass, and thus
not available during base class initialization. I've moved ApplicationImpl
construction (and init) to SetUp, but still, it seems like the base class will
need to provide a RunLoop, or I'll need to wrangle that a bit more... Do you
have any suggestions?

+        &example_client_application,
+        g_shell_scoped_message_pipe_handle_hack.Pass());
+    // Fake the application initialize call with actual command line arguments.
+    application_impl_->Initialize(g_args_hack.Clone());

[viettrungluu, 2014/10/15 20:15:50]

Arguably, since this is nontrivial work, this should be done in SetUp() instead?

[msw, 2014/10/16 00:34:09]

Done.

+    application_impl_->ConnectToService("mojo:mojo_example_service",

[viettrungluu, 2014/10/15 20:15:50]

(And this wouldn't be in the base class, of course.)

[msw, 2014/10/16 00:34:09]

Acknowledged.

+                                        &example_service_);
     example_service_.set_client(&example_client_);
   }
 
-  virtual ~ExampleApptest() override {}
+  virtual ~ExampleApptest() override {
+    g_shell_scoped_message_pipe_handle_hack = application_impl_->UnbindShell();
+    delete application_impl_;
+  }
 
  protected:
   ExampleServicePtr example_service_;
   ExampleClientImpl example_client_;
 
  private:
+  mojo::RunLoop loop_;
+  mojo::ExampleClientApplication example_client_application;
+  mojo::ApplicationImpl* application_impl_;
+
   MOJO_DISALLOW_COPY_AND_ASSIGN(ExampleApptest);
 };
 
@@ -86,29 +103,29 @@ TEST_F(ExampleApptest, RunCallbackViaService) {
 
 MojoResult MojoMain(MojoHandle shell_handle) {
   mojo::Environment env;
-  // TODO(msw): Destroy this ambient RunLoop before running tests.
-  //            Need to CancelWait() / PassMessagePipe() from the ShellPtr?
-  mojo::RunLoop loop;
-  mojo::ApplicationDelegate* delegate = new mojo::ExampleClientApplication();
-  mojo::ApplicationImpl app(delegate, shell_handle);
-  g_application_impl_hack = &app;
-  MOJO_CHECK(app.WaitForInitialize());
 
   {
+    // This RunLoop is used for init, and then destroyed before running tests.
+    mojo::RunLoop loop;
+
+    // Construct an ApplicationImpl just for the GTEST commandline arguments.
+    mojo::ApplicationDelegate dummy_application_delegate;
+    mojo::ApplicationImpl app(&dummy_application_delegate, shell_handle);
+    MOJO_CHECK(app.WaitForInitialize());
+
     // InitGoogleTest expects (argc + 1) elements, including a terminating NULL.
     // It also removes GTEST arguments from |argv| and updates the |argc| count.
-    const mojo::Array<mojo::String>& args = app.args();
-    MOJO_CHECK(args.size() < INT_MAX);
-    int argc = static_cast<int>(args.size());
+    g_args_hack = app.args().Clone();
+    MOJO_CHECK(g_args_hack.size() < INT_MAX);
+    int argc = static_cast<int>(g_args_hack.size());
     std::vector<char*> argv(argc + 1);
     for (int i = 0; i < argc; ++i)
-      argv[i] = const_cast<char*>(args[i].data());
+      argv[i] = const_cast<char*>(g_args_hack[i].data());
     argv[argc] = NULL;
     testing::InitGoogleTest(&argc, &argv[0]);
+    g_shell_scoped_message_pipe_handle_hack = app.UnbindShell();
   }
 
   mojo_ignore_result(RUN_ALL_TESTS());
-
-  delete delegate;
   return MOJO_RESULT_OK;
 }