Fix the geofencing test to not break if the promises resolve asynchronously.

Fix the geofencing test to not break if the promises resolve asynchronously.

Also add a similar test to make sure the API exists/rejects in service workers.

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

[Marijn Kruisselbrink, 2014-09-05 21:37:56]

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

[Marijn Kruisselbrink, 2014-09-05 21:37:57]

This depends on https://codereview.chromium.org/464073002/ (at least the service
worker test part does).
I'm not actually sure if there is any point in adding the service worker test,
since it doesn't really test much, but fixing the existing test is required
before I can land https://codereview.chromium.org/476293002/

[Marijn Kruisselbrink, 2014-09-10 22:36:41]

Patchset #2 (id:20001) has been deleted

[Marijn Kruisselbrink, 2014-09-10 22:36:50]

ping?

[Peter Beverloo, 2014-09-11 16:32:21]

lgtm % comments

If more of your tests start following this paradigm then it may make sense to
generalize the execute-all-tests logic in a small library.

https://codereview.chromium.org/543203003/diff/1/LayoutTests/geofencing/geofe...
File LayoutTests/geofencing/geofencing-not-implemented.html (right):

https://codereview.chromium.org/543203003/diff/1/LayoutTests/geofencing/geofe...
LayoutTests/geofencing/geofencing-not-implemented.html:24: function nextTest() {
The nextTest() function (lines 24-43) sounds really generic, but does in fact
contain the expected results. Perhaps:

  (a) Use test harness for this test, explicitly indicating what is expected.
  (b) Rely on the test expectation file (geofencing-not-implemented.txt) to
indicate whether the promises should reject or resolve.

https://codereview.chromium.org/543203003/diff/1/LayoutTests/http/tests/geofe...
File LayoutTests/http/tests/geofencing/service-worker.html (right):

https://codereview.chromium.org/543203003/diff/1/LayoutTests/http/tests/geofe...
LayoutTests/http/tests/geofencing/service-worker.html:4: <script
src="../serviceworker/resources/test-helpers.js"></script>
This feels really fragile because we don't have explicit dependencies within
layout tests. We have plenty of this, so fine to go ahead, but it might be
useful to generalize some test harness + * Workers utility scripts.

https://codereview.chromium.org/543203003/diff/1/LayoutTests/http/tests/geofe...
LayoutTests/http/tests/geofencing/service-worker.html:6: var test =
async_test('Verify geofencing in a Service Worker');
Please clarify the description a bit (verify which bit of Geofencing?).

https://codereview.chromium.org/543203003/diff/1/LayoutTests/http/tests/geofe...
LayoutTests/http/tests/geofencing/service-worker.html:13: })
nit: indentation in this block seems to be off. Please always use four
characters. You don't need to break lines 9 and 10 (Blink does not have line
length limits).

[Marijn Kruisselbrink, 2014-09-11 19:11:06]

On 2014/09/11 16:32:21, Peter Beverloo wrote:
> If more of your tests start following this paradigm then it may make sense to
> generalize the execute-all-tests logic in a small library.
As it turns out this logic for worker based tests already exists in a small
library, and for non worker based tests no extra logic needed at all, so I got
the code a lot cleaner now.

https://codereview.chromium.org/543203003/diff/1/LayoutTests/geofencing/geofe...
File LayoutTests/geofencing/geofencing-not-implemented.html (right):

https://codereview.chromium.org/543203003/diff/1/LayoutTests/geofencing/geofe...
LayoutTests/geofencing/geofencing-not-implemented.html:24: function nextTest() {
On 2014/09/11 16:32:21, Peter Beverloo wrote:
> The nextTest() function (lines 24-43) sounds really generic, but does in fact
> contain the expected results. Perhaps:
> 
>   (a) Use test harness for this test, explicitly indicating what is expected.
>   (b) Rely on the test expectation file (geofencing-not-implemented.txt) to
> indicate whether the promises should reject or resolve.
I'm now using testharness (and in fact the exact same test code as in the
serviceworker test).

I suppose it might make sense to even split out the tests themselves into a
separate file so both this file and the serviceworker can just include that
file, but then I'm not sure if these tests will stick around anyway. At some
point geofencing will actually be implemented, and I'm not sure what can and
can't be tested in layouttests at that point (the actual implementation of
geofencing will be easiest to test in unittests for the browserprocess code, but
there should still be some kind of test on the blink side as well of course).

https://codereview.chromium.org/543203003/diff/1/LayoutTests/http/tests/geofe...
File LayoutTests/http/tests/geofencing/service-worker.html (right):

https://codereview.chromium.org/543203003/diff/1/LayoutTests/http/tests/geofe...
LayoutTests/http/tests/geofencing/service-worker.html:13: })
On 2014/09/11 16:32:21, Peter Beverloo wrote:
> nit: indentation in this block seems to be off. Please always use four
> characters. You don't need to break lines 9 and 10 (Blink does not have line
> length limits).
Since I rewrote this code entirely, this exact issue no longer exists, but I'm
not entirely sure if the indentation I'm using now is the correct one either.
There seem to be plenty of cases in promises using test code where parts of the
code are not indented to a multiple of four characters, but maybe those are all
wrong...

[Marijn Kruisselbrink, 2014-09-11 19:12:06]

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

[Marijn Kruisselbrink, 2014-09-11 19:12:06]

+asanka, to make sure that what I'm doing with worker-test-harness.js and
promiss_test all looks okay

[asanka, 2014-09-11 19:39:33]

asanka@chromium.org changed reviewers:
+ jsbell@chromium.org
- asanka@chromium.org

[asanka, 2014-09-11 19:47:56]

On 2014/09/11 19:12:06, Marijn Kruisselbrink wrote:
> +asanka, to make sure that what I'm doing with worker-test-harness.js and
> promiss_test all looks okay

Handing off to +jsbell since I'm not an authority here :)

In general we follow the coding style described at
http://dev.chromium.org/blink/serviceworker/testing

So promises based tests tend to look like:

promise_test(function(test) {
    return navigator.geofencing.registerRegion(...)
      .then(test.unreached_func('Promise should not have resolved'))
      .catch(function() {});
  }, 'registerRegion should fail');

Also, if https://codereview.chromium.org/430993002/ were landed, I'd recommend
using assert_promise_throws() instead for testing promises that are expected to
throw (it's expected to throw an exception, right?).

[jsbell, 2014-09-11 21:13:35]

Overall lgtm.

Agree with peter that we should plan to move the SW helpers to somewhere common
once they're a little more stable.

https://codereview.chromium.org/543203003/diff/40001/LayoutTests/geofencing/g...
File LayoutTests/geofencing/geofencing-not-implemented.html (right):

https://codereview.chromium.org/543203003/diff/40001/LayoutTests/geofencing/g...
LayoutTests/geofencing/geofencing-not-implemented.html:1: <!DOCTYPE html>
Nit: add a <title> describing the file

https://codereview.chromium.org/543203003/diff/40001/LayoutTests/geofencing/g...
LayoutTests/geofencing/geofencing-not-implemented.html:17:
promise_test(function(test) {
Factor out a rejecting_promise_test() helper?

... ah, asanka suggested (in the future) using assert_promise_throws() - that'd
be another way to reduce duplication.

(We should probably rename that to 'assert_promise_rejects' instead, though.
I'll note on the other bug.)

https://codereview.chromium.org/543203003/diff/40001/LayoutTests/http/tests/g...
File LayoutTests/http/tests/geofencing/resources/worker.js (right):

https://codereview.chromium.org/543203003/diff/40001/LayoutTests/http/tests/g...
LayoutTests/http/tests/geofencing/resources/worker.js:4: return
navigator.geofencing.registerRegion(new CircularGeofencingRegion({latitude:
37.421999, longitude: -122.084015}))
Maybe break this line (and the similarly long line in the .html?)

(Not strictly required, but doesn't hurt and the SW tests follow the 80-char
rule)

https://codereview.chromium.org/543203003/diff/40001/LayoutTests/http/tests/g...
File LayoutTests/http/tests/geofencing/service-worker.html (right):

https://codereview.chromium.org/543203003/diff/40001/LayoutTests/http/tests/g...
LayoutTests/http/tests/geofencing/service-worker.html:1: <!DOCTYPE html>
Add a <title>

[Marijn Kruisselbrink, 2014-09-11 21:52:44]

Patchset #3 (id:60001) has been deleted

[Marijn Kruisselbrink, 2014-09-11 21:54:27]

https://codereview.chromium.org/543203003/diff/40001/LayoutTests/geofencing/g...
File LayoutTests/geofencing/geofencing-not-implemented.html (right):

https://codereview.chromium.org/543203003/diff/40001/LayoutTests/geofencing/g...
LayoutTests/geofencing/geofencing-not-implemented.html:1: <!DOCTYPE html>
On 2014/09/11 21:13:35, jsbell wrote:
> Nit: add a <title> describing the file

Done.

https://codereview.chromium.org/543203003/diff/40001/LayoutTests/geofencing/g...
LayoutTests/geofencing/geofencing-not-implemented.html:17:
promise_test(function(test) {
On 2014/09/11 21:13:35, jsbell wrote:
> Factor out a rejecting_promise_test() helper?
> 
> ... ah, asanka suggested (in the future) using assert_promise_throws() -
that'd
> be another way to reduce duplication.
> 
> (We should probably rename that to 'assert_promise_rejects' instead, though.
> I'll note on the other bug.)
Yeah, something like assert_promise_throws would be nice for this. It would be
nice for that though if promise_test and assert_promise_throws/rejects are in a
file other than worker-test-harness.js, so they can actually be used by
non-worker promise based tests as well.

https://codereview.chromium.org/543203003/diff/40001/LayoutTests/http/tests/g...
File LayoutTests/http/tests/geofencing/resources/worker.js (right):

https://codereview.chromium.org/543203003/diff/40001/LayoutTests/http/tests/g...
LayoutTests/http/tests/geofencing/resources/worker.js:4: return
navigator.geofencing.registerRegion(new CircularGeofencingRegion({latitude:
37.421999, longitude: -122.084015}))
On 2014/09/11 21:13:35, jsbell wrote:
> Maybe break this line (and the similarly long line in the .html?)
> 
> (Not strictly required, but doesn't hurt and the SW tests follow the 80-char
> rule)

Done. And I changed the indentation a bit as well, to match the SW layout tests
(even though these are not SW tests, but the general layouttests styleguide
seems to explicitly allow things like two space indents anywhere).

[Marijn Kruisselbrink, 2014-09-11 21:54:58]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2014-09-11 21:56:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/543203003/80001

[commit-bot: I haz the power, 2014-09-11 22:59:35]

Committed patchset #3 (id:80001) as 181855