Further refine mojo_example_apptests.

Further refine mojo_example_apptests.

Destroy MojoMain's RunLoop before executing tests.
Reuse the shell handle for multiple test app instances.
Add ApplicationImpl::UnbindShell for message pipe reuse.

Add Apptest, a GTEST base class for application testing.

TODO: Support/Exemplify actual command line arg use.

BUG=392646
TEST=mojo_shell 'mojo:mojo_example_apptests <gtest_args>' still works.
R=viettrungluu@chromium.org

Committed: https://crrev.com/1b38080e455f6ee9e47e97bc1e03c8565111cfc1
Cr-Commit-Position: refs/heads/master@{#300155}

[msw, 2014-10-14 22:52:11]

msw@chromium.org changed reviewers:
+ viettrungluu@chromium.org

[msw, 2014-10-15 00:17:35]

Patchset #2 (id:20001) has been deleted

[msw, 2014-10-15 00:21:28]

Hey Trung, please take a look; thanks!

[viettrungluu, 2014-10-15 20:15:51]

https://codereview.chromium.org/654103002/diff/40001/mojo/examples/apptest/ex...
File mojo/examples/apptest/example_apptest.cc (right):

https://codereview.chromium.org/654103002/diff/40001/mojo/examples/apptest/ex...
mojo/examples/apptest/example_apptest.cc:25: mojo::Array<mojo::String>
g_args_hack;
I don't think the individual tests actually need the real args.

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

https://codereview.chromium.org/654103002/diff/40001/mojo/examples/apptest/ex...
mojo/examples/apptest/example_apptest.cc:36: class ExampleApptest : public
testing::Test {
Maybe provide this as a test fixture base class. The constructor could take a
vector of arguments.

https://codereview.chromium.org/654103002/diff/40001/mojo/examples/apptest/ex...
mojo/examples/apptest/example_apptest.cc:40: application_impl_ = new
mojo::ApplicationImpl(
You may as well initialize this in the initializer list, no?

https://codereview.chromium.org/654103002/diff/40001/mojo/examples/apptest/ex...
mojo/examples/apptest/example_apptest.cc:44:
application_impl_->Initialize(g_args_hack.Clone());
Arguably, since this is nontrivial work, this should be done in SetUp() instead?

https://codereview.chromium.org/654103002/diff/40001/mojo/examples/apptest/ex...
mojo/examples/apptest/example_apptest.cc:45:
application_impl_->ConnectToService("mojo:mojo_example_service",
(And this wouldn't be in the base class, of course.)

[msw, 2014-10-16 00:30:13]

Patchset #3 (id:60001) has been deleted

[msw, 2014-10-16 00:30:18]

Patchset #3 (id:80001) has been deleted

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

Thanks for the good comments, please take another look!

https://codereview.chromium.org/654103002/diff/40001/mojo/examples/apptest/ex...
File mojo/examples/apptest/example_apptest.cc (right):

https://codereview.chromium.org/654103002/diff/40001/mojo/examples/apptest/ex...
mojo/examples/apptest/example_apptest.cc:25: mojo::Array<mojo::String>
g_args_hack;
On 2014/10/15 20:15:50, viettrungluu wrote:
> I don't think the individual tests actually need the real args.
> 
> E.g., a test may want to provide its own args.

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

https://codereview.chromium.org/654103002/diff/40001/mojo/examples/apptest/ex...
mojo/examples/apptest/example_apptest.cc:36: class ExampleApptest : public
testing::Test {
On 2014/10/15 20:15:50, viettrungluu wrote:
> Maybe provide this as a test fixture base class. The constructor could take a
> vector of arguments.

Done.

https://codereview.chromium.org/654103002/diff/40001/mojo/examples/apptest/ex...
mojo/examples/apptest/example_apptest.cc:40: application_impl_ = new
mojo::ApplicationImpl(
On 2014/10/15 20:15:50, viettrungluu wrote:
> You may as well initialize this in the initializer list, no?

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?

https://codereview.chromium.org/654103002/diff/40001/mojo/examples/apptest/ex...
mojo/examples/apptest/example_apptest.cc:44:
application_impl_->Initialize(g_args_hack.Clone());
On 2014/10/15 20:15:50, viettrungluu wrote:
> Arguably, since this is nontrivial work, this should be done in SetUp()
instead?

Done.

https://codereview.chromium.org/654103002/diff/40001/mojo/examples/apptest/ex...
mojo/examples/apptest/example_apptest.cc:45:
application_impl_->ConnectToService("mojo:mojo_example_service",
On 2014/10/15 20:15:50, viettrungluu wrote:
> (And this wouldn't be in the base class, of course.)

Acknowledged.

[viettrungluu, 2014-10-16 02:22:24]

LGTM w/nit.

https://codereview.chromium.org/654103002/diff/100001/mojo/examples/apptest/e...
File mojo/examples/apptest/example_apptest.cc (right):

https://codereview.chromium.org/654103002/diff/100001/mojo/examples/apptest/e...
mojo/examples/apptest/example_apptest.cc:33: application_impl_(NULL) {
nit: nullptr instead of NULL

[msw, 2014-10-16 19:40:59]

Please take another look. As discussed, I moved the RunLoop to the Apptest base
class and now the global is a POD MessagePipeHandle instance (lmk if you'd
prefer a MojoHandle).

https://codereview.chromium.org/654103002/diff/100001/mojo/examples/apptest/e...
File mojo/examples/apptest/example_apptest.cc (right):

https://codereview.chromium.org/654103002/diff/100001/mojo/examples/apptest/e...
mojo/examples/apptest/example_apptest.cc:33: application_impl_(NULL) {
On 2014/10/16 02:22:23, viettrungluu wrote:
> nit: nullptr instead of NULL

Done.

[viettrungluu, 2014-10-17 00:35:54]

lgtm

[msw, 2014-10-17 02:02:32]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-10-17 02:03:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/654103002/160001

[commit-bot: I haz the power, 2014-10-17 02:08:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-17 02:08:38]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_compile_dbg_32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2014-10-17 18:07:50]

Patchset #7 (id:180001) has been deleted

[msw, 2014-10-17 18:08:25]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-10-17 18:10:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/654103002/200001

[msw, 2014-10-17 18:23:20]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-10-17 18:23:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/654103002/220001

[commit-bot: I haz the power, 2014-10-17 19:14:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-17 19:14:17]

Try jobs failed on following builders:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[msw, 2014-10-17 20:22:53]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-10-17 20:23:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/654103002/220001

[commit-bot: I haz the power, 2014-10-17 20:24:56]

Committed patchset #8 (id:220001)

[commit-bot: I haz the power, 2014-10-17 20:27:22]

Patchset 8 (id:??) landed as
https://crrev.com/1b38080e455f6ee9e47e97bc1e03c8565111cfc1
Cr-Commit-Position: refs/heads/master@{#300155}