Basic tests for new webview based signin flow

Basic tests for new webview based signin flow

Extract common code for all login test from SamlTest.
Separate HTML for /embedded/setup/chromeos in fake_gaia.
Updated SAML test to use new GAIA endpoint in webview cases.

BUG=472095
TEST=browser_tests

Committed: https://crrev.com/e29cf14a9baa835b86f199f38bc1ac8db1feab5a
Cr-Commit-Position: refs/heads/master@{#324221}

[Dmitry Polukhin, 2015-04-03 14:54:01]

dpolukhin@chromium.org changed reviewers:
+ bartfab@chromium.org, xiyuan@chromium.org

[Dmitry Polukhin, 2015-04-03 14:54:02]

This CL is just for you information because I'm still debugging why
ExecuteScript hangs most of the time. Also much more work required for existing
SAML that expects old GAIA endpoint.

I'll let you know separate when CL will be ready for review for real.

[xiyuan, 2015-04-03 16:02:35]

On 2015/04/03 14:54:02, Dmitry Polukhin wrote:
> This CL is just for you information because I'm still debugging why
> ExecuteScript hangs most of the time. Also much more work required for
existing
> SAML that expects old GAIA endpoint.
> 
> I'll let you know separate when CL will be ready for review for real.

How feasible it is to merge the common code into LoginManagerTest instead of
creating a new base?

What problem did you see with webview.executeScript? It should not hang, but
might end up in an error, e.g. when the content is properly loaded in webview.
Is that what you are seeing?

[Dmitry Polukhin, 2015-04-03 16:59:57]

On 2015/04/03 16:02:35, xiyuan wrote:
> On 2015/04/03 14:54:02, Dmitry Polukhin wrote:
> > This CL is just for you information because I'm still debugging why
> > ExecuteScript hangs most of the time. Also much more work required for
> existing
> > SAML that expects old GAIA endpoint.
> > 
> > I'll let you know separate when CL will be ready for review for real.
> 
> How feasible it is to merge the common code into LoginManagerTest instead of
> creating a new base?

Unfortunately LoginManagerTest is much simpler and doesn't use fake_gaia at all
(it is used for testing offline authentication in most cases). If I add all
required things to LoginManagerTest, it will be something like LoginTestBase
that I extracted from SamlTest. Perhaps later we could replace LoginManagerTest
with new LoginTestBase.

> What problem did you see with webview.executeScript? It should not hang, but
> might end up in an error, e.g. when the content is properly loaded in webview.
> Is that what you are seeing?

Observed behavior that ExecuteJsInSigninFrame doesn't return so test times out.
I'll look it in debugger on Monday when get to the office.

[xiyuan, 2015-04-03 17:04:25]

On 2015/04/03 16:59:57, Dmitry Polukhin wrote:
> On 2015/04/03 16:02:35, xiyuan wrote:
> > On 2015/04/03 14:54:02, Dmitry Polukhin wrote:
> > > This CL is just for you information because I'm still debugging why
> > > ExecuteScript hangs most of the time. Also much more work required for
> > existing
> > > SAML that expects old GAIA endpoint.
> > > 
> > > I'll let you know separate when CL will be ready for review for real.
> > 
> > How feasible it is to merge the common code into LoginManagerTest instead of
> > creating a new base?
> 
> Unfortunately LoginManagerTest is much simpler and doesn't use fake_gaia at
all
> (it is used for testing offline authentication in most cases). If I add all
> required things to LoginManagerTest, it will be something like LoginTestBase
> that I extracted from SamlTest. Perhaps later we could replace
LoginManagerTest
> with new LoginTestBase.
> 

Then how about merge into OobeBaseTest? That uses FakeGaia and is the base for
OAuth2Test and KioskTest.

> > What problem did you see with webview.executeScript? It should not hang, but
> > might end up in an error, e.g. when the content is properly loaded in
webview.
> > Is that what you are seeing?
> 
> Observed behavior that ExecuteJsInSigninFrame doesn't return so test times
out.
> I'll look it in debugger on Monday when get to the office.

I see. I thought you were talking about webview.executeScripts. :)

[Dmitry Polukhin, 2015-04-06 14:18:41]

dpolukhin@chromium.org changed reviewers:
+ rogerta@chromium.org

[Dmitry Polukhin, 2015-04-06 15:09:45]

On 2015/04/03 17:04:25, xiyuan wrote:
> Then how about merge into OobeBaseTest? That uses FakeGaia and is the base for
> OAuth2Test and KioskTest.

OobeBaseTest looks like much better candidate so I'll try to enhance it to fit
my test and SAML tests.

> I see. I thought you were talking about webview.executeScripts. :)

I fixed it. It was my mistake because I used 'location.replace' in onload
handler. Now I use history.replaceState at it works.

[Dmitry Polukhin, 2015-04-07 10:25:06]

The CQ bit was checked by dpolukhin@chromium.org to run a CQ dry run

[Dmitry Polukhin, 2015-04-07 10:25:19]

The CQ bit was unchecked by dpolukhin@chromium.org

[Dmitry Polukhin, 2015-04-07 10:35:16]

dpolukhin@chromium.org changed reviewers:
+ nkostylev@chromium.org

[Dmitry Polukhin, 2015-04-07 10:35:17]

PTAL

[Dmitry Polukhin, 2015-04-07 10:36:27]

The CQ bit was checked by dpolukhin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-07 10:36:43]

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

[Dmitry Polukhin, 2015-04-07 10:37:28]

The CQ bit was unchecked by dpolukhin@chromium.org

[Nikita (slow), 2015-04-07 12:37:48]

lgtm with nits

https://codereview.chromium.org/1054403002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/test/oobe_base_test.cc (right):

https://codereview.chromium.org/1054403002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/test/oobe_base_test.cc:234:
content::RenderFrameHost* frame = InlineLoginUI::GetAuthIframe(
Please rename to InlineLoginUI::GetAuthFrame() since it works with both webview
and iframe.

https://codereview.chromium.org/1054403002/diff/60001/google_apis/gaia/fake_g...
File google_apis/gaia/fake_gaia.cc (right):

https://codereview.chromium.org/1054403002/diff/60001/google_apis/gaia/fake_g...
google_apis/gaia/fake_gaia.cc:233: // Handles /_/embedded/signin/challenge for
/embedded/setup/chromeos
embedded/lookup/accountlookup

https://codereview.chromium.org/1054403002/diff/60001/google_apis/test/embedd...
File google_apis/test/embedded_setup_chromeos.html (right):

https://codereview.chromium.org/1054403002/diff/60001/google_apis/test/embedd...
google_apis/test/embedded_setup_chromeos.html:1: <!-- <HTML> -->
Does it have to be in comment?

https://codereview.chromium.org/1054403002/diff/60001/google_apis/test/embedd...
google_apis/test/embedded_setup_chromeos.html:2: <HEAD>
nit: lowercase here and below

https://codereview.chromium.org/1054403002/diff/60001/google_apis/test/embedd...
google_apis/test/embedded_setup_chromeos.html:51: if (request.readyState == 4 &&
request.status == 200) {
nit: "4" - extract constant.

https://codereview.chromium.org/1054403002/diff/60001/google_apis/test/embedd...
google_apis/test/embedded_setup_chromeos.html:73:
gaia.chromeOSLogin.attemptLogin(email, password);
should follow "B-2" scenario

* Call clearOldAttempts on identifier page load
* Call attemptLogin with both email address and password when password is
entered

[Dmitry Polukhin, 2015-04-07 13:24:40]

https://codereview.chromium.org/1054403002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/test/oobe_base_test.cc (right):

https://codereview.chromium.org/1054403002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/test/oobe_base_test.cc:234:
content::RenderFrameHost* frame = InlineLoginUI::GetAuthIframe(
On 2015/04/07 12:37:48, Nikita Kostylev wrote:
> Please rename to InlineLoginUI::GetAuthFrame() since it works with both
webview
> and iframe.

Done.

https://codereview.chromium.org/1054403002/diff/60001/google_apis/gaia/fake_g...
File google_apis/gaia/fake_gaia.cc (right):

https://codereview.chromium.org/1054403002/diff/60001/google_apis/gaia/fake_g...
google_apis/gaia/fake_gaia.cc:233: // Handles /_/embedded/signin/challenge for
/embedded/setup/chromeos
On 2015/04/07 12:37:48, Nikita Kostylev wrote:
> embedded/lookup/accountlookup

Done.

https://codereview.chromium.org/1054403002/diff/60001/google_apis/test/embedd...
File google_apis/test/embedded_setup_chromeos.html (right):

https://codereview.chromium.org/1054403002/diff/60001/google_apis/test/embedd...
google_apis/test/embedded_setup_chromeos.html:1: <!-- <HTML> -->
On 2015/04/07 12:37:48, Nikita Kostylev wrote:
> Does it have to be in comment?

Done.

https://codereview.chromium.org/1054403002/diff/60001/google_apis/test/embedd...
google_apis/test/embedded_setup_chromeos.html:2: <HEAD>
On 2015/04/07 12:37:48, Nikita Kostylev wrote:
> nit: lowercase here and below

Done.

https://codereview.chromium.org/1054403002/diff/60001/google_apis/test/embedd...
google_apis/test/embedded_setup_chromeos.html:51: if (request.readyState == 4 &&
request.status == 200) {
On 2015/04/07 12:37:48, Nikita Kostylev wrote:
> nit: "4" - extract constant.

Discussed and decided to keep as is in tests. 200 - is HTTP_OK, 4 is request
completed.

https://codereview.chromium.org/1054403002/diff/60001/google_apis/test/embedd...
google_apis/test/embedded_setup_chromeos.html:73:
gaia.chromeOSLogin.attemptLogin(email, password);
On 2015/04/07 12:37:48, Nikita Kostylev wrote:
> should follow "B-2" scenario
> 
> * Call clearOldAttempts on identifier page load
> * Call attemptLogin with both email address and password when password is
> entered

Done.

[xiyuan, 2015-04-07 16:05:00]

LGTM with nits

https://codereview.chromium.org/1054403002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/test/oobe_base_test.cc (left):

https://codereview.chromium.org/1054403002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/test/oobe_base_test.cc:98: GURL gaia_url =
server_url.ReplaceComponents(replace_gaia_host);
Please keep this part. The intention of the code is to use a "gaia" as the host
name for FakeGaia. It is needed because KioskTest uses EmbeddedTestServer as
both Gaia and CWS. The default "localhost" would cause problems in
chrome_resource_dispatcher_host_delegate.cc [1]


[1]:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ren...

https://codereview.chromium.org/1054403002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/webview_login_browsertest.cc (right):

https://codereview.chromium.org/1054403002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/webview_login_browsertest.cc:46: "});"));
nit: indent properly, i.e. two more spaces on "function() {" and two more spaces
on "window.domAutomationController" lines.

https://codereview.chromium.org/1054403002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/webview_login_browsertest.cc:51:
content::DOMMessageQueue message_queue;
nit: We might want to move this before ShowSigninUI() call just in case the
event is fired before the function returns.

https://codereview.chromium.org/1054403002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/webview_login_browsertest.cc:75:
content::WindowedNotificationObserver session_start_waiter(
nit: Move this before ExecuteJsInSigninFrame at line 73, just in case the
session started notifiction is fired before the call returns.

[Dmitry Polukhin, 2015-04-07 16:34:14]

https://codereview.chromium.org/1054403002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/test/oobe_base_test.cc (left):

https://codereview.chromium.org/1054403002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/test/oobe_base_test.cc:98: GURL gaia_url =
server_url.ReplaceComponents(replace_gaia_host);
On 2015/04/07 16:04:59, xiyuan wrote:
> Please keep this part. The intention of the code is to use a "gaia" as the
host
> name for FakeGaia. It is needed because KioskTest uses EmbeddedTestServer as
> both Gaia and CWS. The default "localhost" would cause problems in
> chrome_resource_dispatcher_host_delegate.cc [1]
> 
> 
> [1]:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ren...

Unfortunately I had to remove this part because it cause certificate for GAIA
and it cannot be loaded in webview. I removed it because there is no test that
us it - all tests passes.

[xiyuan, 2015-04-07 17:02:56]

https://codereview.chromium.org/1054403002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/test/oobe_base_test.cc (left):

https://codereview.chromium.org/1054403002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/test/oobe_base_test.cc:98: GURL gaia_url =
server_url.ReplaceComponents(replace_gaia_host);
On 2015/04/07 16:34:13, Dmitry Polukhin wrote:
> On 2015/04/07 16:04:59, xiyuan wrote:
> > Please keep this part. The intention of the code is to use a "gaia" as the
> host
> > name for FakeGaia. It is needed because KioskTest uses EmbeddedTestServer as
> > both Gaia and CWS. The default "localhost" would cause problems in
> > chrome_resource_dispatcher_host_delegate.cc [1]
> > 
> > 
> > [1]:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ren...
> 
> Unfortunately I had to remove this part because it cause certificate for GAIA
> and it cannot be loaded in webview. I removed it because there is no test that
> us it - all tests passes.

Okay. It is probably because the test cert is for 127.0.0.1 and tests are
passing beccause we also use "cws" for CWS domain. Feels like we have one less
insurance but I guess I have to live with that. :p

https://codereview.chromium.org/1054403002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/test/oobe_base_test.cc (right):

https://codereview.chromium.org/1054403002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/test/oobe_base_test.cc:152: // webstore in
chrome_resource_dispatcher_host_delegate.cc.
nit: This comment can go then.

[Dmitry Polukhin, 2015-04-08 01:14:02]

rogerta@ could you please take a look, I need OWNER review for google_apis/*

https://codereview.chromium.org/1054403002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/test/oobe_base_test.cc (left):

https://codereview.chromium.org/1054403002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/test/oobe_base_test.cc:98: GURL gaia_url =
server_url.ReplaceComponents(replace_gaia_host);
On 2015/04/07 17:02:55, xiyuan wrote:
> On 2015/04/07 16:34:13, Dmitry Polukhin wrote:
> > On 2015/04/07 16:04:59, xiyuan wrote:
> > > Please keep this part. The intention of the code is to use a "gaia" as the
> > host
> > > name for FakeGaia. It is needed because KioskTest uses EmbeddedTestServer
as
> > > both Gaia and CWS. The default "localhost" would cause problems in
> > > chrome_resource_dispatcher_host_delegate.cc [1]
> > > 
> > > 
> > > [1]:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ren...
> > 
> > Unfortunately I had to remove this part because it cause certificate for
GAIA
> > and it cannot be loaded in webview. I removed it because there is no test
that
> > us it - all tests passes.
> 
> Okay. It is probably because the test cert is for 127.0.0.1 and tests are
> passing beccause we also use "cws" for CWS domain. Feels like we have one less
> insurance but I guess I have to live with that. :p

I think the same. HTTPS server is simple Python script that uses certificate
with IP address. In Chrome we change DNS resolver map all names to 127.0.0.1 but
there is no way to do the same in HTTPS forwarder script.

https://codereview.chromium.org/1054403002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/test/oobe_base_test.cc (right):

https://codereview.chromium.org/1054403002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/test/oobe_base_test.cc:152: // webstore in
chrome_resource_dispatcher_host_delegate.cc.
On 2015/04/07 17:02:55, xiyuan wrote:
> nit: This comment can go then.

Done.

https://codereview.chromium.org/1054403002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/webview_login_browsertest.cc (right):

https://codereview.chromium.org/1054403002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/webview_login_browsertest.cc:46: "});"));
On 2015/04/07 16:04:59, xiyuan wrote:
> nit: indent properly, i.e. two more spaces on "function() {" and two more
spaces
> on "window.domAutomationController" lines.

Done. It was 'git cl format' and I didn't notice it.

https://codereview.chromium.org/1054403002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/webview_login_browsertest.cc:51:
content::DOMMessageQueue message_queue;
On 2015/04/07 16:04:59, xiyuan wrote:
> nit: We might want to move this before ShowSigninUI() call just in case the
> event is fired before the function returns.

Done.

https://codereview.chromium.org/1054403002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/login/webview_login_browsertest.cc:75:
content::WindowedNotificationObserver session_start_waiter(
On 2015/04/07 16:04:59, xiyuan wrote:
> nit: Move this before ExecuteJsInSigninFrame at line 73, just in case the
> session started notifiction is fired before the call returns.

Done.

[Roger Tawa OOO till Jul 10th, 2015-04-08 01:35:44]

lgtm google_apis

[Dmitry Polukhin, 2015-04-08 01:36:38]

The CQ bit was checked by dpolukhin@chromium.org

[Dmitry Polukhin, 2015-04-08 01:36:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
nkostylev@chromium.org, xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/1054403002/#ps160001
(title: "comments resolved")

[commit-bot: I haz the power, 2015-04-08 01:37:03]

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

[commit-bot: I haz the power, 2015-04-08 04:35:50]

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

[commit-bot: I haz the power, 2015-04-08 04:35:51]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Dmitry Polukhin, 2015-04-08 10:42:18]

The CQ bit was checked by dpolukhin@chromium.org

[Dmitry Polukhin, 2015-04-08 10:42:18]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
nkostylev@chromium.org, rogerta@chromium.org
Link to the patchset: https://codereview.chromium.org/1054403002/#ps180001
(title: "remove flaky back-button check")

[commit-bot: I haz the power, 2015-04-08 10:42:53]

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

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

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-04-08 13:00:51]

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