Teach the network simulator to wait for Promises.

Teach the network simulator to wait for Promises.

Before, it would continue as soon as the user's function returned, which might
not wait adequately for Promises in the user's code.

NOTRY=true

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

[Jeffrey Yasskin, 2014-09-03 22:11:49]

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

[Jeffrey Yasskin, 2014-09-03 22:11:50]

https://codereview.chromium.org/536163002/diff/1/Tools/GardeningServer/lib/ne...
File Tools/GardeningServer/lib/network-simulator.html (left):

https://codereview.chromium.org/536163002/diff/1/Tools/GardeningServer/lib/ne...
Tools/GardeningServer/lib/network-simulator.html:47: return new
Promise(function(resolve, reject) {
I think it's an antipattern to use the resolve/reject functions unless you have
to deal with an existing callback-taking system. Using them makes the first hop
of a promise asymmetric with the rest of the hops.

https://codereview.chromium.org/536163002/diff/1/Tools/GardeningServer/lib/ne...
File Tools/GardeningServer/lib/network-simulator.html (right):

https://codereview.chromium.org/536163002/diff/1/Tools/GardeningServer/lib/ne...
Tools/GardeningServer/lib/network-simulator.html:73: return
self.resolvePromises().then(function() {
I'm not sure we need the resolvePromises() step anymore, since now callers can
explicitly chain from those promises.

https://codereview.chromium.org/536163002/diff/1/Tools/GardeningServer/lib/ne...
Tools/GardeningServer/lib/network-simulator.html:78: self._assert(false, "Failed
to finish test:" + e);
I'm not sure if we want to throw a string here or the original error. Throwing
the original error gives a much better stack trace but isn't as clear that it
happened during cleanup.

https://codereview.chromium.org/536163002/diff/1/Tools/GardeningServer/lib/ne...
Tools/GardeningServer/lib/network-simulator.html:80: }).then(this._done,
this._done);
Mocha actually supports returning promises from the it() function
(http://visionmedia.github.io/mocha/#asynchronous-code), so we could omit all
the use of _done here and have callers "return runTest(...)" instead.

https://codereview.chromium.org/536163002/diff/1/Tools/GardeningServer/script...
File Tools/GardeningServer/scripts/test/results-tests.html (right):

https://codereview.chromium.org/536163002/diff/1/Tools/GardeningServer/script...
Tools/GardeningServer/scripts/test/results-tests.html:81: return
results.fetchResultsURLs({
The only changes here are the extra "}).then(function() {" lines, the addition
of a 'return' on the 'results.fetchResultsURLs(' lines, and the removal of
'done()'. Everything else is indentation.

[ojan, 2014-09-04 18:26:54]

lgtm

https://codereview.chromium.org/536163002/diff/1/Tools/GardeningServer/lib/ne...
File Tools/GardeningServer/lib/network-simulator.html (right):

https://codereview.chromium.org/536163002/diff/1/Tools/GardeningServer/lib/ne...
Tools/GardeningServer/lib/network-simulator.html:78: self._assert(false, "Failed
to finish test:" + e);
On 2014/09/03 at 22:11:50, Jeffrey Yasskin wrote:
> I'm not sure if we want to throw a string here or the original error. Throwing
the original error gives a much better stack trace but isn't as clear that it
happened during cleanup.

I think this is fine for now. Or, we could just do two _assert calls.

https://codereview.chromium.org/536163002/diff/1/Tools/GardeningServer/lib/ne...
Tools/GardeningServer/lib/network-simulator.html:80: }).then(this._done,
this._done);
On 2014/09/03 at 22:11:50, Jeffrey Yasskin wrote:
> Mocha actually supports returning promises from the it() function
(http://visionmedia.github.io/mocha/#asynchronous-code), so we could omit all
the use of _done here and have callers "return runTest(...)" instead.

sgtm

[Jeffrey Yasskin, 2014-09-04 18:48:11]

Thanks!

https://codereview.chromium.org/536163002/diff/1/Tools/GardeningServer/lib/ne...
File Tools/GardeningServer/lib/network-simulator.html (right):

https://codereview.chromium.org/536163002/diff/1/Tools/GardeningServer/lib/ne...
Tools/GardeningServer/lib/network-simulator.html:78: self._assert(false, "Failed
to finish test:" + e);
On 2014/09/04 18:26:53, ojan-only-code-yellow-reviews wrote:
> On 2014/09/03 at 22:11:50, Jeffrey Yasskin wrote:
> > I'm not sure if we want to throw a string here or the original error.
Throwing
> the original error gives a much better stack trace but isn't as clear that it
> happened during cleanup.
> 
> I think this is fine for now. Or, we could just do two _assert calls.

Two _assert calls doesn't help: Chai uses exceptions to report assertion
failures, so the second _assert would never run. I'll leave it like this for
now.

https://codereview.chromium.org/536163002/diff/1/Tools/GardeningServer/lib/ne...
Tools/GardeningServer/lib/network-simulator.html:80: }).then(this._done,
this._done);
On 2014/09/04 18:26:53, ojan-only-code-yellow-reviews wrote:
> On 2014/09/03 at 22:11:50, Jeffrey Yasskin wrote:
> > Mocha actually supports returning promises from the it() function
> (http://visionmedia.github.io/mocha/#asynchronous-code), so we could omit all
> the use of _done here and have callers "return runTest(...)" instead.
> 
> sgtm

After experimenting a bit, it looks safer to keep the explicit _done calls. If
we switch to returning a Promise, and a caller forgets to propagate that Promise
out of the it() function, any delayed assertions are simply ignored, which will
be hard to debug. Making people pass in a _done function makes sure Mocha knows
the test will be asynchronous.

[Jeffrey Yasskin, 2014-09-04 18:48:14]

The CQ bit was checked by jyasskin@chromium.org

[commit-bot: I haz the power, 2014-09-04 18:48:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jyasskin@chromium.org/536163002/1

[Jeffrey Yasskin, 2014-09-04 21:57:54]

The CQ bit was unchecked by jyasskin@chromium.org

[Jeffrey Yasskin, 2014-09-04 21:58:17]

The CQ bit was checked by jyasskin@chromium.org

[commit-bot: I haz the power, 2014-09-04 21:59:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jyasskin@chromium.org/536163002/1

[commit-bot: I haz the power, 2014-09-04 22:00:28]

Committed patchset #1 (id:1) as 181404