Seed test for SyncUI: sign in successfully with mocks.

Seed test for SyncUI: sign in successfully with mocks.

R=binji@chromium.org
BUG=102750
TEST=browser_tests --gtest_filter=SyncSetupWebUITest.*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110006

[Sheridan Rawlins, 2011-11-08 06:58:14]

http://codereview.chromium.org/8503003/diff/1/chrome/browser/ui/webui/sync_se...
File chrome/browser/ui/webui/sync_setup_browsertest.js (right):

http://codereview.chromium.org/8503003/diff/1/chrome/browser/ui/webui/sync_se...
chrome/browser/ui/webui/sync_setup_browsertest.js:5: // TODO(scr) remove this
debugging tool.
I plan on removing this before commit, but it is useful in case we need to
adjust this test.

[James Hawkins, 2011-11-08 17:08:37]

http://codereview.chromium.org/8503003/diff/1/chrome/browser/ui/webui/sync_se...
File chrome/browser/ui/webui/sync_setup_browsertest.js (right):

http://codereview.chromium.org/8503003/diff/1/chrome/browser/ui/webui/sync_se...
chrome/browser/ui/webui/sync_setup_browsertest.js:27: 
Inconsistency: Blank line at beginning of method but not at the end; I prefer no
blank lines.

http://codereview.chromium.org/8503003/diff/1/chrome/browser/ui/webui/sync_se...
chrome/browser/ui/webui/sync_setup_browsertest.js:41: var mockHandler =
this.mockHandler = mock(MockSyncSetupHandler);
What's with this construct?

var mockHandler = this.mockHandler =...

?

http://codereview.chromium.org/8503003/diff/1/chrome/browser/ui/webui/sync_se...
chrome/browser/ui/webui/sync_setup_browsertest.js:62:
assertTrue(customizeSyncButton.hidden);
Testing wether elemenets are hidden or not (i.e., testing the presentation)
seems extremely flaky. Perhaps there's not a better way to test though.

http://codereview.chromium.org/8503003/diff/1/chrome/browser/ui/webui/sync_se...
chrome/browser/ui/webui/sync_setup_browsertest.js:69: var startStopSyncButton =
$('start-stop-sync');
I'm also hesitant to have tests reference element IDs directly. Abstracting this
into the SyncSetupOverlay class would be better, e.g.,

SyncSetupOverlay.getSigninButton()

etc.

[Sheridan Rawlins, 2011-11-08 21:32:03]

http://codereview.chromium.org/8503003/diff/1/chrome/browser/ui/webui/sync_se...
File chrome/browser/ui/webui/sync_setup_browsertest.js (right):

http://codereview.chromium.org/8503003/diff/1/chrome/browser/ui/webui/sync_se...
chrome/browser/ui/webui/sync_setup_browsertest.js:27: 
On 2011/11/08 17:08:37, James Hawkins wrote:
> Inconsistency: Blank line at beginning of method but not at the end; I prefer
no
> blank lines.

Done.

http://codereview.chromium.org/8503003/diff/1/chrome/browser/ui/webui/sync_se...
chrome/browser/ui/webui/sync_setup_browsertest.js:41: var mockHandler =
this.mockHandler = mock(MockSyncSetupHandler);
It's useful in other tests which set up several stubs() calls against the
mockHandler in the preLoad.  For this one, it's not needed.

Done.

On 2011/11/08 17:08:37, James Hawkins wrote:
> What's with this construct?
> 
> var mockHandler = this.mockHandler =...
> 
> ?

http://codereview.chromium.org/8503003/diff/1/chrome/browser/ui/webui/sync_se...
chrome/browser/ui/webui/sync_setup_browsertest.js:62:
assertTrue(customizeSyncButton.hidden);
How 'bout checking syncSetupCompleted is false?

Done.

On 2011/11/08 17:08:37, James Hawkins wrote:
> Testing wether elemenets are hidden or not (i.e., testing the presentation)
> seems extremely flaky. Perhaps there's not a better way to test though.

http://codereview.chromium.org/8503003/diff/1/chrome/browser/ui/webui/sync_se...
chrome/browser/ui/webui/sync_setup_browsertest.js:69: var startStopSyncButton =
$('start-stop-sync');
On 2011/11/08 17:08:37, James Hawkins wrote:
> I'm also hesitant to have tests reference element IDs directly. Abstracting
this
> into the SyncSetupOverlay class would be better, e.g.,
> 
> SyncSetupOverlay.getSigninButton()
> 
> etc.

Done.

[Dan Beam, 2011-11-08 21:54:47]

Sorry for the drive by, it's more a feature request for the framework (but
depends on how common this is).

http://codereview.chromium.org/8503003/diff/1/chrome/browser/ui/webui/sync_se...
File chrome/browser/ui/webui/sync_setup_browsertest.js (right):

http://codereview.chromium.org/8503003/diff/1/chrome/browser/ui/webui/sync_se...
chrome/browser/ui/webui/sync_setup_browsertest.js:39: SyncSetupShowSetupUI:
function() {},
Do we do this alot?  Seems like we would.  If we do, seems like it'd be nice to
have a stub(), emptyStub(), or noop() to mix in key names, i.e.

  stub(MockSetupHandler.prototype, ['stopSyncing', 'SyncSetupDidClosePage']);

Also, it's not a big deal, but if you just make one blank function and re-use it
everywhere, i.e.

  function blank() {}

  MockSetupHandler.prototype = {
    stopSyncing: blank,
    // ...
  };

supposedly that'll save memory as each function(){} is a different instance of a
Function object (which has some memory, construction, and parse time associated
with it).  In theory v8 would be smart enough to combine Functions with the same
function body with the same scope chain, but dunno.

[Sheridan Rawlins, 2011-11-09 02:44:21]

Thanks, Dan.

Incorporated that change too, and altered places that can use it.

http://codereview.chromium.org/8503003/diff/1/chrome/browser/ui/webui/sync_se...
File chrome/browser/ui/webui/sync_setup_browsertest.js (right):

http://codereview.chromium.org/8503003/diff/1/chrome/browser/ui/webui/sync_se...
chrome/browser/ui/webui/sync_setup_browsertest.js:39: SyncSetupShowSetupUI:
function() {},
On 2011/11/08 21:54:48, Dan Beam wrote:
> Do we do this alot?  Seems like we would.  If we do, seems like it'd be nice
to
> have a stub(), emptyStub(), or noop() to mix in key names, i.e.
> 
>   stub(MockSetupHandler.prototype, ['stopSyncing', 'SyncSetupDidClosePage']);
> 
> Also, it's not a big deal, but if you just make one blank function and re-use
it
> everywhere, i.e.
> 
>   function blank() {}
> 
>   MockSetupHandler.prototype = {
>     stopSyncing: blank,
>     // ...
>   };
> 
> supposedly that'll save memory as each function(){} is a different instance of
a
> Function object (which has some memory, construction, and parse time
associated
> with it).  In theory v8 would be smart enough to combine Functions with the
same
> function body with the same scope chain, but dunno.

Thanks - cool suggestion to make it easier (& memory efficient) to stub these
up.

Done.

[James Hawkins, 2011-11-09 17:15:56]

http://codereview.chromium.org/8503003/diff/2004/chrome/browser/resources/syn...
File chrome/browser/resources/sync_setup_overlay.js (right):

http://codereview.chromium.org/8503003/diff/2004/chrome/browser/resources/syn...
chrome/browser/resources/sync_setup_overlay.js:564: getLoginEmail_: function() {
Document these methods.

http://codereview.chromium.org/8503003/diff/2004/chrome/browser/resources/syn...
chrome/browser/resources/sync_setup_overlay.js:565: return $('gaia-email');
Two space indentation, here and elsewhere.

http://codereview.chromium.org/8503003/diff/2004/chrome/browser/resources/syn...
chrome/browser/resources/sync_setup_overlay.js:834:
SyncSetupOverlay.getLoginEmail = function() {
Document that these should only be called by the tests.

http://codereview.chromium.org/8503003/diff/2004/chrome/browser/ui/webui/sync...
File chrome/browser/ui/webui/sync_setup_browsertest.js (right):

http://codereview.chromium.org/8503003/diff/2004/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_browsertest.js:5: // TODO(scr) remove this
debugging tool.
Time to remove this.

http://codereview.chromium.org/8503003/diff/2004/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_browsertest.js:49: * Verify starting point is
not synced.
Verifies

http://codereview.chromium.org/8503003/diff/2004/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_browsertest.js:56: * Click the "Sign in to
Chrome" button.
Clicks

http://codereview.chromium.org/8503003/diff/2004/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_browsertest.js:78: gaiaLogin: function() {
Document the method.

http://codereview.chromium.org/8503003/diff/2004/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_browsertest.js:102:
TEST_F('SyncSetupWebUITest', 'VerifySignIn', function() {
This test method looks weird...it just calls other methods. More of the
expectations should be in this method.

http://codereview.chromium.org/8503003/diff/2004/chrome/test/data/webui/async...
File chrome/test/data/webui/async_gen.js (right):

http://codereview.chromium.org/8503003/diff/2004/chrome/test/data/webui/async...
chrome/test/data/webui/async_gen.js:273: var mockGlobals = this.mockGlobals;
Why are you using both this.mockGlobals and local var mockGlobals?

http://codereview.chromium.org/8503003/diff/2004/chrome/test/data/webui/test_...
File chrome/test/data/webui/test_api.js (right):

http://codereview.chromium.org/8503003/diff/2004/chrome/test/data/webui/test_...
chrome/test/data/webui/test_api.js:129: makeAndRegisterMockGlobals:
function(functionNames) {
Is this referring to JS globals?  Why are we mocking those?

[Sheridan Rawlins, 2011-11-09 23:50:59]

http://codereview.chromium.org/8503003/diff/2004/chrome/browser/resources/syn...
File chrome/browser/resources/sync_setup_overlay.js (right):

http://codereview.chromium.org/8503003/diff/2004/chrome/browser/resources/syn...
chrome/browser/resources/sync_setup_overlay.js:564: getLoginEmail_: function() {
On 2011/11/09 17:15:56, James Hawkins wrote:
> Document these methods.

Done.

http://codereview.chromium.org/8503003/diff/2004/chrome/browser/resources/syn...
chrome/browser/resources/sync_setup_overlay.js:565: return $('gaia-email');
On 2011/11/09 17:15:56, James Hawkins wrote:
> Two space indentation, here and elsewhere.

Done.

http://codereview.chromium.org/8503003/diff/2004/chrome/browser/resources/syn...
chrome/browser/resources/sync_setup_overlay.js:834:
SyncSetupOverlay.getLoginEmail = function() {
On 2011/11/09 17:15:56, James Hawkins wrote:
> Document that these should only be called by the tests.

Done.

http://codereview.chromium.org/8503003/diff/2004/chrome/browser/ui/webui/sync...
File chrome/browser/ui/webui/sync_setup_browsertest.js (right):

http://codereview.chromium.org/8503003/diff/2004/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_browsertest.js:5: // TODO(scr) remove this
debugging tool.
On 2011/11/09 17:15:56, James Hawkins wrote:
> Time to remove this.

Done.

http://codereview.chromium.org/8503003/diff/2004/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_browsertest.js:49: * Verify starting point is
not synced.
On 2011/11/09 17:15:56, James Hawkins wrote:
> Verifies

Done.

http://codereview.chromium.org/8503003/diff/2004/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_browsertest.js:56: * Click the "Sign in to
Chrome" button.
On 2011/11/09 17:15:56, James Hawkins wrote:
> Clicks

Done.

http://codereview.chromium.org/8503003/diff/2004/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_browsertest.js:78: gaiaLogin: function() {
On 2011/11/09 17:15:56, James Hawkins wrote:
> Document the method.

Moved into test; moot.

http://codereview.chromium.org/8503003/diff/2004/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_browsertest.js:102:
TEST_F('SyncSetupWebUITest', 'VerifySignIn', function() {
On 2011/11/09 17:15:56, James Hawkins wrote:
> This test method looks weird...it just calls other methods. More of the
> expectations should be in this method.

I had the feeling that further tests might need to share.  I can see this for
the first two cases, but not the gaiaLogin case.  For instance, you may wish to
startSyncing then do oauth login in the future.  I moved the guts of gaia login
into this test; let me know if you want the entire test in the test, and we can
refactor back to helper methods if it makes sense.

[James Hawkins, 2011-11-10 00:24:43]

http://codereview.chromium.org/8503003/diff/2004/chrome/test/data/webui/async...
File chrome/test/data/webui/async_gen.js (right):

http://codereview.chromium.org/8503003/diff/2004/chrome/test/data/webui/async...
chrome/test/data/webui/async_gen.js:273: var mockGlobals = this.mockGlobals;
On 2011/11/09 17:15:56, James Hawkins wrote:
> Why are you using both this.mockGlobals and local var mockGlobals?

Is there a reason you keep ignoring this question?

http://codereview.chromium.org/8503003/diff/2004/chrome/test/data/webui/test_...
File chrome/test/data/webui/test_api.js (right):

http://codereview.chromium.org/8503003/diff/2004/chrome/test/data/webui/test_...
chrome/test/data/webui/test_api.js:129: makeAndRegisterMockGlobals:
function(functionNames) {
On 2011/11/09 17:15:56, James Hawkins wrote:
> Is this referring to JS globals?  Why are we mocking those?

Still not answered.

http://codereview.chromium.org/8503003/diff/9002/chrome/browser/resources/syn...
File chrome/browser/resources/sync_setup_overlay.js (right):

http://codereview.chromium.org/8503003/diff/9002/chrome/browser/resources/syn...
chrome/browser/resources/sync_setup_overlay.js:565: * Get the login email text
input DOM element.
@private, here and elsewhere.

http://codereview.chromium.org/8503003/diff/9002/chrome/browser/ui/webui/sync...
File chrome/browser/ui/webui/sync_setup_browsertest.js (right):

http://codereview.chromium.org/8503003/diff/9002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_browsertest.js:37: verifyUnsynced: function()
{
This helper method only makes one call. Just inline it in the test.

http://codereview.chromium.org/8503003/diff/9002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_browsertest.js:45: var startStopSyncButton =
$('start-stop-sync');
Tests should not be querying elements using ID.

http://codereview.chromium.org/8503003/diff/9002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_browsertest.js:49:
OptionsPage.navigateToPage('syncSetup');
Two space indentation.

http://codereview.chromium.org/8503003/diff/9002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_browsertest.js:70: // Find the DOM objects on
the page.
s/Find/Verify/

http://codereview.chromium.org/8503003/diff/9002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_browsertest.js:78: // Set up mock
expectations
period at end of sentence; however, this particular comment adds little value.
Either beef it up or remove it.

http://codereview.chromium.org/8503003/diff/9002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_browsertest.js:85: // set the email &
password, then sign in.
Capitalize sentence.

[Sheridan Rawlins, 2011-11-10 02:02:12]

No intention to ignore comments/questions.

http://codereview.chromium.org/8503003/diff/2004/chrome/test/data/webui/async...
File chrome/test/data/webui/async_gen.js (right):

http://codereview.chromium.org/8503003/diff/2004/chrome/test/data/webui/async...
chrome/test/data/webui/async_gen.js:273: var mockGlobals = this.mockGlobals;
On 2011/11/09 17:15:56, James Hawkins wrote:
> Why are you using both this.mockGlobals and local var mockGlobals?

no & done.

http://codereview.chromium.org/8503003/diff/2004/chrome/test/data/webui/test_...
File chrome/test/data/webui/test_api.js (right):

http://codereview.chromium.org/8503003/diff/2004/chrome/test/data/webui/test_...
chrome/test/data/webui/test_api.js:129: makeAndRegisterMockGlobals:
function(functionNames) {
On 2011/11/09 17:15:56, James Hawkins wrote:
> Is this referring to JS globals?  Why are we mocking those?

Yes.  This gives the facility to stub out an existing global function with a
mock so that expectations can be set and test will fail if the global function
is not called or called incorrectly.  It also remembers the global function so
that it can be called as well.

http://codereview.chromium.org/8503003/diff/9002/chrome/browser/resources/syn...
File chrome/browser/resources/sync_setup_overlay.js (right):

http://codereview.chromium.org/8503003/diff/9002/chrome/browser/resources/syn...
chrome/browser/resources/sync_setup_overlay.js:565: * Get the login email text
input DOM element.
On 2011/11/10 00:24:43, James Hawkins wrote:
> @private, here and elsewhere.

Done.

http://codereview.chromium.org/8503003/diff/9002/chrome/browser/ui/webui/sync...
File chrome/browser/ui/webui/sync_setup_browsertest.js (right):

http://codereview.chromium.org/8503003/diff/9002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_browsertest.js:37: verifyUnsynced: function()
{
On 2011/11/10 00:24:43, James Hawkins wrote:
> This helper method only makes one call. Just inline it in the test.

Done.

http://codereview.chromium.org/8503003/diff/9002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_browsertest.js:45: var startStopSyncButton =
$('start-stop-sync');
On 2011/11/10 00:24:43, James Hawkins wrote:
> Tests should not be querying elements using ID.

Done.

http://codereview.chromium.org/8503003/diff/9002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_browsertest.js:49:
OptionsPage.navigateToPage('syncSetup');
On 2011/11/10 00:24:43, James Hawkins wrote:
> Two space indentation.

Done.

http://codereview.chromium.org/8503003/diff/9002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_browsertest.js:70: // Find the DOM objects on
the page.
On 2011/11/10 00:24:43, James Hawkins wrote:
> s/Find/Verify/

Done.

http://codereview.chromium.org/8503003/diff/9002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_browsertest.js:78: // Set up mock
expectations
On 2011/11/10 00:24:43, James Hawkins wrote:
> period at end of sentence; however, this particular comment adds little value.
> Either beef it up or remove it.

Done.

http://codereview.chromium.org/8503003/diff/9002/chrome/browser/ui/webui/sync...
chrome/browser/ui/webui/sync_setup_browsertest.js:85: // set the email &
password, then sign in.
On 2011/11/10 00:24:43, James Hawkins wrote:
> Capitalize sentence.

Done.

[Sheridan Rawlins, 2011-11-12 00:48:26]

After gchat with James, removed ALL instances of var mockGlobals and mockHandler
(used this.mockGlobals & this.mockHandler).

[James Hawkins, 2011-11-14 20:23:12]

Much cleaner, LGTM.

[binji, 2011-11-14 22:26:08]

On 2011/11/14 20:23:12, James Hawkins wrote:
> Much cleaner, LGTM.

LGTM as well. Though I wonder if we will want to put some delays in the tests at
some point; the test in sync_setup_browsertest.js assumes that clicking the
sign-in button will immediately respond with a result.

It probably doesn't matter for this test, but for some tests it seems as though
it could make a difference.

[James Hawkins, 2011-11-14 23:24:52]

On 2011/11/14 22:26:08, binji wrote:
> On 2011/11/14 20:23:12, James Hawkins wrote:
> > Much cleaner, LGTM.
> 
> LGTM as well. Though I wonder if we will want to put some delays in the tests
at
> some point; the test in sync_setup_browsertest.js assumes that clicking the
> sign-in button will immediately respond with a result.
> 
> It probably doesn't matter for this test, but for some tests it seems as
though
> it could make a difference.

Using delays is flaky. If the operation is asynchronous, we need to use an
'observer' of some sort.

[Sheridan Rawlins, 2011-11-14 23:30:54]

Because the chrome.send is mocked in JS, the click _is_ synchronous.

On 2011/11/14 22:26:08, binji wrote:
> On 2011/11/14 20:23:12, James Hawkins wrote:
> > Much cleaner, LGTM.
> 
> LGTM as well. Though I wonder if we will want to put some delays in the tests
at
> some point; the test in sync_setup_browsertest.js assumes that clicking the
> sign-in button will immediately respond with a result.
> 
> It probably doesn't matter for this test, but for some tests it seems as
though
> it could make a difference.

[commit-bot: I haz the power, 2011-11-15 00:02:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scr@chromium.org/8503003/19001

[commit-bot: I haz the power, 2011-11-15 01:32:37]

Change committed as 110006