Implement browser test for It2Me

Implement browser test for It2Me

This CL implements a browser test for It2Me connection by automating
the steps below:
1. Generates an access code.
2. Launches another Chrome Remote Desktop App instance.
3. Connects with the generated access code.
4. Verifies that the session is connected.

This CL also
1. Modifies LaunchingChromotingApp to takes an optional parameter
   so that it can launch the app in a new tab instead of the current tab.
2. Allows a JavaScript browser test to send a value back to the C++
   code when a browser test succeeds.

BUG=446342
Committed: https://crrev.com/d90fe697c8b0ae5325ca5d2acccced2e284050b6
Cr-Commit-Position: refs/heads/master@{#311159}

[kelvinp, 2015-01-06 00:47:27]

kelvinp@chromium.org changed reviewers:
+ jamiewalch@chromium.org, weitaosu@chromium.org

[kelvinp, 2015-01-06 00:51:04]

PTAL

[Jamie, 2015-01-06 22:18:52]

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/it2me_b...
File chrome/test/remoting/it2me_browsertest.cc (right):

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/it2me_b...
chrome/test/remoting/it2me_browsertest.cc:20: LoadScript(app_web_content(),
FILE_PATH_LITERAL("browser_test.js"));
This is basically the same code as RemoteDesktopBrowserTest::SetUpTestForMe2Me.
I think you could rename that method, make it expand both sections (not just the
Me2Me section) and use it for IT2Me tests as well. If there's a reason why we
need separate methods, then they should at least be implemented by the same
class.

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/it2me_b...
chrome/test/remoting/it2me_browsertest.cc:25: RunJavaScriptTest(contents,
"GET_ACCESS_CODE", "{}", &return_value);
GET_ACCESS_CODE is not a test in the same sense that CONNECT_IT2ME is, so it
seems strange to use RunJavaScriptTest to run it. Is there anything about
RunScriptAndExtractString that prevents you using that here?

It's also dependent on it2me_browser_test.js being loaded, which is not ideal in
a helper function. SetUpTestForIt2Me method should probable load the file that
defines it (whether that be it2me_browser_test.js or another file dedicated to
helper functions).

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/it2me_b...
chrome/test/remoting/it2me_browsertest.cc:38: LaunchChromotingApp(false /*
defer_start*/, NEW_FOREGROUND_TAB);
Does NEW_FOREGROUND_TAB work for apps v2?

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/it2me_b...
chrome/test/remoting/it2me_browsertest.cc:38: LaunchChromotingApp(false /*
defer_start*/, NEW_FOREGROUND_TAB);
We haven't documented the |false| parameter anywhere else, so I don't think it's
necessary here. If you want to keep it, you should add a space before */ for
consistency with other comments.

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/it2me_b...
chrome/test/remoting/it2me_browsertest.cc:39: content::WebContents*
helper_content = active_web_contents();
Would it be possible to return the web contents from LaunchChromotingApp? I
think this code would be clearer if it looked like:

  helper_content = LaunchChromotingApp(...);
  LoadScript(helper_content,...);

But I don't know if that's feasible given the current implementation.

There's also duplication here between SetUpTestFor* and the code that sets up
the new tab (both start the app and load the same JS files). Ideally, we'd be
able to share that code.

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/it2me_b...
chrome/test/remoting/it2me_browsertest.cc:48: }
Can you also add a test to make sure that trying to connect with an invalid
access code doesn't work?

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/remote_...
File chrome/test/remoting/remote_desktop_browsertest.cc (right):

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_desktop_browsertest.cc:775: delete return_value;
No need to test for null before deleting. Better still, can you use a scoped_ptr
to do the delete for you?

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/remote_...
File chrome/test/remoting/remote_desktop_browsertest.h (right):

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_desktop_browsertest.h:96: WindowOpenDisposition
opt_window_open_disposition);
window_open_disposition is not optional, so the opt_ prefix is not appropriate
here.

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_desktop_browsertest.h:302: base::Value**
opt_return_value);
Similarly, opt_ is not appropriate here (although it would be better if you
could use ExecuteScriptAndExtract* in place of this new method).

https://codereview.chromium.org/838543002/diff/1/remoting/webapp/browser_test...
File remoting/webapp/browser_test/browser_test.js (right):

https://codereview.chromium.org/838543002/diff/1/remoting/webapp/browser_test...
remoting/webapp/browser_test/browser_test.js:59: ((result.value !== undefined)?
' Value: ' + result.value :  ''));
Nit: Space before '?'

https://codereview.chromium.org/838543002/diff/1/remoting/webapp/browser_test...
remoting/webapp/browser_test/browser_test.js:102: * @param {?} opt_value An
optional value to return when the test succeeds.
I think this should be ?=

https://codereview.chromium.org/838543002/diff/1/remoting/webapp/browser_test...
File remoting/webapp/browser_test/it2me_browser_test.js (right):

https://codereview.chromium.org/838543002/diff/1/remoting/webapp/browser_test...
remoting/webapp/browser_test/it2me_browser_test.js:36:
browserTest.CONNECT_IT2ME.prototype.clickOnAccessButton = function() {
These two methods (and onUserInfoReady) should be annotated as @private.

https://codereview.chromium.org/838543002/diff/1/remoting/webapp/browser_test...
remoting/webapp/browser_test/it2me_browser_test.js:61: "The access code should
be numeric.");
It has to be a positive integer, so this test could be made stricter.

https://codereview.chromium.org/838543002/diff/1/remoting/webapp/browser_test...
remoting/webapp/browser_test/it2me_browser_test.js:72:
remoting.identity.getUserInfo(resolve, reject);
getUserInfo doesn't cache anything, so I'm not sure what you gain by calling it
prior to attempting to connect.

[kelvinp, 2015-01-09 22:15:40]

Patchset #2 (id:20001) has been deleted

[kelvinp, 2015-01-09 22:20:59]

Patchset #2 (id:40001) has been deleted

[kelvinp, 2015-01-09 22:21:30]

PTAL

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/it2me_b...
File chrome/test/remoting/it2me_browsertest.cc (right):

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/it2me_b...
chrome/test/remoting/it2me_browsertest.cc:20: LoadScript(app_web_content(),
FILE_PATH_LITERAL("browser_test.js"));
On 2015/01/06 22:18:51, Jamie wrote:
> This is basically the same code as
RemoteDesktopBrowserTest::SetUpTestForMe2Me.
> I think you could rename that method, make it expand both sections (not just
the
> Me2Me section) and use it for IT2Me tests as well. If there's a reason why we
> need separate methods, then they should at least be implemented by the same
> class.

Removed.

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/it2me_b...
chrome/test/remoting/it2me_browsertest.cc:25: RunJavaScriptTest(contents,
"GET_ACCESS_CODE", "{}", &return_value);
On 2015/01/06 22:18:51, Jamie wrote:
> GET_ACCESS_CODE is not a test in the same sense that CONNECT_IT2ME is, so it
> seems strange to use RunJavaScriptTest to run it. Is there anything about
> RunScriptAndExtractString that prevents you using that here?
> 
> It's also dependent on it2me_browser_test.js being loaded, which is not ideal
in
> a helper function. SetUpTestForIt2Me method should probable load the file that
> defines it (whether that be it2me_browser_test.js or another file dedicated to
> helper functions).

Discussed offline

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/it2me_b...
chrome/test/remoting/it2me_browsertest.cc:38: LaunchChromotingApp(false /*
defer_start*/, NEW_FOREGROUND_TAB);
On 2015/01/06 22:18:51, Jamie wrote:
> We haven't documented the |false| parameter anywhere else, so I don't think
it's
> necessary here. If you want to keep it, you should add a space before */ for
> consistency with other comments.

Done.

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/it2me_b...
chrome/test/remoting/it2me_browsertest.cc:38: LaunchChromotingApp(false /*
defer_start*/, NEW_FOREGROUND_TAB);
On 2015/01/06 22:18:51, Jamie wrote:
> Does NEW_FOREGROUND_TAB work for apps v2?

Good point.  Comment added and modified the code to always use NEW_WINDOW for v2
app.

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/it2me_b...
chrome/test/remoting/it2me_browsertest.cc:39: content::WebContents*
helper_content = active_web_contents();
On 2015/01/06 22:18:51, Jamie wrote:
> Would it be possible to return the web contents from LaunchChromotingApp? I
> think this code would be clearer if it looked like:
> 
>   helper_content = LaunchChromotingApp(...);
>   LoadScript(helper_content,...);
> 
> But I don't know if that's feasible given the current implementation.
> 
> There's also duplication here between SetUpTestFor* and the code that sets up
> the new tab (both start the app and load the same JS files). Ideally, we'd be
> able to share that code.

Done.

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/it2me_b...
chrome/test/remoting/it2me_browsertest.cc:48: }
On 2015/01/06 22:18:51, Jamie wrote:
> Can you also add a test to make sure that trying to connect with an invalid
> access code doesn't work?

Done.

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/remote_...
File chrome/test/remoting/remote_desktop_browsertest.cc (right):

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_desktop_browsertest.cc:775: delete return_value;
On 2015/01/06 22:18:51, Jamie wrote:
> No need to test for null before deleting. Better still, can you use a
scoped_ptr
> to do the delete for you?

Done.

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/remote_...
File chrome/test/remoting/remote_desktop_browsertest.h (right):

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_desktop_browsertest.h:96: WindowOpenDisposition
opt_window_open_disposition);
On 2015/01/06 22:18:51, Jamie wrote:
> window_open_disposition is not optional, so the opt_ prefix is not appropriate
> here.

Done.

https://codereview.chromium.org/838543002/diff/1/chrome/test/remoting/remote_...
chrome/test/remoting/remote_desktop_browsertest.h:302: base::Value**
opt_return_value);
On 2015/01/06 22:18:51, Jamie wrote:
> Similarly, opt_ is not appropriate here (although it would be better if you
> could use ExecuteScriptAndExtract* in place of this new method).

Done.

https://codereview.chromium.org/838543002/diff/1/remoting/webapp/browser_test...
File remoting/webapp/browser_test/browser_test.js (right):

https://codereview.chromium.org/838543002/diff/1/remoting/webapp/browser_test...
remoting/webapp/browser_test/browser_test.js:59: ((result.value !== undefined)?
' Value: ' + result.value :  ''));
On 2015/01/06 22:18:52, Jamie wrote:
> Nit: Space before '?'

Done.

https://codereview.chromium.org/838543002/diff/1/remoting/webapp/browser_test...
remoting/webapp/browser_test/browser_test.js:102: * @param {?} opt_value An
optional value to return when the test succeeds.
On 2015/01/06 22:18:52, Jamie wrote:
> I think this should be ?=

Done.

https://codereview.chromium.org/838543002/diff/1/remoting/webapp/browser_test...
File remoting/webapp/browser_test/it2me_browser_test.js (right):

https://codereview.chromium.org/838543002/diff/1/remoting/webapp/browser_test...
remoting/webapp/browser_test/it2me_browser_test.js:36:
browserTest.CONNECT_IT2ME.prototype.clickOnAccessButton = function() {
On 2015/01/06 22:18:52, Jamie wrote:
> These two methods (and onUserInfoReady) should be annotated as @private.

Done.

https://codereview.chromium.org/838543002/diff/1/remoting/webapp/browser_test...
remoting/webapp/browser_test/it2me_browser_test.js:61: "The access code should
be numeric.");
On 2015/01/06 22:18:52, Jamie wrote:
> It has to be a positive integer, so this test could be made stricter.

Done.

https://codereview.chromium.org/838543002/diff/1/remoting/webapp/browser_test...
remoting/webapp/browser_test/it2me_browser_test.js:72:
remoting.identity.getUserInfo(resolve, reject);
On 2015/01/06 22:18:52, Jamie wrote:
> getUserInfo doesn't cache anything, so I'm not sure what you gain by calling
it
> prior to attempting to connect.

It2me currently calls getCachedEmail() to provide email credentials when
attempting to connect.  However, if we call getCachedEmail() before the user
info is first populated, getCachedEmail() will return null and the connection
will fail.

So this function basically force a call to getUserInfo() to ensure
getCachedEmail() returns a non-null value.

I think I should fix the problem in the production code instead of making the
test to workaround it.

[Jamie, 2015-01-10 00:14:24]

https://codereview.chromium.org/838543002/diff/1/remoting/webapp/browser_test...
File remoting/webapp/browser_test/it2me_browser_test.js (right):

https://codereview.chromium.org/838543002/diff/1/remoting/webapp/browser_test...
remoting/webapp/browser_test/it2me_browser_test.js:36:
browserTest.CONNECT_IT2ME.prototype.clickOnAccessButton = function() {
On 2015/01/09 22:21:30, kelvinp wrote:
> On 2015/01/06 22:18:52, Jamie wrote:
> > These two methods (and onUserInfoReady) should be annotated as @private.
> 
> Done.

No you haven't ;)

https://codereview.chromium.org/838543002/diff/60001/chrome/test/remoting/it2...
File chrome/test/remoting/it2me_browsertest.cc (right):

https://codereview.chromium.org/838543002/diff/60001/chrome/test/remoting/it2...
chrome/test/remoting/it2me_browsertest.cc:28: LoadScript(helper_content,
FILE_PATH_LITERAL("browser_test.js"));
Since my CL adding LoadBrowserTestJavaScript is already CQ'd, you might as well
wait until it lands and call that instead of loading browser_test.js explicitly
(though you don't technically need all the files it loads, it will make this
test more robust to refactorings of browser_test.js).

https://codereview.chromium.org/838543002/diff/60001/chrome/test/remoting/it2...
chrome/test/remoting/it2me_browsertest.cc:45:
IN_PROC_BROWSER_TEST_F(It2MeBrowserTest, MANUAL_InvalidAccessCode) {
We should have two tests here: one to check that an invalid "cloud id" causes a
failure, and one to check that an invalid PIN causes a failure. It can be a
follow-up CL, because doing it in a robust way is somewhat involved because we
don't want this test to have knowledge of how the PIN is composed (we'll
probably have to call out to JS to decompose the string we extract from the DOM
and to recompose it after corrupting it). As you pointed out off-line, we should
probably modify the directory server to have a specific "cloud id" reserved for
testing so that we don't run the risk of accidentally picking a valid one.

https://codereview.chromium.org/838543002/diff/60001/chrome/test/remoting/rem...
File chrome/test/remoting/remote_desktop_browsertest.cc (right):

https://codereview.chromium.org/838543002/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_desktop_browsertest.cc:191: window_open_disposition
= NEW_WINDOW;
Could we get rid of the disposition parameter if we made the It2Me test v2-only?
It's a shame to need a separate version of this method just so that we can pass
in a disposition for one test (especially when that disposition is overridden
for v2 anyway).

https://codereview.chromium.org/838543002/diff/60001/chrome/test/remoting/rem...
File chrome/test/remoting/remote_desktop_browsertest.h (right):

https://codereview.chromium.org/838543002/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_desktop_browsertest.h:101: content::WebContents**
app_web_content);
Why not just return the WebContents pointer? It would save you defining a
separate version that doesn't take the additional OUT parameter.

https://codereview.chromium.org/838543002/diff/60001/remoting/remoting_webapp...
File remoting/remoting_webapp_files.gypi (right):

https://codereview.chromium.org/838543002/diff/60001/remoting/remoting_webapp...
remoting/remoting_webapp_files.gypi:198:
'<@(remoting_webapp_js_browser_test_files)'
I don't think you meant to leave this in for review.

[Jamie, 2015-01-10 00:15:08]

With the exception of the change that I called out as being a follow-up, my
remaining comments are nits, so LGTM unless you want me to take another look.

[kelvinp, 2015-01-12 21:00:07]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2015-01-12 21:01:02]

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

[commit-bot: I haz the power, 2015-01-12 21:06:49]

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

[commit-bot: I haz the power, 2015-01-12 21:06:50]

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

[kelvinp, 2015-01-12 21:15:24]

FYI

https://codereview.chromium.org/838543002/diff/60001/chrome/test/remoting/it2...
File chrome/test/remoting/it2me_browsertest.cc (right):

https://codereview.chromium.org/838543002/diff/60001/chrome/test/remoting/it2...
chrome/test/remoting/it2me_browsertest.cc:28: LoadScript(helper_content,
FILE_PATH_LITERAL("browser_test.js"));
On 2015/01/10 00:14:24, Jamie wrote:
> Since my CL adding LoadBrowserTestJavaScript is already CQ'd, you might as
well
> wait until it lands and call that instead of loading browser_test.js
explicitly
> (though you don't technically need all the files it loads, it will make this
> test more robust to refactorings of browser_test.js).

Acknowledged.

https://codereview.chromium.org/838543002/diff/60001/chrome/test/remoting/it2...
chrome/test/remoting/it2me_browsertest.cc:45:
IN_PROC_BROWSER_TEST_F(It2MeBrowserTest, MANUAL_InvalidAccessCode) {
On 2015/01/10 00:14:23, Jamie wrote:
> We should have two tests here: one to check that an invalid "cloud id" causes
a
> failure, and one to check that an invalid PIN causes a failure. It can be a
> follow-up CL, because doing it in a robust way is somewhat involved because we
> don't want this test to have knowledge of how the PIN is composed (we'll
> probably have to call out to JS to decompose the string we extract from the
DOM
> and to recompose it after corrupting it). As you pointed out off-line, we
should
> probably modify the directory server to have a specific "cloud id" reserved
for
> testing so that we don't run the risk of accidentally picking a valid one.

Acknowledged.

https://codereview.chromium.org/838543002/diff/60001/chrome/test/remoting/rem...
File chrome/test/remoting/remote_desktop_browsertest.cc (right):

https://codereview.chromium.org/838543002/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_desktop_browsertest.cc:191: window_open_disposition
= NEW_WINDOW;
On 2015/01/10 00:14:24, Jamie wrote:
> Could we get rid of the disposition parameter if we made the It2Me test
v2-only?
> It's a shame to need a separate version of this method just so that we can
pass
> in a disposition for one test (especially when that disposition is overridden
> for v2 anyway).

V2 test currently doesn't run reliably.  I prefer to keep the parameter now and
remove it when our v2 test becomes reliable

https://codereview.chromium.org/838543002/diff/60001/chrome/test/remoting/rem...
File chrome/test/remoting/remote_desktop_browsertest.h (right):

https://codereview.chromium.org/838543002/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_desktop_browsertest.h:101: content::WebContents**
app_web_content);
On 2015/01/10 00:14:24, Jamie wrote:
> Why not just return the WebContents pointer? It would save you defining a
> separate version that doesn't take the additional OUT parameter.

LaunchChromotingApp uses the ASSERT_MACRO which uses the return statement to do
early returns.  As a result, the calling function must return void.

https://codereview.chromium.org/838543002/diff/60001/remoting/remoting_webapp...
File remoting/remoting_webapp_files.gypi (right):

https://codereview.chromium.org/838543002/diff/60001/remoting/remoting_webapp...
remoting/remoting_webapp_files.gypi:198:
'<@(remoting_webapp_js_browser_test_files)'
On 2015/01/10 00:14:24, Jamie wrote:
> I don't think you meant to leave this in for review.

Done.

[kelvinp, 2015-01-12 22:27:41]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2015-01-12 22:28:39]

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

[commit-bot: I haz the power, 2015-01-12 22:32:45]

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

[commit-bot: I haz the power, 2015-01-12 22:32:46]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[kelvinp, 2015-01-12 23:47:14]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2015-01-12 23:48:04]

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

[commit-bot: I haz the power, 2015-01-13 00:33:04]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-01-13 00:34:15]

Patchset 5 (id:??) landed as
https://crrev.com/d90fe697c8b0ae5325ca5d2acccced2e284050b6
Cr-Commit-Position: refs/heads/master@{#311159}