mojo: add some end-to-end shell tests and a new test sample app

mojo: add some end-to-end shell tests and a new test sample app

* Adds several shell integration tests that connect multiple services and applications together.
* Adds ToyMonitoringService and associated standalone application. Used to collect data about various services that connect with it and provide a report (e.g to the shell) for test verification purposes.

BUG=378155
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280956

[tim (not reviewing), 2014-06-24 19:28:42]

On 2014/06/24 19:24:08, timsteele wrote:

Note my main goal so far was to convince myself I could something meaningful
working :) There are a few things in this patch I might clean up, for example,
the way I organized the 'monitoring' files under test_service and grouped the
bindings together with test_service_bindings.  I thought that as part of the
review I'd take a step back to see if there were any opportunities to clean up /
make it easier to extend the shell tests afterwards.

[tim (not reviewing), 2014-06-26 02:27:48]

On 2014/06/24 19:28:42, timsteele wrote:
> On 2014/06/24 19:24:08, timsteele wrote:
> 
> Note my main goal so far was to convince myself I could something meaningful
> working :) There are a few things in this patch I might clean up, for example,
> the way I organized the 'monitoring' files under test_service and grouped the
> bindings together with test_service_bindings.  I thought that as part of the
> review I'd take a step back to see if there were any opportunities to clean up
/
> make it easier to extend the shell tests afterwards.

Rebased with davemoore's ServiceProvider changes.

[viettrungluu, 2014-06-30 17:08:25]

https://codereview.chromium.org/349303006/diff/60001/mojo/services/test_servi...
File mojo/services/test_service/test_service.mojom (right):

https://codereview.chromium.org/349303006/diff/60001/mojo/services/test_servi...
mojo/services/test_service/test_service.mojom:8: const string
kWelcomeMessagePrefix = "Party like it's ";
I'm "surprised" we support const strings; I'm not sure if we should[*] so
perhaps we should avoid using them.

[*] Currently, the mojom bindings generator doesn't do any
validation/escaping/etc. of strings, and we don't need string literals
elsewhere.

https://codereview.chromium.org/349303006/diff/60001/mojo/services/test_servi...
mojo/services/test_service/test_service.mojom:11: StartMonitoring() => ();
Does this really need to have a response?

https://codereview.chromium.org/349303006/diff/60001/mojo/services/test_servi...
File mojo/services/test_service/toy_monitoring_service_impl.cc (right):

https://codereview.chromium.org/349303006/diff/60001/mojo/services/test_servi...
mojo/services/test_service/toy_monitoring_service_impl.cc:25:
assert(context_->ids_to_names.find(client_id) !=
You can use base/logging.h (CHECK/DCHECK) here.

https://codereview.chromium.org/349303006/diff/60001/mojo/shell/shell_test_ba...
File mojo/shell/shell_test_base_unittest.cc (right):

https://codereview.chromium.org/349303006/diff/60001/mojo/shell/shell_test_ba...
mojo/shell/shell_test_base_unittest.cc:20: using mojo::test::ServiceReport;
I wonder how specific to shell_tests we should make the "test service".

Option 1: Decide that it'll only be used in shell_tests, in which case probably
its .mojom should probably be in the mojo.shell.test "module" (and perhaps it
should be renamed to "shell_test_service", or is that "shell_tests_service"?).

Option 2: Make it generic (e.g., so that it could be reasonably used from other
tests -- other other services, languages, etc.), in which case we should be a
bit careful about what things we put in it.

https://codereview.chromium.org/349303006/diff/60001/mojo/shell/shell_test_ba...
mojo/shell/shell_test_base_unittest.cc:223: EXPECT_EQ("mojo::test::TestService",
reports[0].service_name);
Probably you should use the "Name_" member, rather than hardcoding it. (Probably
we'll change the name components to be separate by "." rather than "::".)

https://codereview.chromium.org/349303006/diff/60001/mojo/shell/shell_test_ba...
mojo/shell/shell_test_base_unittest.cc:258: EXPECT_EQ("mojo::test::TestService",
reports[0].service_name);
"

[tim (not reviewing), 2014-06-30 22:20:55]

+mpcomplete for comment about const strings; I'm not sure what the right thing
is here but it sounds like it might be bad to support const strings in mojoms at
the moment.

https://codereview.chromium.org/349303006/diff/60001/mojo/services/test_servi...
File mojo/services/test_service/test_service.mojom (right):

https://codereview.chromium.org/349303006/diff/60001/mojo/services/test_servi...
mojo/services/test_service/test_service.mojom:8: const string
kWelcomeMessagePrefix = "Party like it's ";
On 2014/06/30 17:08:24, viettrungluu wrote:
> I'm "surprised" we support const strings; I'm not sure if we should[*] so
> perhaps we should avoid using them.
> 
> [*] Currently, the mojom bindings generator doesn't do any
> validation/escaping/etc. of strings, and we don't need string literals
> elsewhere.

Interesting. I saw a const string in sample_service.mojom and also
https://codereview.chromium.org/268363003, and assumed this was OK.

I can remove it, I was only using it for convenience here, but I could imagine
some services wanting to specify constant strings like this at some point.

https://codereview.chromium.org/349303006/diff/60001/mojo/services/test_servi...
mojo/services/test_service/test_service.mojom:11: StartMonitoring() => ();
It wouldn't if I didn't care about verifying certain things are happening by a
certain point. Otherwise something like

service->StartMonitoring
service->Ping
...
loop->Run

means the pings will always hit the service before the monitoring handshake has
completed, which is fine except that you can't query the (separate) monitoring
service and EXPECT_ that the pings were registered (yet).

So to facilitate testing I opted to make it easy to track progress of the
different steps.  

On 2014/06/30 17:08:24, viettrungluu wrote:
> Does this really need to have a response?

https://codereview.chromium.org/349303006/diff/60001/mojo/services/test_servi...
File mojo/services/test_service/toy_monitoring_service_impl.cc (right):

https://codereview.chromium.org/349303006/diff/60001/mojo/services/test_servi...
mojo/services/test_service/toy_monitoring_service_impl.cc:25:
assert(context_->ids_to_names.find(client_id) !=
On 2014/06/30 17:08:24, viettrungluu wrote:
> You can use base/logging.h (CHECK/DCHECK) here.

Done.

https://codereview.chromium.org/349303006/diff/60001/mojo/shell/shell_test_ba...
File mojo/shell/shell_test_base_unittest.cc (right):

https://codereview.chromium.org/349303006/diff/60001/mojo/shell/shell_test_ba...
mojo/shell/shell_test_base_unittest.cc:20: using mojo::test::ServiceReport;
On 2014/06/30 17:08:25, viettrungluu wrote:
> I wonder how specific to shell_tests we should make the "test service".
> 
> Option 1: Decide that it'll only be used in shell_tests, in which case
probably
> its .mojom should probably be in the mojo.shell.test "module" (and perhaps it
> should be renamed to "shell_test_service", or is that "shell_tests_service"?).
> 
> Option 2: Make it generic (e.g., so that it could be reasonably used from
other
> tests -- other other services, languages, etc.), in which case we should be a
> bit careful about what things we put in it.

Good question. I set out wanting to keep things relatively generic, but I also
think it makes sense to have "example real applications that we run as part of
tests" doing real things in an integration suite. We should be able to call it
one way or the other though, and truth be told I probably wrote most of this
assuming it was specific to "shell tests".

Rationale:
"get time" made sense to me as a basic "test service" function (in the same vein
as "ping"). So I created the TestTimeService to have a service return a
deterministic time. [I've since renamed the TestService method to make it more
generically "contact a different app to get-and-ferry data back to the requestor
for verification"]. 

Reporting the number of requests made to a service is a scenario inspired by the
ServiceManagerTest, where this is done via explicit interaction with the impl
classes as the primary way to verify communication worked as expected. In the
same way, reporting #requests makes sense to me as part of a generic test
service, e.g

StartReportingRequestsTo(TestRequestTracker tracker)

Implementing it so that you can collect this data from an arbitrary number of
services in your suite all through IDLs is a bit more interesting than Ping and
GetTime, and was an opportunity to exercise cross-app per-connection
client<>server relationships established / torn-down by the shell.

If we excluded it from the TestService then other {services, languages} using
TestService would quite possibly come up with their own methods. I decided to
make it easy enough to use if you wanted to but not require it (which is why I
didn't connect the monitoring on construction of TestServiceImpl). 

I could see us creating a separate ShellTestServiceForReportingTests with its
own "ping" methods and reporting built in and consider it as in the "Option 1" /
"example real applications that we run as part of tests" camp, or leaving things
mostly as is in the "Option 2" camp, probably with a few renames,
ToyMonitoringService => TestRequestTracker).  

I think I'm leaning towards the latter for now, given that it is not totally
clear (and perhaps growing the test suite is the only real way to know?), and
given that nothing I implemented here service-wise is all that complicated and I
can keep watch / move it pretty easily if it becomes clearer.

https://codereview.chromium.org/349303006/diff/60001/mojo/shell/shell_test_ba...
mojo/shell/shell_test_base_unittest.cc:223: EXPECT_EQ("mojo::test::TestService",
reports[0].service_name);
On 2014/06/30 17:08:25, viettrungluu wrote:
> Probably you should use the "Name_" member, rather than hardcoding it.
(Probably
> we'll change the name components to be separate by "." rather than "::".)

Done.

https://codereview.chromium.org/349303006/diff/60001/mojo/shell/shell_test_ba...
mojo/shell/shell_test_base_unittest.cc:258: EXPECT_EQ("mojo::test::TestService",
reports[0].service_name);
On 2014/06/30 17:08:24, viettrungluu wrote:
> "

Done.

[viettrungluu, 2014-06-30 22:31:32]

On 2014/06/30 22:20:55, timsteele wrote:
> +mpcomplete for comment about const strings; I'm not sure what the right thing
> is here but it sounds like it might be bad to support const strings in mojoms
at
> the moment.

(His calendar says he's OOO this week.)

> 
>
https://codereview.chromium.org/349303006/diff/60001/mojo/services/test_servi...
> File mojo/services/test_service/test_service.mojom (right):
> 
>
https://codereview.chromium.org/349303006/diff/60001/mojo/services/test_servi...
> mojo/services/test_service/test_service.mojom:8: const string
> kWelcomeMessagePrefix = "Party like it's ";
> On 2014/06/30 17:08:24, viettrungluu wrote:
> > I'm "surprised" we support const strings; I'm not sure if we should[*] so
> > perhaps we should avoid using them.
> > 
> > [*] Currently, the mojom bindings generator doesn't do any
> > validation/escaping/etc. of strings, and we don't need string literals
> > elsewhere.
> 
> Interesting. I saw a const string in sample_service.mojom and also
> https://codereview.chromium.org/268363003, and assumed this was OK.

Everyone likes to add features, except for me who likes to remove them. :)

There's just a lot of things to deal to properly handle strings, and we're not
even close.

> 
> I can remove it, I was only using it for convenience here, but I could imagine
> some services wanting to specify constant strings like this at some point.

I agree that at some point it might be desirable, but I'd prefer to put that off
for now....

> 
>
https://codereview.chromium.org/349303006/diff/60001/mojo/services/test_servi...
> mojo/services/test_service/test_service.mojom:11: StartMonitoring() => ();
> It wouldn't if I didn't care about verifying certain things are happening by a
> certain point. Otherwise something like
> 
> service->StartMonitoring
> service->Ping
> ...
> loop->Run
> 
> means the pings will always hit the service before the monitoring handshake
has
> completed, which is fine except that you can't query the (separate) monitoring
> service and EXPECT_ that the pings were registered (yet).
> 
> So to facilitate testing I opted to make it easy to track progress of the
> different steps.  

Ah, good point.

> 
> On 2014/06/30 17:08:24, viettrungluu wrote:
> > Does this really need to have a response?
> 
>
https://codereview.chromium.org/349303006/diff/60001/mojo/services/test_servi...
> File mojo/services/test_service/toy_monitoring_service_impl.cc (right):
> 
>
https://codereview.chromium.org/349303006/diff/60001/mojo/services/test_servi...
> mojo/services/test_service/toy_monitoring_service_impl.cc:25:
> assert(context_->ids_to_names.find(client_id) !=
> On 2014/06/30 17:08:24, viettrungluu wrote:
> > You can use base/logging.h (CHECK/DCHECK) here.
> 
> Done.
> 
>
https://codereview.chromium.org/349303006/diff/60001/mojo/shell/shell_test_ba...
> File mojo/shell/shell_test_base_unittest.cc (right):
> 
>
https://codereview.chromium.org/349303006/diff/60001/mojo/shell/shell_test_ba...
> mojo/shell/shell_test_base_unittest.cc:20: using mojo::test::ServiceReport;
> On 2014/06/30 17:08:25, viettrungluu wrote:
> > I wonder how specific to shell_tests we should make the "test service".
> > 
> > Option 1: Decide that it'll only be used in shell_tests, in which case
> probably
> > its .mojom should probably be in the mojo.shell.test "module" (and perhaps
it
> > should be renamed to "shell_test_service", or is that
"shell_tests_service"?).
> > 
> > Option 2: Make it generic (e.g., so that it could be reasonably used from
> other
> > tests -- other other services, languages, etc.), in which case we should be
a
> > bit careful about what things we put in it.
> 
> Good question. I set out wanting to keep things relatively generic, but I also
> think it makes sense to have "example real applications that we run as part of
> tests" doing real things in an integration suite. We should be able to call it
> one way or the other though, and truth be told I probably wrote most of this
> assuming it was specific to "shell tests".
> 
> Rationale:
> "get time" made sense to me as a basic "test service" function (in the same
vein
> as "ping"). So I created the TestTimeService to have a service return a
> deterministic time. [I've since renamed the TestService method to make it more
> generically "contact a different app to get-and-ferry data back to the
requestor
> for verification"]. 
> 
> Reporting the number of requests made to a service is a scenario inspired by
the
> ServiceManagerTest, where this is done via explicit interaction with the impl
> classes as the primary way to verify communication worked as expected. In the
> same way, reporting #requests makes sense to me as part of a generic test
> service, e.g
> 
> StartReportingRequestsTo(TestRequestTracker tracker)
> 
> Implementing it so that you can collect this data from an arbitrary number of
> services in your suite all through IDLs is a bit more interesting than Ping
and
> GetTime, and was an opportunity to exercise cross-app per-connection
> client<>server relationships established / torn-down by the shell.
> 
> If we excluded it from the TestService then other {services, languages} using
> TestService would quite possibly come up with their own methods. I decided to
> make it easy enough to use if you wanted to but not require it (which is why I
> didn't connect the monitoring on construction of TestServiceImpl). 
> 
> I could see us creating a separate ShellTestServiceForReportingTests with its
> own "ping" methods and reporting built in and consider it as in the "Option 1"
/
> "example real applications that we run as part of tests" camp, or leaving
things
> mostly as is in the "Option 2" camp, probably with a few renames,
> ToyMonitoringService => TestRequestTracker).  
> 
> I think I'm leaning towards the latter for now, given that it is not totally
> clear (and perhaps growing the test suite is the only real way to know?), and
> given that nothing I implemented here service-wise is all that complicated and
I
> can keep watch / move it pretty easily if it becomes clearer.

Option 2 sounds good to me, with the rename as suggested.

> 
>
https://codereview.chromium.org/349303006/diff/60001/mojo/shell/shell_test_ba...
> mojo/shell/shell_test_base_unittest.cc:223:
EXPECT_EQ("mojo::test::TestService",
> reports[0].service_name);
> On 2014/06/30 17:08:25, viettrungluu wrote:
> > Probably you should use the "Name_" member, rather than hardcoding it.
> (Probably
> > we'll change the name components to be separate by "." rather than "::".)
> 
> Done.
> 
>
https://codereview.chromium.org/349303006/diff/60001/mojo/shell/shell_test_ba...
> mojo/shell/shell_test_base_unittest.cc:258:
EXPECT_EQ("mojo::test::TestService",
> reports[0].service_name);
> On 2014/06/30 17:08:24, viettrungluu wrote:
> > "
> 
> Done.

LGTM w/"Option 2 rename" and string const removal.

[tim (not reviewing), 2014-07-01 00:32:54]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-07-01 00:37:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tim@chromium.org/349303006/160001

[commit-bot: I haz the power, 2014-07-01 01:59:47]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-01 02:03:01]

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

[commit-bot: I haz the power, 2014-07-01 02:03:02]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[tim (not reviewing), 2014-07-02 01:01:02]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-07-02 01:02:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tim@chromium.org/349303006/340001

[commit-bot: I haz the power, 2014-07-02 07:04:19]

Change committed as 280956