Settings People Revamp: Add Manage Profile browsertests.

Settings People Revamp: Add Manage Profile browsertests.

BUG=563721
Committed: https://crrev.com/3fb6a884fbfeef4bf73ba977be8f10234b4aeeee
Cr-Commit-Position: refs/heads/master@{#386250}

[tommycli, 2016-04-08 00:47:57]

tommycli@chromium.org changed reviewers:
+ dpapad@chromium.org

[tommycli, 2016-04-08 00:47:58]

dpapad: PTAL, this is the browser test as promised from before for Manage
People. Your advice earlier this afternoon made implementing this easy.

[dpapad, 2016-04-08 01:15:12]

https://codereview.chromium.org/1873613002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/people_page_test.js (right):

https://codereview.chromium.org/1873613002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:7: var TestNames = {
I also used to gather the test names in an enum, see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w....

Eventually I did not find this very useful so I stopped doing it this in later
tests I authored (certificate manager tests and search engine tests). Up to you
if you want to keep it or just inline the names, just mentioning this in case
you also don't find it very useful, but copied the pattern from elsewhere.

https://codereview.chromium.org/1873613002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:42:
this.methodCalled('setProfileIconAndName');
No need to store the passed arguments manually. Instead you can do the
following,

this.methodCalled('setProfileIconAndName', [iconUrl, name]);
Then the array will be passed automatically to your whenCalled() Promise
callback, see comment at line 86 on how to retrieve those later.

https://codereview.chromium.org/1873613002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:53:
'chrome://md-settings/people_page/manage_profile.html');
Is this necessary? Isn't this already imported by the initial navigation to
chrome://md-settings/people_page/people_page.html?

https://codereview.chromium.org/1873613002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:86: function() {
return browserProxy.whenCalled('setProfileIconAndName').then(
    function(args) {
      assertEquals('fake-icon-2.png', args[0]);
      assertEquals('Initial Fake Name', args[1]);
    });

https://codereview.chromium.org/1873613002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:117:
nameField.fire('change');
Nit: Is there a way this could be done using MockInteractions instead? I am
doing something similar with a paper-input at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w...,
although that is firing an 'input' event instead of a 'change'.

[tommycli, 2016-04-08 17:33:30]

dpapad: thanks for the quick review!

https://codereview.chromium.org/1873613002/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/people_page_test.js (right):

https://codereview.chromium.org/1873613002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:7: var TestNames = {
On 2016/04/08 01:15:12, dpapad wrote:
> I also used to gather the test names in an enum, see
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w....
> 
> Eventually I did not find this very useful so I stopped doing it this in later
> tests I authored (certificate manager tests and search engine tests). Up to
you
> if you want to keep it or just inline the names, just mentioning this in case
> you also don't find it very useful, but copied the pattern from elsewhere.

Done. Removed, thahnks for the heads up.

https://codereview.chromium.org/1873613002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:42:
this.methodCalled('setProfileIconAndName');
On 2016/04/08 01:15:12, dpapad wrote:
> No need to store the passed arguments manually. Instead you can do the
> following,
> 
> this.methodCalled('setProfileIconAndName', [iconUrl, name]);
> Then the array will be passed automatically to your whenCalled() Promise
> callback, see comment at line 86 on how to retrieve those later.

Done.

https://codereview.chromium.org/1873613002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:53:
'chrome://md-settings/people_page/manage_profile.html');
On 2016/04/08 01:15:12, dpapad wrote:
> Is this necessary? Isn't this already imported by the initial navigation to
> chrome://md-settings/people_page/people_page.html?

Done.

https://codereview.chromium.org/1873613002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:86: function() {
On 2016/04/08 01:15:12, dpapad wrote:
> return browserProxy.whenCalled('setProfileIconAndName').then(
>     function(args) {
>       assertEquals('fake-icon-2.png', args[0]);
>       assertEquals('Initial Fake Name', args[1]);
>     });

Done.

https://codereview.chromium.org/1873613002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:117:
nameField.fire('change');
On 2016/04/08 01:15:12, dpapad wrote:
> Nit: Is there a way this could be done using MockInteractions instead? I am
> doing something similar with a paper-input at
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w...,
> although that is firing an 'input' event instead of a 'change'.

Unfortunately, no.

I tried MockInteractions.pressAndReleaseKeyOn. Unfortunately, they don't change
the value, nor do they fire a change event. I think this is because of:

http://stackoverflow.com/questions/13944835/how-to-simulate-typing-in-input-f...

[dpapad, 2016-04-08 17:51:37]

https://codereview.chromium.org/1873613002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/people_page_test.js (right):

https://codereview.chromium.org/1873613002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:25: getAvailableIcons:
function(sendSettings) {
|sendSettings| parameter seems like a copy-paste error from somewhere else?

https://codereview.chromium.org/1873613002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:33: this.lastSetName = name;
These are still being used 108,109. Can you remove and do the same trick you did
at line 74,75?

[tommycli, 2016-04-08 17:58:04]

dpapad: thanks, I had some crazy brain fart and didn't fix it completely on the
first go.

https://codereview.chromium.org/1873613002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/people_page_test.js (right):

https://codereview.chromium.org/1873613002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:25: getAvailableIcons:
function(sendSettings) {
On 2016/04/08 17:51:37, dpapad wrote:
> |sendSettings| parameter seems like a copy-paste error from somewhere else?

Done.

https://codereview.chromium.org/1873613002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/settings/people_page_test.js:33: this.lastSetName = name;
On 2016/04/08 17:51:37, dpapad wrote:
> These are still being used 108,109. Can you remove and do the same trick you
did
> at line 74,75?

Done.

[dpapad, 2016-04-08 18:00:24]

lgtm

[tommycli, 2016-04-08 18:46:25]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-04-08 18:47:32]

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

[commit-bot: I haz the power, 2016-04-08 19:59:39]

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

[commit-bot: I haz the power, 2016-04-08 19:59:40]

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_...)

[tommycli, 2016-04-08 22:37:08]

I had to modify this patch to restrict it to non-ChromeOS only. The
people_page_manage_profile_test.js file has no change other than renaming
settings_people_page to settings_people_page_manage_profile.

[tommycli, 2016-04-08 22:37:14]

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

[commit-bot: I haz the power, 2016-04-08 22:37:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1873613002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1873613002/120001

[dpapad, 2016-04-08 22:40:43]

On 2016/04/08 at 22:37:08, tommycli wrote:
> I had to modify this patch to restrict it to non-ChromeOS only. The
people_page_manage_profile_test.js file has no change other than renaming
settings_people_page to settings_people_page_manage_profile.

Still LGTM

[tommycli, 2016-04-08 22:41:36]

The CQ bit was unchecked by tommycli@chromium.org

[tommycli, 2016-04-08 22:41:37]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-04-08 22:41:55]

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

[commit-bot: I haz the power, 2016-04-08 23:56:07]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-08 23:58:24]

Description was changed from

==========
Settings People Revamp: Add Manage Profile browsertests.

BUG=563721
==========

to

==========
Settings People Revamp: Add Manage Profile browsertests.

BUG=563721

Committed: https://crrev.com/3fb6a884fbfeef4bf73ba977be8f10234b4aeeee
Cr-Commit-Position: refs/heads/master@{#386250}
==========

[commit-bot: I haz the power, 2016-04-08 23:58:25]

Patchset 7 (id:??) landed as
https://crrev.com/3fb6a884fbfeef4bf73ba977be8f10234b4aeeee
Cr-Commit-Position: refs/heads/master@{#386250}