Flakyness: fix notifications/ tests.

Flakyness: fix notifications/ tests.

This is using the new PermissionsHelper.setPermission method that
prevents race conditions when trying to use testRunner.setPermission.

BUG=518981, 518980, 518979, 518976, 518978
Committed: https://crrev.com/21fc7fcbf05a64c7183747a04bbef93c663cee9e
git-svn-id: svn://svn.chromium.org/blink/trunk@202169 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[mlamouri (slow - plz ping), 2015-08-25 20:36:36]

mlamouri@chromium.org changed reviewers:
+ johnme@chromium.org

[johnme, 2015-08-26 12:27:32]

testRunner.setPermission is called incorrectly (with the same race condition)
from 77 test files.

If it's too complicated to fix setPermission to wait until the permission is
actually set (like most other testRunner methods which act synchronously), then
please rename it to setPermissionAsync, and while you're doing that add TODOs to
the other tests mentioning the race condition.

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/notifications/click-document.html (right):

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/notifications/click-document.html:16: var script =
document.createElement('script');
Nit: other http/tests/notifications/ use 4-space indent for JS.

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/notifications/click-window-focus-document.html
(right):

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/notifications/click-window-focus-document.html:20:
PermissionsHelper.setPermission('notifications',
'granted').then(test.step_func(function() {
This test supports being run manually (notice how it checks for
window.testRunner). But here you unconditionally call
PermissionsHelper.setPermission, which unconditionally calls
testRunner.setPermission if permission hasn't yet been set, rather than reaching
the nice assert below.

Ditto for the other tests that say they can be run manually.

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/notifications/click-window-focus-document.html:43:
notification.addEventListener('error', function() {
Why is this one not a test.step_func?

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/notifications/update-document.html (right):

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/notifications/update-document.html:19:
document.body.appendChild(script);
I wonder if this is a common enough use case to add a
PermissionsHelper.setPermissionThenRunScript('notifications', 'granted',
'resources/update-event-test.js') style API. I guess we can wait and see...

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/resources/permissions-helper.js (left):

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/resources/permissions-helper.js:20: case
"push-messaging":
It's a bit weird for push-messaging to pass userVisibleOnly:true. Perhaps rename
this to push-notifications?

[johnme, 2015-08-26 12:28:03]

Thanks for looking into this btw, much appreciated!

[johnme, 2015-08-27 13:38:10]

I just hit the same problem in
http/tests/notifications/click-dedicated-worker.html when running tests
locally...

[mlamouri (slow - plz ping), 2015-08-31 16:31:42]

Comments applied. PTAL.

Regarding the Async prefix, I will introduce a method using a callback (it seems
that it is actually doable). Then I will update the Blink callers. Though, this
shouldn't impact this CL.

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/notifications/click-document.html (right):

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/notifications/click-document.html:16: var script =
document.createElement('script');
On 2015/08/26 at 12:27:32, johnme wrote:
> Nit: other http/tests/notifications/ use 4-space indent for JS.

Done.

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/notifications/click-window-focus-document.html
(right):

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/notifications/click-window-focus-document.html:20:
PermissionsHelper.setPermission('notifications',
'granted').then(test.step_func(function() {
On 2015/08/26 at 12:27:32, johnme wrote:
> This test supports being run manually (notice how it checks for
window.testRunner). But here you unconditionally call
PermissionsHelper.setPermission, which unconditionally calls
testRunner.setPermission if permission hasn't yet been set, rather than reaching
the nice assert below.
> 
> Ditto for the other tests that say they can be run manually.

In order to run manually this test requires the permission to be granted before
the test runs. If you do so, testRunner from PermissionHelpers.setPermission()
will not be called. AFAICT, the behaviour is unchanged.

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/notifications/click-window-focus-document.html:43:
notification.addEventListener('error', function() {
On 2015/08/26 at 12:27:32, johnme wrote:
> Why is this one not a test.step_func?

Because it doesn't need to be called.

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/notifications/close-document.html (right):

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/notifications/close-document.html:16: var script =
document.createElement('script');
I also updated this indentation.

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/notifications/update-document.html (right):

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/notifications/update-document.html:17: var script =
document.createElement('script');
.. and this one.

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/notifications/update-document.html:19:
document.body.appendChild(script);
On 2015/08/26 at 12:27:32, johnme wrote:
> I wonder if this is a common enough use case to add a
PermissionsHelper.setPermissionThenRunScript('notifications', 'granted',
'resources/update-event-test.js') style API. I guess we can wait and see...

Yeah, no need to do that unless it becomes a trend.

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/resources/permissions-helper.js (left):

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/resources/permissions-helper.js:20: case
"push-messaging":
On 2015/08/26 at 12:27:32, johnme wrote:
> It's a bit weird for push-messaging to pass userVisibleOnly:true. Perhaps
rename this to push-notifications?

I don't mind either way. Feel free to file a bug.

[johnme, 2015-09-02 10:28:24]

lgtm with async_test question. looking forward to the method using a callback :)

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
File LayoutTests/http/tests/notifications/click-window-focus-document.html
(right):

https://codereview.chromium.org/1312623003/diff/20001/LayoutTests/http/tests/...
LayoutTests/http/tests/notifications/click-window-focus-document.html:20:
PermissionsHelper.setPermission('notifications',
'granted').then(test.step_func(function() {
On 2015/08/31 16:31:42, Mounir Lamouri wrote:
> On 2015/08/26 at 12:27:32, johnme wrote:
> > This test supports being run manually (notice how it checks for
> window.testRunner). But here you unconditionally call
> PermissionsHelper.setPermission, which unconditionally calls
> testRunner.setPermission if permission hasn't yet been set, rather than
reaching
> the nice assert below.
> > 
> > Ditto for the other tests that say they can be run manually.
> 
> In order to run manually this test requires the permission to be granted
before
> the test runs. If you do so, testRunner from PermissionHelpers.setPermission()
> will not be called. AFAICT, the behaviour is unchanged.

Sort of; if you forget to grant notification permission in advance, you used to
get a clear "No permission has been granted for displaying notifications."
assert, whereas now you get "Uncaught ReferenceError: testRunner is not
defined".

Perhaps PermissionsHelper could reject the promise in such cases with "Could not
set permission ${permissionName} because testRunner is not available. If running
this test manually, perhaps you need to set permissions beforehand?"

https://codereview.chromium.org/1312623003/diff/40001/LayoutTests/http/tests/...
File LayoutTests/http/tests/notifications/click-document.html (right):

https://codereview.chromium.org/1312623003/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/notifications/click-document.html:15:
PermissionsHelper.setPermission('notifications', 'granted').then(function() {
Should this be wrapped in async_test (since otherwise execution reaches the end
of the page without any test stuff getting called)?

That might require some refactoring, since click-event-test.js has async_test in
it.

[mlamouri (slow - plz ping), 2015-09-13 15:27:01]

https://codereview.chromium.org/1312623003/diff/40001/LayoutTests/http/tests/...
File LayoutTests/http/tests/notifications/click-document.html (right):

https://codereview.chromium.org/1312623003/diff/40001/LayoutTests/http/tests/...
LayoutTests/http/tests/notifications/click-document.html:15:
PermissionsHelper.setPermission('notifications', 'granted').then(function() {
On 2015/09/02 at 10:28:24, johnme wrote:
> Should this be wrapped in async_test (since otherwise execution reaches the
end of the page without any test stuff getting called)?
> 
> That might require some refactoring, since click-event-test.js has async_test
in it.

testharness seems to wait for a test to be ran before finishing the test. I
checked and the async_test from inside the script is ran even if the load event
is fired before the script is inserted.

[mlamouri (slow - plz ping), 2015-09-13 15:27:04]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2015-09-13 15:27:04]

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

[commit-bot: I haz the power, 2015-09-13 15:27:10]

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

[commit-bot: I haz the power, 2015-09-13 17:46:41]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202169

[commit-bot: I haz the power, 2015-09-23 12:28:32]

Patchset 4 (id:??) landed as
https://crrev.com/21fc7fcbf05a64c7183747a04bbef93c663cee9e