Convert RTCPeerConnection-state.html to use testharness.js.

Convert RTCPeerConnection-state.html to use testharness.js.

BUG=614963
Committed: https://crrev.com/fb84b3b9087d1836e5b00773dbc5b280d4b5f1d8
Cr-Commit-Position: refs/heads/master@{#416974}

[qyearsley, 2016-09-04 22:55:50]

qyearsley@chromium.org changed reviewers:
+ guidou@chromium.org, jsbell@chromium.org

[Guido Urdaneta, 2016-09-05 08:13:51]

lgtm

[qyearsley, 2016-09-06 16:07:56]

https://codereview.chromium.org/2305263002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-state.html
(right):

https://codereview.chromium.org/2305263002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-state.html:15:
assert_equals(pc.signalingState, 'closed');
If I change this string to something else, the test times out instead of failing
quickly, but I'm not sure why; I thought that if a testharness.js comes across a
failing assertion then it fails immediately, but apparently that isn't
happening.

Does anyone know if I should be doing something different here?

[jsbell, 2016-09-06 16:26:35]

https://codereview.chromium.org/2305263002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-state.html
(right):

https://codereview.chromium.org/2305263002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-state.html:15:
assert_equals(pc.signalingState, 'closed');
On 2016/09/06 16:07:55, qyearsley wrote:
> If I change this string to something else, the test times out instead of
failing
> quickly, but I'm not sure why; I thought that if a testharness.js comes across
a
> failing assertion then it fails immediately, but apparently that isn't
> happening.
> 
> Does anyone know if I should be doing something different here?

In async tests you must wrap callback functions so that the assertion exception
is caught.

https://codereview.chromium.org/2305263002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-state.html:19:
test.step(function() {
test.step() is not needed here since this is already the body of the async_test
function.

https://codereview.chromium.org/2305263002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-state.html:22:
pc.onsignalingstatechange = stateChanged;
The timeout should go away if you replace this with:

pc.onsignalingstatechange = test.step_func(stateChanged);

[qyearsley, 2016-09-06 19:17:41]

On 2016/09/06 at 16:26:35, jsbell wrote:
>
https://codereview.chromium.org/2305263002/diff/1/third_party/WebKit/LayoutTe...
> File
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-state.html
(right):
> 
>
https://codereview.chromium.org/2305263002/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-state.html:15:
assert_equals(pc.signalingState, 'closed');
> On 2016/09/06 16:07:55, qyearsley wrote:
> > If I change this string to something else, the test times out instead of
failing
> > quickly, but I'm not sure why; I thought that if a testharness.js comes
across a
> > failing assertion then it fails immediately, but apparently that isn't
> > happening.
> > 
> > Does anyone know if I should be doing something different here?
> 
> In async tests you must wrap callback functions so that the assertion
exception is caught.
> 
>
https://codereview.chromium.org/2305263002/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-state.html:19:
test.step(function() {
> test.step() is not needed here since this is already the body of the
async_test function.
> 
>
https://codereview.chromium.org/2305263002/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-state.html:22:
pc.onsignalingstatechange = stateChanged;
> The timeout should go away if you replace this with:
> 
> pc.onsignalingstatechange = test.step_func(stateChanged);

Excellent -- with those changes, the test now completes quickly, and fails
quickly as expected when I change those strings :-)

[jsbell, 2016-09-06 20:02:07]

lgtm with one nit

(Ideally the file would have a title, and it might be more readable to make the
callback an anonymous function, but since this is a conversion it's fine to keep
the existing style)

https://codereview.chromium.org/2305263002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-state.html
(right):

https://codereview.chromium.org/2305263002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-state.html:16:
var pc = new webkitRTCPeerConnection({iceServers: []}, null);
The `var` here is not required.

[qyearsley, 2016-09-07 16:00:21]

On 2016/09/06 at 20:02:07, jsbell wrote:
> lgtm with one nit
> 
> (Ideally the file would have a title, and it might be more readable to make
the callback an anonymous function, but since this is a conversion it's fine to
keep the existing style)
> 
>
https://codereview.chromium.org/2305263002/diff/20001/third_party/WebKit/Layo...
> File
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-state.html
(right):
> 
>
https://codereview.chromium.org/2305263002/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-state.html:16:
var pc = new webkitRTCPeerConnection({iceServers: []}, null);
> The `var` here is not required.

Excellent, thanks for the suggestions :-)

[qyearsley, 2016-09-07 16:00:36]

The CQ bit was checked by qyearsley@chromium.org

[qyearsley, 2016-09-07 16:00:36]

The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org,
jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/2305263002/#ps40001
(title: "Use anonymous function; add title.")

[commit-bot: I haz the power, 2016-09-07 16:01:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 17:11:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-07 17:13:21]

Description was changed from

==========
Convert RTCPeerConnection-state.html to use testharness.js.

BUG=614963
==========

to

==========
Convert RTCPeerConnection-state.html to use testharness.js.

BUG=614963

Committed: https://crrev.com/fb84b3b9087d1836e5b00773dbc5b280d4b5f1d8
Cr-Commit-Position: refs/heads/master@{#416974}
==========

[commit-bot: I haz the power, 2016-09-07 17:13:22]

Patchset 3 (id:??) landed as
https://crrev.com/fb84b3b9087d1836e5b00773dbc5b280d4b5f1d8
Cr-Commit-Position: refs/heads/master@{#416974}