MD Settings: Fix and re-enable settings-main tests.

MD Settings: Fix and re-enable settings-main tests.

 - Updated cases where the code had rotten (for example functions
   that used to return synchronously now return a Promise).
 - Updated test logic where Polymer.dom.flush() was not sufficient
   anymore, to use a MutationObserer (for detecting visibility changes).
 - Re-enable tests an all platforms, for non-debug builds. Debug builds
   remain disabled.

The tests still use the real chrome.settingsPrivate API, which makes it
likely that they could flake (timeout). Will address that if it happens.


BUG=667882
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2863213002
Cr-Commit-Position: refs/heads/master@{#470123}
Committed: https://chromium.googlesource.com/chromium/src/+/1510abbfe7ba6d200cfcc0f461c5a38c761fa353

[dpapad, 2017-05-05 20:50:04]

Description was changed from

==========
MD Settings: Fix and re-enable settings-main tests.

BUG=667882
==========

to

==========
MD Settings: Fix and re-enable settings-main tests.

BUG=667882
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-05-05 23:31:49]

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

[commit-bot: I haz the power, 2017-05-05 23:33:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-05-06 01:33:35]

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

[dpapad, 2017-05-06 01:33:36]

Green tests at patch 2.

[dpapad, 2017-05-06 01:37:39]

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

[commit-bot: I haz the power, 2017-05-06 01:38:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-06 02:44:51]

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

[commit-bot: I haz the power, 2017-05-06 02:44:52]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2017-05-08 18:00:04]

Would you mind adding something to the description about how the tests are being
fixed?

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

https://codereview.chromium.org/2863213002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/settings_main_test.js:183: expectedAdvanced ||
'block',
why the || 'block'? (is it related to callers invoking this function with empty
strings?)

https://codereview.chromium.org/2863213002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/settings_main_test.js:260: advancedPage,
'hidden', true);
The 'hidden' attribute can never be a boolean; it will be |null| or a string.

Seems like test_util.whenAttributeIs is written incorrectly:

    return target[attributeName] === attributeValue;

That is testing a property, not an attribute. Can you add a TODO (here or there)
to fix it or rename it?

[dpapad, 2017-05-08 19:07:36]

Description was changed from

==========
MD Settings: Fix and re-enable settings-main tests.

BUG=667882
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Fix and re-enable settings-main tests.

 - Updated cases where the code had rotten (for example functions
   that used to return synchronously now return a Promise).
 - Updated test logic where Polymer.dom.flush() was not sufficient
   anymore, to use a MutationObserer (for detecting visibility changes).
 - Re-enable tests an all platforms, for non-debug builds. Debug builds
   remain disabled.

The tests still use the real chrome.settingsPrivate API, which makes it
likely that they could flake (timeout). Will address that if it happens.


BUG=667882
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-05-08 19:07:52]

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

https://codereview.chromium.org/2863213002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/settings_main_test.js:183: expectedAdvanced ||
'block',
On 2017/05/08 at 18:00:04, michaelpg wrote:
> why the || 'block'? (is it related to callers invoking this function with
empty strings?)

Updated all callers to pass 'block' instead of '', and removed from here. Before
we were using |style.display| (and assuming that worked at the time the tests
were originally authored), whereas now the test is using
getComputedStyle(element).display, which is more robust.

https://codereview.chromium.org/2863213002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/settings_main_test.js:260: advancedPage,
'hidden', true);
On 2017/05/08 at 18:00:03, michaelpg wrote:
> The 'hidden' attribute can never be a boolean; it will be |null| or a string.
> 
> Seems like test_util.whenAttributeIs is written incorrectly:
> 
>     return target[attributeName] === attributeValue;
> 
> That is testing a property, not an attribute. Can you add a TODO (here or
there) to fix it or rename it?

Added TODO.

[dpapad, 2017-05-08 19:26:16]

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

[commit-bot: I haz the power, 2017-05-08 19:27:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[michaelpg, 2017-05-08 20:01:33]

lgtm

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

https://codereview.chromium.org/2863213002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/settings_main_test.js:183: expectedAdvanced ||
'block',
On 2017/05/08 19:07:52, dpapad wrote:
> On 2017/05/08 at 18:00:04, michaelpg wrote:
> > why the || 'block'? (is it related to callers invoking this function with
> empty strings?)
> 
> Updated all callers to pass 'block' instead of '', and removed from here.
Before
> we were using |style.display| (and assuming that worked at the time the tests
> were originally authored), whereas now the test is using
> getComputedStyle(element).display, which is more robust.

Oh, I see. Thanks for updating!

[dpapad, 2017-05-08 20:41:37]

The CQ bit was unchecked by dpapad@chromium.org

[dpapad, 2017-05-08 20:41:39]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-05-08 20:42:09]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 21:08:02]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494276099820210,
"parent_rev": "ba8991ee197a4aea56536c6096fc8d35adfe454f", "commit_rev":
"1510abbfe7ba6d200cfcc0f461c5a38c761fa353"}

[commit-bot: I haz the power, 2017-05-08 21:08:14]

Description was changed from

==========
MD Settings: Fix and re-enable settings-main tests.

 - Updated cases where the code had rotten (for example functions
   that used to return synchronously now return a Promise).
 - Updated test logic where Polymer.dom.flush() was not sufficient
   anymore, to use a MutationObserer (for detecting visibility changes).
 - Re-enable tests an all platforms, for non-debug builds. Debug builds
   remain disabled.

The tests still use the real chrome.settingsPrivate API, which makes it
likely that they could flake (timeout). Will address that if it happens.


BUG=667882
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Fix and re-enable settings-main tests.

 - Updated cases where the code had rotten (for example functions
   that used to return synchronously now return a Promise).
 - Updated test logic where Polymer.dom.flush() was not sufficient
   anymore, to use a MutationObserer (for detecting visibility changes).
 - Re-enable tests an all platforms, for non-debug builds. Debug builds
   remain disabled.

The tests still use the real chrome.settingsPrivate API, which makes it
likely that they could flake (timeout). Will address that if it happens.


BUG=667882
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2863213002
Cr-Commit-Position: refs/heads/master@{#470123}
Committed:
https://chromium.googlesource.com/chromium/src/+/1510abbfe7ba6d200cfcc0f461c5...
==========

[commit-bot: I haz the power, 2017-05-08 21:08:17]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/1510abbfe7ba6d200cfcc0f461c5...