Settings People Revamp: Add tests for ChromeOS Change Picture.

Settings People Revamp: Add tests for ChromeOS Change Picture.

Adds a browsertest for ChromeOS Change Picture Settings subpage.

Modelled after the bluetooth_page_browsertest_chromeos.

BUG=563721
Committed: https://crrev.com/1be9999be9c68a87ba4e092f0ecac835cd0876fd
Cr-Commit-Position: refs/heads/master@{#370748}

[tommycli, 2016-01-19 19:12:15]

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

[tommycli, 2016-01-19 19:12:16]

dpapad: PTAL added some tests for the Change Picture

[dpapad, 2016-01-19 19:44:59]

https://codereview.chromium.org/1585963004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1585963004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/change_picture.js:5: var
changePictureOptions = changePictureOptions || {
Is there a way we can avoid this global variable? Can we turn it into a member
variable of the custom element instead?

https://codereview.chromium.org/1585963004/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/change_picture_browsertest_chromeos.js
(right):

https://codereview.chromium.org/1585963004/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/change_picture_browsertest_chromeos.js:56: var
handler = function() {
function whenSelectedImageUrlChanged() {
  return new Promise(function(resolve, reject) {
    var handler = function() {
      // I think you can refer to "handler" from itself just fine.
      changePicture.removeEventListener('selected-image-url_-changed', handler);
      resolve();
    };
    changePicture.addEventListener('selected-image-url_-changed', handler);
  };
}

https://codereview.chromium.org/1585963004/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/change_picture_browsertest_chromeos.js:76:
onceSelectedImageUrlChanged(function() {
No need to pass your assertions as a callback. See suggestion at line 55. Then
you can do the following here.


return whenSelectedImageUrlChanged().then(function() {
  expectEquals(changePicture.selectedImageUrl_,
               changePicture.profileImageUrl_);
  Polymer.dom.flush();
  expectTrue(profileImage.active);
});

[tommycli, 2016-01-20 00:30:27]

dpapad: Hi, thanks for the feedback, :D

Tommy

https://codereview.chromium.org/1585963004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1585963004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/change_picture.js:5: var
changePictureOptions = changePictureOptions || {
On 2016/01/19 19:44:59, dpapad wrote:
> Is there a way we can avoid this global variable? Can we turn it into a member
> variable of the custom element instead?

I could not figure out an alternative. I followed the example here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

Is there a way to declare static variables on Polymer elements? This relies on
the fact that the test has a chance to declare the global BEFORE polymer does.

https://codereview.chromium.org/1585963004/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/change_picture_browsertest_chromeos.js
(right):

https://codereview.chromium.org/1585963004/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/change_picture_browsertest_chromeos.js:56: var
handler = function() {
On 2016/01/19 19:44:59, dpapad wrote:
> function whenSelectedImageUrlChanged() {
>   return new Promise(function(resolve, reject) {
>     var handler = function() {
>       // I think you can refer to "handler" from itself just fine.
>       changePicture.removeEventListener('selected-image-url_-changed',
handler);
>       resolve();
>     };
>     changePicture.addEventListener('selected-image-url_-changed', handler);
>   };
> }

Done.

https://codereview.chromium.org/1585963004/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/change_picture_browsertest_chromeos.js:76:
onceSelectedImageUrlChanged(function() {
On 2016/01/19 19:44:59, dpapad wrote:
> No need to pass your assertions as a callback. See suggestion at line 55. Then
> you can do the following here.
> 
> 
> return whenSelectedImageUrlChanged().then(function() {
>   expectEquals(changePicture.selectedImageUrl_,
>                changePicture.profileImageUrl_);
>   Polymer.dom.flush();
>   expectTrue(profileImage.active);
> });

Done. That is much better.

[dpapad, 2016-01-20 01:09:30]

lgtm

https://codereview.chromium.org/1585963004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1585963004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/change_picture.js:5: var
changePictureOptions = changePictureOptions || {
On 2016/01/20 at 00:30:27, tommycli wrote:
> On 2016/01/19 19:44:59, dpapad wrote:
> > Is there a way we can avoid this global variable? Can we turn it into a
member
> > variable of the custom element instead?
> 
> I could not figure out an alternative. I followed the example here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> 
> Is there a way to declare static variables on Polymer elements? This relies on
the fact that the test has a chance to declare the global BEFORE polymer does.

Two ways I can think of.
1) Don't test that the URL actually changes. Instead just test that
receiveSelectedImage() is being called with the new URL.
2) Keep testing selectedImageUrl_ via the change notification but define a
namespace for the global config var,
   cr.define('settings_test', function() {
     var changePictureOptions = ......;
     return {changePictureOptions: changePictureOptions};
   });

[tommycli, 2016-01-20 22:38:12]

https://codereview.chromium.org/1585963004/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1585963004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/change_picture.js:5: var
changePictureOptions = changePictureOptions || {
On 2016/01/20 01:09:29, dpapad wrote:
> On 2016/01/20 at 00:30:27, tommycli wrote:
> > On 2016/01/19 19:44:59, dpapad wrote:
> > > Is there a way we can avoid this global variable? Can we turn it into a
> member
> > > variable of the custom element instead?
> > 
> > I could not figure out an alternative. I followed the example here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> > 
> > Is there a way to declare static variables on Polymer elements? This relies
on
> the fact that the test has a chance to declare the global BEFORE polymer does.
> 
> Two ways I can think of.
> 1) Don't test that the URL actually changes. Instead just test that
> receiveSelectedImage() is being called with the new URL.
> 2) Keep testing selectedImageUrl_ via the change notification but define a
> namespace for the global config var,
>    cr.define('settings_test', function() {
>      var changePictureOptions = ......;
>      return {changePictureOptions: changePictureOptions};
>    });

Done.

Okay cool! i was able to namespace it to settings_test! In the future I might
choose to take Option a) instead.

Thanks!

[tommycli, 2016-01-20 22:38:31]

tommycli@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[tommycli, 2016-01-20 22:38:31]

phajdan.jr: May I have a review for BUILD.gn? Thanks!

[Paweł Hajdan Jr., 2016-01-21 15:46:16]

chrome/test LGTM

[tommycli, 2016-01-21 18:21:48]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-01-21 18:21:49]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/1585963004/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-01-21 18:22:38]

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

[tommycli, 2016-01-21 18:47:24]

phajdan.jr: thank you sir!

[commit-bot: I haz the power, 2016-01-21 19:36:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-21 19:37:38]

Description was changed from

==========
Settings People Revamp: Add tests for ChromeOS Change Picture.

Adds a browsertest for ChromeOS Change Picture Settings subpage.

Modelled after the bluetooth_page_browsertest_chromeos.

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

to

==========
Settings People Revamp: Add tests for ChromeOS Change Picture.

Adds a browsertest for ChromeOS Change Picture Settings subpage.

Modelled after the bluetooth_page_browsertest_chromeos.

BUG=563721

Committed: https://crrev.com/1be9999be9c68a87ba4e092f0ecac835cd0876fd
Cr-Commit-Position: refs/heads/master@{#370748}
==========

[commit-bot: I haz the power, 2016-01-21 19:37:38]

Patchset 4 (id:??) landed as
https://crrev.com/1be9999be9c68a87ba4e092f0ecac835cd0876fd
Cr-Commit-Position: refs/heads/master@{#370748}

[Dan Beam, 2016-03-16 17:22:28]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-03-16 17:22:29]

https://codereview.chromium.org/1585963004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/change_picture.js (right):

https://codereview.chromium.org/1585963004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/change_picture.js:14: });
why can't this just be

cr.exportPath('settings_test');

OR

cr.exportPath('settings_test').changePictureNotifyForTest = false;

the difference between cr.define() and cr.exportPath() is that cr.define() is
more useful when an inner scope is required for bookkeeping.  in a case as
simple as this, cr.exportPath() may be preferred.

[Finnur, 2016-03-17 11:09:47]

Description was changed from

==========
Settings People Revamp: Add tests for ChromeOS Change Picture.

Adds a browsertest for ChromeOS Change Picture Settings subpage.

Modelled after the bluetooth_page_browsertest_chromeos.

BUG=563721

Committed: https://crrev.com/1be9999be9c68a87ba4e092f0ecac835cd0876fd
Cr-Commit-Position: refs/heads/master@{#370748}
==========

to

==========
Settings People Revamp: Add tests for ChromeOS Change Picture.

Adds a browsertest for ChromeOS Change Picture Settings subpage.

Modelled after the bluetooth_page_browsertest_chromeos.

BUG=563721

Committed: https://crrev.com/1be9999be9c68a87ba4e092f0ecac835cd0876fd
Cr-Commit-Position: refs/heads/master@{#370748}
==========