Move promise_test and assert_promise_rejects out of worker-test-harness.

Move promise_test and assert_promise_rejects out of worker-test-harness.

These methods have nothing to do with service worker based tests, and are just
as useful in normal testharness based tests, so move them to a separate file.

Additionally rename worker-test-harness.js to worker-testharness.js for consistency.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183557

[Marijn Kruisselbrink, 2014-10-09 18:37:35]

mek@chromium.org changed reviewers:
+ asanka@chromium.org

[Marijn Kruisselbrink, 2014-10-09 18:37:36]

As suggested in https://codereview.chromium.org/623813002, move the promise
testharness helpers to a separate file. These methods are just as useful outside
of workers, so having them in worker-test-harness.js doesn't really make sense.

I'm not sure if having worker-test-harness.js import the new
testharness-helpers.js (like I've done now) is best, or if it is better for
individual tests that use this to import the -helpers.js file?

(currently this CL is made on top of 623813002, but if I can land this CL first
I'll of course get rid of the geofencing changes in this CL and instead
integrate those into the other CL directly).

[jsbell, 2014-10-09 18:46:06]

jsbell@chromium.org changed reviewers:
+ dpranke@google.com, jsbell@chromium.org

[jsbell, 2014-10-09 18:46:06]

+dpranke who cares about such things

https://codereview.chromium.org/647493002/diff/1/LayoutTests/http/tests/resou...
File LayoutTests/http/tests/resources/testharness-helpers.js (right):

https://codereview.chromium.org/647493002/diff/1/LayoutTests/http/tests/resou...
LayoutTests/http/tests/resources/testharness-helpers.js:3: * allow them to be
used accross multiple tests before they have been
typo: accross -> across

[jsbell, 2014-10-10 17:18:08]

And (nit/suggestion aside) I'm fine with this, so lgtm but asanka and dpranke
should weigh in.

https://codereview.chromium.org/647493002/diff/100001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js
(right):

https://codereview.chromium.org/647493002/diff/100001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js:76:
importScripts('/resources/testharness-helpers.js');
After some pondering, I think we should probably remove this and have tests
import this directly. When the fixes here are upstreamed we'll want workers to
import testharness.js directly. But at that point - and likely, always - there
will be additional 'helpers' that have not yet been upstreamed. So we might was
well separate these out and have tests do both imports now.

(But I don't feel strongly)

[asanka, 2014-10-10 19:39:11]

lgtm w/ suggestion + nits

https://codereview.chromium.org/647493002/diff/100001/LayoutTests/http/tests/...
File LayoutTests/http/tests/resources/testharness-helpers.js (right):

https://codereview.chromium.org/647493002/diff/100001/LayoutTests/http/tests/...
LayoutTests/http/tests/resources/testharness-helpers.js:4: * upstreamed.
Could you mention here that testharness-helpers.js is intended to be usable from
both document and worker environments? Adding anything here that relies on the
DOM will likely break things on the worker tests. But it's good to have that
limitation be spelled out explicitly rather than be left to be discovered by
test failures.

https://codereview.chromium.org/647493002/diff/100001/LayoutTests/http/tests/...
LayoutTests/http/tests/resources/testharness-helpers.js:36: throw
'assert_promise_rejects: ' + description + ' Promise did not throw.';
Nit: 'Promise did not reject.'

https://codereview.chromium.org/647493002/diff/100001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js
(right):

https://codereview.chromium.org/647493002/diff/100001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js:76:
importScripts('/resources/testharness-helpers.js');
On 2014/10/10 17:18:07, jsbell wrote:
> After some pondering, I think we should probably remove this and have tests
> import this directly. When the fixes here are upstreamed we'll want workers to
> import testharness.js directly. But at that point - and likely, always - there
> will be additional 'helpers' that have not yet been upstreamed. So we might
was
> well separate these out and have tests do both imports now.
> 
> (But I don't feel strongly)

+1. I also don't feel strongly, but I'm in favor of having a common testharness
helper file. That way this file can be the "worker" friendly testharness.js
which can just be testharness.js at such time that the worker changes are
upstreamed.

If you are making that change, please also consider renaming this file to
worker-testharness.js for consistency.

[Marijn Kruisselbrink, 2014-10-10 20:32:56]

https://codereview.chromium.org/647493002/diff/100001/LayoutTests/http/tests/...
File LayoutTests/http/tests/resources/testharness-helpers.js (right):

https://codereview.chromium.org/647493002/diff/100001/LayoutTests/http/tests/...
LayoutTests/http/tests/resources/testharness-helpers.js:4: * upstreamed.
On 2014/10/10 19:39:11, asanka wrote:
> Could you mention here that testharness-helpers.js is intended to be usable
from
> both document and worker environments? Adding anything here that relies on the
> DOM will likely break things on the worker tests. But it's good to have that
> limitation be spelled out explicitly rather than be left to be discovered by
> test failures.

Done.

https://codereview.chromium.org/647493002/diff/100001/LayoutTests/http/tests/...
LayoutTests/http/tests/resources/testharness-helpers.js:36: throw
'assert_promise_rejects: ' + description + ' Promise did not throw.';
On 2014/10/10 19:39:11, asanka wrote:
> Nit: 'Promise did not reject.'

Done.

https://codereview.chromium.org/647493002/diff/100001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js
(right):

https://codereview.chromium.org/647493002/diff/100001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js:76:
importScripts('/resources/testharness-helpers.js');
On 2014/10/10 19:39:11, asanka wrote:
> On 2014/10/10 17:18:07, jsbell wrote:
> > After some pondering, I think we should probably remove this and have tests
> > import this directly. When the fixes here are upstreamed we'll want workers
to
> > import testharness.js directly. But at that point - and likely, always -
there
> > will be additional 'helpers' that have not yet been upstreamed. So we might
> was
> > well separate these out and have tests do both imports now.
> > 
> > (But I don't feel strongly)
> 
> +1. I also don't feel strongly, but I'm in favor of having a common
testharness
> helper file. That way this file can be the "worker" friendly testharness.js
> which can just be testharness.js at such time that the worker changes are
> upstreamed.
Done.

> If you are making that change, please also consider renaming this file to
> worker-testharness.js for consistency.
Done.

[Dirk Pranke, 2014-10-10 20:46:33]

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

[Dirk Pranke, 2014-10-10 20:46:34]

lgtm

[Marijn Kruisselbrink, 2014-10-10 20:47:07]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2014-10-10 20:48:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/647493002/320001

[commit-bot: I haz the power, 2014-10-10 21:45:34]

Committed patchset #4 (id:320001) as 183557