Add support for writing unit tests for Mojo-backed apps/extensions APIs.

Add support for writing unit tests for Mojo-backed apps/extensions APIs.

This change adds the infrastructure for unit testing the JS parts of
apps and extensions APIs implemented on top of Mojo services. The test
environment provides a partial implementation of the JS test API
provided to browser tests. A TestServiceProvider implementation is
used to provide test implementations of the appropriate Mojo services to
the JS code.

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

[not at google - send to devlin, 2014-07-18 15:09:14]

https://codereview.chromium.org/399363002/diff/60001/extensions/renderer/api_...
File extensions/renderer/api_test_base.cc (right):

https://codereview.chromium.org/399363002/diff/60001/extensions/renderer/api_...
extensions/renderer/api_test_base.cc:50:
base::MessageLoop::current()->PostTask(FROM_HERE, run_loop_->QuitClosure());
do you mean to also pump the message loop here? how does just posting a
QuitClosure do anything? is that what run_loop_ does?

https://codereview.chromium.org/399363002/diff/60001/extensions/renderer/api_...
extensions/renderer/api_test_base.cc:112: env()->RegisterModule("json_schema",
IDR_JSON_SCHEMA_JS);
shame to need to do all of this, since it means that when something requires
another transitive dependency it needs to be updated in 2 places.

maybe you can pull out Dispatcher::PopulateSourceMap into a common place?

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...

like a CreateCoreSourceMap function... somewhere.

https://codereview.chromium.org/399363002/diff/60001/extensions/renderer/api_...
File extensions/renderer/api_test_base.h (right):

https://codereview.chromium.org/399363002/diff/60001/extensions/renderer/api_...
extensions/renderer/api_test_base.h:59: 
comment?

https://codereview.chromium.org/399363002/diff/60001/extensions/renderer/api_...
extensions/renderer/api_test_base.h:77: base::RunLoop run_loop_;
I don't think this class is meant to be used as a member variable:

https://code.google.com/p/chromium/codesearch#chromium/src/base/run_loop.h&q=...

https://codereview.chromium.org/399363002/diff/60001/extensions/test/data/api...
File extensions/test/data/api_test_base_unittest.js (right):

https://codereview.chromium.org/399363002/diff/60001/extensions/test/data/api...
extensions/test/data/api_test_base_unittest.js:30: var binding =
require('lastError');
by re-assigning binding each time aren't you nullifying lines 28-29?

https://codereview.chromium.org/399363002/diff/60001/extensions/test/data/tes...
File extensions/test/data/test_unittest_custom_bindings.js (right):

https://codereview.chromium.org/399363002/diff/60001/extensions/test/data/tes...
extensions/test/data/test_unittest_custom_bindings.js:7: // This is a fork of
src/extensions/renderer/resources/test_custom_bindings.js.
I'm not sure about this. It seems like you could at least pull out a small
Delegate-style interface. Forking a file seems quite heavyweight.

https://codereview.chromium.org/399363002/diff/60001/extensions/test/data/tes...
extensions/test/data/test_unittest_custom_bindings.js:379: 
not this

[Sam McNally, 2014-07-21 03:56:28]

https://codereview.chromium.org/399363002/diff/60001/extensions/renderer/api_...
File extensions/renderer/api_test_base.cc (right):

https://codereview.chromium.org/399363002/diff/60001/extensions/renderer/api_...
extensions/renderer/api_test_base.cc:50:
base::MessageLoop::current()->PostTask(FROM_HERE, run_loop_->QuitClosure());
On 2014/07/18 15:09:13, kalman wrote:
> do you mean to also pump the message loop here? how does just posting a
> QuitClosure do anything? is that what run_loop_ does?

The test functions and their callbacks run on the message loop. RunTest starts a
RunLoop to pump it. This is to stop the RunLoop when the tests finish.

https://codereview.chromium.org/399363002/diff/60001/extensions/renderer/api_...
extensions/renderer/api_test_base.cc:112: env()->RegisterModule("json_schema",
IDR_JSON_SCHEMA_JS);
On 2014/07/18 15:09:13, kalman wrote:
> shame to need to do all of this, since it means that when something requires
> another transitive dependency it needs to be updated in 2 places.
> 
> maybe you can pull out Dispatcher::PopulateSourceMap into a common place?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...
> 
> like a CreateCoreSourceMap function... somewhere.

Done.

https://codereview.chromium.org/399363002/diff/60001/extensions/renderer/api_...
File extensions/renderer/api_test_base.h (right):

https://codereview.chromium.org/399363002/diff/60001/extensions/renderer/api_...
extensions/renderer/api_test_base.h:59: 
On 2014/07/18 15:09:14, kalman wrote:
> comment?

Done.

https://codereview.chromium.org/399363002/diff/60001/extensions/renderer/api_...
extensions/renderer/api_test_base.h:77: base::RunLoop run_loop_;
On 2014/07/18 15:09:14, kalman wrote:
> I don't think this class is meant to be used as a member variable:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/run_loop.h&q=...

Done.

https://codereview.chromium.org/399363002/diff/60001/extensions/test/data/api...
File extensions/test/data/api_test_base_unittest.js (right):

https://codereview.chromium.org/399363002/diff/60001/extensions/test/data/api...
extensions/test/data/api_test_base_unittest.js:30: var binding =
require('lastError');
On 2014/07/18 15:09:14, kalman wrote:
> by re-assigning binding each time aren't you nullifying lines 28-29?

Done.

https://codereview.chromium.org/399363002/diff/60001/extensions/test/data/tes...
File extensions/test/data/test_unittest_custom_bindings.js (right):

https://codereview.chromium.org/399363002/diff/60001/extensions/test/data/tes...
extensions/test/data/test_unittest_custom_bindings.js:7: // This is a fork of
src/extensions/renderer/resources/test_custom_bindings.js.
On 2014/07/18 15:09:14, kalman wrote:
> I'm not sure about this. It seems like you could at least pull out a small
> Delegate-style interface. Forking a file seems quite heavyweight.

Done.

https://codereview.chromium.org/399363002/diff/60001/extensions/test/data/tes...
extensions/test/data/test_unittest_custom_bindings.js:379: 
On 2014/07/18 15:09:14, kalman wrote:
> not this

Done.

[raymes, 2014-07-24 08:17:39]

https://codereview.chromium.org/399363002/diff/180001/extensions/renderer/api...
File extensions/renderer/api_test_base.h (right):

https://codereview.chromium.org/399363002/diff/180001/extensions/renderer/api...
extensions/renderer/api_test_base.h:88: gin::Handle<TestServiceProvider>
service_provider_;
gin handles aren't safe to have on the heap, but this will probably work by
chance. Probably worth a noting in a comment, at least.

https://codereview.chromium.org/399363002/diff/180001/extensions/renderer/res...
File extensions/renderer/resources/test_custom_bindings.js (right):

https://codereview.chromium.org/399363002/diff/180001/extensions/renderer/res...
extensions/renderer/resources/test_custom_bindings.js:10: var delegate =
require('test_delegate');
'test_environment_specific_bindings' (or anything more descriptive than
delegate)

[Sam McNally, 2014-07-24 08:44:37]

https://codereview.chromium.org/399363002/diff/180001/extensions/renderer/api...
File extensions/renderer/api_test_base.h (right):

https://codereview.chromium.org/399363002/diff/180001/extensions/renderer/api...
extensions/renderer/api_test_base.h:88: gin::Handle<TestServiceProvider>
service_provider_;
On 2014/07/24 08:17:38, raymes wrote:
> gin handles aren't safe to have on the heap, but this will probably work by
> chance. Probably worth a noting in a comment, at least.

Done.

https://codereview.chromium.org/399363002/diff/180001/extensions/renderer/res...
File extensions/renderer/resources/test_custom_bindings.js (right):

https://codereview.chromium.org/399363002/diff/180001/extensions/renderer/res...
extensions/renderer/resources/test_custom_bindings.js:10: var delegate =
require('test_delegate');
On 2014/07/24 08:17:38, raymes wrote:
> 'test_environment_specific_bindings' (or anything more descriptive than
> delegate)

Done.

[not at google - send to devlin, 2014-07-25 00:17:58]

looks fine but 2 questions

https://codereview.chromium.org/399363002/diff/270001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/399363002/diff/270001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:493: std::make_pair(mojo::kCodecModuleName,
IDR_MOJO_CODEC_JS));
this mojo stuff is new. will it be needed outside a testing environment?

https://codereview.chromium.org/399363002/diff/270001/extensions/renderer/res...
File extensions/renderer/resources/test_custom_bindings.js (left):

https://codereview.chromium.org/399363002/diff/270001/extensions/renderer/res...
extensions/renderer/resources/test_custom_bindings.js:344:
apiFunctions.setHandleRequest('runWithUserGesture', function(callback) {
I guess you need to pull these out because userGestures is a native handler?

Is that a symptom of a larger problem, that you should be exposing a similar API
as Dispatcher::GetJsResources for native handlers? Since there doesn't seem to
be an inherent reason why you can't support these methods.

[Sam McNally, 2014-07-25 07:38:59]

https://codereview.chromium.org/399363002/diff/270001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/399363002/diff/270001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:493: std::make_pair(mojo::kCodecModuleName,
IDR_MOJO_CODEC_JS));
On 2014/07/25 00:17:57, kalman wrote:
> this mojo stuff is new. will it be needed outside a testing environment?

They will eventually, but not yet.

https://codereview.chromium.org/399363002/diff/270001/extensions/renderer/res...
File extensions/renderer/resources/test_custom_bindings.js (left):

https://codereview.chromium.org/399363002/diff/270001/extensions/renderer/res...
extensions/renderer/resources/test_custom_bindings.js:344:
apiFunctions.setHandleRequest('runWithUserGesture', function(callback) {
On 2014/07/25 00:17:57, kalman wrote:
> I guess you need to pull these out because userGestures is a native handler?
> 
> Is that a symptom of a larger problem, that you should be exposing a similar
API
> as Dispatcher::GetJsResources for native handlers? Since there doesn't seem to
> be an inherent reason why you can't support these methods.

Done, except for getApiFeatures which requires a real Dispatcher.

[not at google - send to devlin, 2014-07-25 15:59:07]

lgtm

https://codereview.chromium.org/399363002/diff/370001/extensions/renderer/dis...
File extensions/renderer/dispatcher.h (right):

https://codereview.chromium.org/399363002/diff/370001/extensions/renderer/dis...
extensions/renderer/dispatcher.h:143: static void
RegisterNativeHandlers(ModuleSystem* module_system,
document this method and mention that |dispatcher|, |request_sender|, and
|v8_schema_registry| can all be NULL.

it's pretty weird to have a static method on Dispatcher which takes a Dispatcher
that can be NULL. oh well, no big deal.

[Sam McNally, 2014-07-25 22:38:11]

+darin for the DEPS changes.

[darin (slow to review), 2014-07-25 22:53:32]

LGTM for DEPS change

[Sam McNally, 2014-07-25 23:00:07]

The CQ bit was checked by sammc@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sammc@chromium.org/399363002/370001

[commit-bot: I haz the power, 2014-07-25 23:08:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[Sam McNally, 2014-07-25 23:09:32]

The CQ bit was unchecked by sammc@chromium.org

[Sam McNally, 2014-07-25 23:22:03]

The CQ bit was checked by sammc@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sammc@chromium.org/399363002/390001

[Sam McNally, 2014-07-26 01:06:25]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-07-26 05:00:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sammc@chromium.org/399363002/410001

[Sam McNally, 2014-07-26 11:11:22]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-07-26 11:13:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sammc@chromium.org/399363002/430001

[commit-bot: I haz the power, 2014-07-26 19:48:08]

Change committed as 285792