Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2273363004:
Convert RTCPeerConnection-ice test from js-test.js to testharness.js (Closed)
DescriptionConvert RTCPeerConnection-ice test from js-test.js to testharness.js
(This is my first CL so please let me know if I'm doing something wrong)
BUG=614963
Committed: https://crrev.com/c4e727c88c254a9d5a04432f1b540321bc85c137
Cr-Commit-Position: refs/heads/master@{#414747}
Patch Set 1 #Patch Set 2 : Convert RTCPeerConnection-ice test from js-test.js to testharness.js #
Total comments: 6
Patch Set 3 : Address feedback, assign and address window variables explicitly #
Total comments: 2
Messages
Total messages: 21 (8 generated)
The CQ bit was checked by jeffcarp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Convert RTCPeerConnection-ice test from js-test.js to testharness.js BUG=614963 (This is my first CL so please let me know if I'm doing something wrong) ========== to ========== Convert RTCPeerConnection-ice test from js-test.js to testharness.js BUG=614963 (This is my first CL so please let me know if I'm doing something wrong) ==========
jeffcarp@chromium.org changed reviewers: + guidou@chromium.org, qyearsley@chromium.org
LGTM, with some notes/suggestions https://codereview.chromium.org/2273363004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html (right): https://codereview.chromium.org/2273363004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html:29: testRTC.done(); To make this more explicitly refer to the global variable, you could change this to window.testRTC.done(). https://codereview.chromium.org/2273363004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html:65: window.error = error; Looks like we may be able to remove window.error; not sure though. https://codereview.chromium.org/2273363004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html:103: window.testRTC = async_test("Tests the RTCPeerConnection Ice functionality."); For consistency, it seems like we should declare all global variables the same way -- either set window.pc, window.iceCandidate and window.calledCallbacks at the top, or use `var testRTC` here. The former seems more explicit, but both seem OK.
Description was changed from ========== Convert RTCPeerConnection-ice test from js-test.js to testharness.js BUG=614963 (This is my first CL so please let me know if I'm doing something wrong) ========== to ========== Convert RTCPeerConnection-ice test from js-test.js to testharness.js (This is my first CL so please let me know if I'm doing something wrong) BUG=614963 ==========
On 2016/08/26 at 00:49:39, qyearsley wrote: > LGTM, with some notes/suggestions > > https://codereview.chromium.org/2273363004/diff/20001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html (right): > > https://codereview.chromium.org/2273363004/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html:29: testRTC.done(); > To make this more explicitly refer to the global variable, you could change this to window.testRTC.done(). > > https://codereview.chromium.org/2273363004/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html:65: window.error = error; > Looks like we may be able to remove window.error; not sure though. > > https://codereview.chromium.org/2273363004/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html:103: window.testRTC = async_test("Tests the RTCPeerConnection Ice functionality."); > For consistency, it seems like we should declare all global variables the same way -- either set window.pc, window.iceCandidate and window.calledCallbacks at the top, or use `var testRTC` here. > > The former seems more explicit, but both seem OK. I pushed a new patch that prefixes assignments and references with window. Feels a little verbose this way but I'm ok with it.
https://codereview.chromium.org/2273363004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html (right): https://codereview.chromium.org/2273363004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html:29: testRTC.done(); On 2016/08/26 at 00:49:38, qyearsley wrote: > To make this more explicitly refer to the global variable, you could change this to window.testRTC.done(). I like that. https://codereview.chromium.org/2273363004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html:65: window.error = error; On 2016/08/26 at 00:49:38, qyearsley wrote: > Looks like we may be able to remove window.error; not sure though. It's not used elsewhere, so it looks like we can, thanks! https://codereview.chromium.org/2273363004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html:103: window.testRTC = async_test("Tests the RTCPeerConnection Ice functionality."); On 2016/08/26 at 00:49:38, qyearsley wrote: > For consistency, it seems like we should declare all global variables the same way -- either set window.pc, window.iceCandidate and window.calledCallbacks at the top, or use `var testRTC` here. > > The former seems more explicit, but both seem OK. I'll check if there's a convention in other tests but I prefer window.*
LGTM
The CQ bit was checked by jeffcarp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from qyearsley@chromium.org Link to the patchset: https://codereview.chromium.org/2273363004/#ps40001 (title: "Address feedback, assign and address window variables explicitly")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Convert RTCPeerConnection-ice test from js-test.js to testharness.js (This is my first CL so please let me know if I'm doing something wrong) BUG=614963 ========== to ========== Convert RTCPeerConnection-ice test from js-test.js to testharness.js (This is my first CL so please let me know if I'm doing something wrong) BUG=614963 Committed: https://crrev.com/c4e727c88c254a9d5a04432f1b540321bc85c137 Cr-Commit-Position: refs/heads/master@{#414747} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c4e727c88c254a9d5a04432f1b540321bc85c137 Cr-Commit-Position: refs/heads/master@{#414747}
Message was sent while issue was closed.
https://codereview.chromium.org/2273363004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html (right): https://codereview.chromium.org/2273363004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html:25: window.testRTC.done(); I was experimenting with trying to get convert tests to use testharness.js, and was having some trouble; then I tried running this test, and noticed that: - This test times out (instead of showing the testharness.js UI with "pass") if I load the file directly in the browser. - It passes if I run it with run-webkit-tests, but if I change it (e.g. by changing the array above) then it times out instead of passing, although I expected it to fail quickly instead of timing out. Jeff, do you remember seeing the same behavior?
Message was sent while issue was closed.
https://codereview.chromium.org/2273363004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html (right): https://codereview.chromium.org/2273363004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html:25: window.testRTC.done(); On 2016/09/06 at 16:09:36, qyearsley wrote: > I was experimenting with trying to get convert tests to use testharness.js, and was having some trouble; then I tried running this test, and noticed that: > > - This test times out (instead of showing the testharness.js UI with "pass") if I load the file directly in the browser. > - It passes if I run it with run-webkit-tests, but if I change it (e.g. by changing the array above) then it times out instead of passing, although I expected it to fail quickly instead of timing out. > > Jeff, do you remember seeing the same behavior? When I was testing this I remember it failing quickly, not timing out. But it sounds like there's a way to make it time out instead of failing quickly. I tried replicating the existing logic from the test - is the timeout problem something that converting to testharness.js introduced, or something that was there before?
Message was sent while issue was closed.
On 2016/09/06 at 17:26:38, jeffcarp wrote: > https://codereview.chromium.org/2273363004/diff/40001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html (right): > > https://codereview.chromium.org/2273363004/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-ice.html:25: window.testRTC.done(); > On 2016/09/06 at 16:09:36, qyearsley wrote: > > I was experimenting with trying to get convert tests to use testharness.js, and was having some trouble; then I tried running this test, and noticed that: > > > > - This test times out (instead of showing the testharness.js UI with "pass") if I load the file directly in the browser. > > - It passes if I run it with run-webkit-tests, but if I change it (e.g. by changing the array above) then it times out instead of passing, although I expected it to fail quickly instead of timing out. > > > > Jeff, do you remember seeing the same behavior? > > When I was testing this I remember it failing quickly, not timing out. But it sounds like there's a way to make it time out instead of failing quickly. I tried replicating the existing logic from the test - is the timeout problem something that converting to testharness.js introduced, or something that was there before? Was able to reproduce this in the browser, working on a patch to use step_func instead.
Message was sent while issue was closed.
Message was sent while issue was closed.
On 2016/09/06 at 19:31:44, jeffcarp wrote: > Hey +qyearsley, it seems like `oniceconnectionstatechange` doesn't get called in the browser. Do you think there's a flag you have to enable for P2P connections like this? I refactored the code in https://codereview.chromium.org/2316573004 to try to fix the in-browser test failure but it didn't solve the problem.
Message was sent while issue was closed.
On 2016/09/07 at 17:52:09, jeffcarp wrote: > On 2016/09/06 at 19:31:44, jeffcarp wrote: > > > > Hey +qyearsley, it seems like `oniceconnectionstatechange` doesn't get called in the browser. Do you think there's a flag you have to enable for P2P connections like this? > > I refactored the code in https://codereview.chromium.org/2316573004 to try to fix the in-browser test failure but it didn't solve the problem. I'm not sure; guidou@, do you know? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
