bluetooth: Use sequential_promise_test to stop test from running in parallel.

bluetooth: Use sequential_promise_test to stop test from running in parallel.

Each test calls setBluetoothMockDataSet() which sets a global variable
in the bluetooth dispatcher. Since async_tests run in parallel the calls
to setBluetoothMockDataSet() where interleaving causing tests to be
flaky. This patch uses sequential_promise_test to ensure that tests don't interfere with each other.

BUG=420284, 470307
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193657

[ortuno, 2015-04-07 23:24:57]

ortuno@chromium.org changed reviewers:
+ jyasskin@chromium.org, scheib@chromium.org

[ortuno, 2015-04-07 23:24:58]

Please review @jyasskin, @scheib

[Jeffrey Yasskin, 2015-04-07 23:30:01]

jyasskin@chromium.org changed reviewers:
+ esprehn@chromium.org

[Jeffrey Yasskin, 2015-04-07 23:30:01]

+Elliott since he replied about async_test.

[Jeffrey Yasskin, 2015-04-07 23:46:38]

https://codereview.chromium.org/1062393002/diff/20001/LayoutTests/bluetooth/r...
File LayoutTests/bluetooth/requestDevice.html (right):

https://codereview.chromium.org/1062393002/diff/20001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:20: nextTest();
Probably use `Promise.resolve().then(nextTest)` or `setTimeout(nextTest, 0)` to
avoid calling the next test recursively.

https://codereview.chromium.org/1062393002/diff/20001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:29: getLock(function(t) {
I'm not sure the test will wait for all of the tests if async_test isn't called
synchronously. Maybe call getLock inside async_test?

async_test(function(t) {
  getLock(function() {
    testRunner.setBluetoothMockDataSet...
  });
});

https://codereview.chromium.org/1062393002/diff/20001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:31:
testRunner.setBluetoothMockDataSet('RejectRequestDevice_NotFoundError');
If this throws, it'll hang the test. It'd be better to incorporate the locking
into async_test and step_func, but we may also have a guarantee that the
relevant functions here don't throw.

[esprehn, 2015-04-08 00:05:44]

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

[esprehn, 2015-04-08 00:05:44]

This feels like we're hacking around the test framework, we should make the
framework either have this mode, or just do run all async tests in sequence
(basically all other JS frameworks do this).

[Jeffrey Yasskin, 2015-04-08 00:07:46]

On 2015/04/08 00:05:44, esprehn wrote:
> This feels like we're hacking around the test framework, we should make the
> framework either have this mode, or just do run all async tests in sequence
> (basically all other JS frameworks do this).

Is the right way to do that to patch
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...,
or do we need to go through the W3C somehow?

[esprehn, 2015-04-08 00:10:29]

I think you can do this simpler like:

var sequence = Promise.resolve();

function wait(fn) {
  sequence.then(function() {
     return new Promise(function(resolve) {
       fn(resolve);
     });
  });
}

async_test(function(t) {
  wait(function(done) {
    // write your test here.
    done();
  });
});

https://codereview.chromium.org/1062393002/diff/20001/LayoutTests/bluetooth/r...
File LayoutTests/bluetooth/requestDevice.html (right):

https://codereview.chromium.org/1062393002/diff/20001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:8: var getLock = function(test) {
function getLock, no need for the var.

https://codereview.chromium.org/1062393002/diff/20001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:17: var releaseLock = function() {
function releaseLock()

[esprehn, 2015-04-08 00:11:22]

On 2015/04/08 at 00:07:46, jyasskin wrote:
> On 2015/04/08 00:05:44, esprehn wrote:
> > This feels like we're hacking around the test framework, we should make the
> > framework either have this mode, or just do run all async tests in sequence
> > (basically all other JS frameworks do this).
> 
> Is the right way to do that to patch
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...,
or do we need to go through the W3C somehow?

I think you'd need to do this upstream in the w3c. dpranke@ probably knows.

[Dirk Pranke, 2015-04-08 00:13:45]

On 2015/04/08 00:11:22, esprehn wrote:
> On 2015/04/08 at 00:07:46, jyasskin wrote:
> > On 2015/04/08 00:05:44, esprehn wrote:
> > > This feels like we're hacking around the test framework, we should make
the
> > > framework either have this mode, or just do run all async tests in
sequence
> > > (basically all other JS frameworks do this).
> > 
> > Is the right way to do that to patch
>
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...,
> or do we need to go through the W3C somehow?
> 
> I think you'd need to do this upstream in the w3c. dpranke@ probably knows.

Yup, probably should get it fixed upstream. You should not locally patch this
file.

[ortuno, 2015-04-10 16:46:43]

On 2015/04/08 at 00:13:45, dpranke wrote:
> On 2015/04/08 00:11:22, esprehn wrote:
> > On 2015/04/08 at 00:07:46, jyasskin wrote:
> > > On 2015/04/08 00:05:44, esprehn wrote:
> > > > This feels like we're hacking around the test framework, we should make
the
> > > > framework either have this mode, or just do run all async tests in
sequence
> > > > (basically all other JS frameworks do this).
> > > 
> > > Is the right way to do that to patch
> >
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...,
> > or do we need to go through the W3C somehow?
> > 
> > I think you'd need to do this upstream in the w3c. dpranke@ probably knows.
> 
> Yup, probably should get it fixed upstream. You should not locally patch this
file.

@dpranke I submitted a pull request:
https://github.com/w3c/testharness.js/pull/115 but I haven't gotten any
response. Could I, in the mean time, add the function to
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
?

Otherwise do you know who I could ping to get the PR reviewed?

[Dirk Pranke, 2015-04-10 20:38:03]

On 2015/04/10 16:46:43, ortuno wrote:
> @dpranke I submitted a pull request:
> https://github.com/w3c/testharness.js/pull/115 but I haven't gotten any
> response. Could I, in the mean time, add the function to
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> ?

No. 

I would either just skip this test for now, or manually fork the file with your
mods.

> 
> Otherwise do you know who I could ping to get the PR reviewed?

Maybe kenjibaheux@chromium.org, not sure. I'm not sure who all in Blink/Chromium
reviews stuff in wpt these days.

Or you could ping James Graham, james@hoppipolla.co.uk, or send an email to
public-test-infra@w3.org .

[Jeffrey Yasskin, 2015-04-10 23:56:13]

LGTM, but give Dirk/Elliott until Monday to chime in.

https://codereview.chromium.org/1062393002/diff/60001/LayoutTests/bluetooth/r...
File LayoutTests/bluetooth/requestDevice.html (right):

https://codereview.chromium.org/1062393002/diff/60001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:7: // Helper function to run promise
tests one after the other.
Add a TODO to remove this once
https://github.com/w3c/testharness.js/pull/115/files gets in.

[ortuno, 2015-04-10 23:59:06]

On 2015/04/10 at 23:56:13, jyasskin wrote:
> LGTM, but give Dirk/Elliott until Monday to chime in.
> 
>
https://codereview.chromium.org/1062393002/diff/60001/LayoutTests/bluetooth/r...
> File LayoutTests/bluetooth/requestDevice.html (right):
> 
>
https://codereview.chromium.org/1062393002/diff/60001/LayoutTests/bluetooth/r...
> LayoutTests/bluetooth/requestDevice.html:7: // Helper function to run promise
tests one after the other.
> Add a TODO to remove this once
https://github.com/w3c/testharness.js/pull/115/files gets in.

@esprehn & @dpranke: please take a look.

[Dirk Pranke, 2015-04-11 00:26:14]

looks plausible to me, but my knowledge of writing promise-based tests is not
strong. I defer to the others.

[esprehn, 2015-04-11 00:45:02]

Seems reasonable, lgtm

https://codereview.chromium.org/1062393002/diff/80001/LayoutTests/bluetooth/r...
File LayoutTests/bluetooth/requestDevice.html (right):

https://codereview.chromium.org/1062393002/diff/80001/LayoutTests/bluetooth/r...
LayoutTests/bluetooth/requestDevice.html:15: function() {
I think we usually put the function() inline with the then(

[ortuno, 2015-04-13 15:33:06]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2015-04-13 15:33:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
jyasskin@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1062393002/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-04-13 15:33:42]

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

[commit-bot: I haz the power, 2015-04-13 16:52:16]

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

[commit-bot: I haz the power, 2015-04-13 16:52:17]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/58971)

[ortuno, 2015-04-13 19:03:07]

The CQ bit was checked by ortuno@chromium.org

[commit-bot: I haz the power, 2015-04-13 19:03:33]

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

[scheib, 2015-04-13 22:33:11]

LGTM

[commit-bot: I haz the power, 2015-04-13 23:16:49]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193657