|
|
Descriptionwebrtc: Fix missing port forwarding on CrOS
There is a local streaming server started in webrtc benchmark for
CrOS browser to connect to, however, the http server is not on the
CrOS device and is not reachable from the device without remote
port forwarding. The patch addresses this issue by creating a
remote port forwarder if the local server and device are not on
the same machine.
BUG=chromium:740900
TEST=run_benchmark --browser=cros-chrome --output-format=chartjson --remote=DUT_IP webrtc
Signed-off-by: Chung-yih Wang <cywang@chromium.org>
Review-Url: https://codereview.chromium.org/2982743002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/1075e5dc8bc2ef383e67391639b0a23eed9f3546
Patch Set 1 #
Total comments: 4
Patch Set 2 : webrtc: Fix missing port forwarding on CrOS #
Total comments: 2
Patch Set 3 : webrtc: Fix missing port forwarding on CrOS #Patch Set 4 : webrtc: Fix missing port forwarding on CrOS #Patch Set 5 : webrtc: Fix missing port forwarding on CrOS and android #
Total comments: 4
Patch Set 6 : webrtc: Fix missing port forwarding on CrOS and android #
Total comments: 2
Patch Set 7 : webrtc: Fix missing port forwarding on CrOS #
Messages
Total messages: 44 (18 generated)
cywang@chromium.org changed reviewers: + achuith@chromium.org, bccheng@chromium.org, nednguyen@google.com
https://codereview.chromium.org/2982743002/diff/1/telemetry/telemetry/core/cr... File telemetry/telemetry/core/cros_interface.py (right): https://codereview.chromium.org/2982743002/diff/1/telemetry/telemetry/core/cr... telemetry/telemetry/core/cros_interface.py:156: # As remote port forwarding might conflicts with the control socket nit: s/conflicts/conflict https://codereview.chromium.org/2982743002/diff/1/telemetry/telemetry/core/cr... telemetry/telemetry/core/cros_interface.py:157: # sharing, skip the control socket args if it is for remote pot forwarding. s/pot/port
Thanks! https://codereview.chromium.org/2982743002/diff/1/telemetry/telemetry/core/cr... File telemetry/telemetry/core/cros_interface.py (right): https://codereview.chromium.org/2982743002/diff/1/telemetry/telemetry/core/cr... telemetry/telemetry/core/cros_interface.py:156: # As remote port forwarding might conflicts with the control socket On 2017/07/13 19:30:00, malmnas wrote: > nit: s/conflicts/conflict Done. https://codereview.chromium.org/2982743002/diff/1/telemetry/telemetry/core/cr... telemetry/telemetry/core/cros_interface.py:157: # sharing, skip the control socket args if it is for remote pot forwarding. On 2017/07/13 19:30:00, malmnas wrote: > s/pot/port Done.
lgtm
dtosic@chromium.org changed reviewers: + dtosic@chromium.org
The CQ bit was checked by dtosic@chromium.org
lgtm Thanks!
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-catapult-committers". Note that this has nothing to do with OWNERS files.
cywang@chromium.org changed reviewers: + wuchengli@chromium.org
Hi Wu-cheng, Please help to review the patch, thanks!
On 2017/07/17 10:30:57, cywang wrote: > Hi Wu-cheng, > > Please help to review the patch, thanks! lgtm. Have you verified that this does not break any of the normal flows?
https://codereview.chromium.org/2982743002/diff/20001/telemetry/telemetry/cor... File telemetry/telemetry/core/platform.py (right): https://codereview.chromium.org/2982743002/diff/20001/telemetry/telemetry/cor... telemetry/telemetry/core/platform.py:418: if not self._platform_backend._cri.local: This seems very wrong. Not all platform_backend have "._cri" object, so this will break on non ChromeOS backend.
https://codereview.chromium.org/2982743002/diff/20001/telemetry/telemetry/cor... File telemetry/telemetry/core/platform.py (right): https://codereview.chromium.org/2982743002/diff/20001/telemetry/telemetry/cor... telemetry/telemetry/core/platform.py:418: if not self._platform_backend._cri.local: On 2017/07/18 00:44:58, nednguyen wrote: > This seems very wrong. Not all platform_backend have "._cri" object, so this > will break on non ChromeOS backend. Thanks a lot, I change the section to make it CrOS only.
On 2017/07/17 19:09:21, achuithb wrote: > lgtm. Have you verified that this does not break any of the normal flows? Hi Achuith, I just verify with 1. test_that with args 'local=True' and 2. run_benchmark directly. Other regular test_that should work fine. Do you have any test in mind we should verify with?
On 2017/07/18 09:19:13, cywang wrote: > On 2017/07/17 19:09:21, achuithb wrote: > > lgtm. Have you verified that this does not break any of the normal flows? > > Hi Achuith, > > I just verify with 1. test_that with args 'local=True' and 2. run_benchmark > directly. Other regular test_that should work fine. Do you have any test in mind > we should verify with? 1) If this works for all remote platforms, we should change the logic to: if self._platform.is_remote: .... 2) You should add tests for this that would catch the bug. I expect the test to also pass on android platform (which is also remote).
On 2017/07/18 12:47:31, nednguyen wrote: > On 2017/07/18 09:19:13, cywang wrote: > > On 2017/07/17 19:09:21, achuithb wrote: > > > lgtm. Have you verified that this does not break any of the normal flows? > > > > Hi Achuith, > > > > I just verify with 1. test_that with args 'local=True' and 2. run_benchmark > > directly. Other regular test_that should work fine. Do you have any test in > mind > > we should verify with? > > 1) If this works for all remote platforms, we should change the logic to: > if self._platform.is_remote: > .... > > 2) You should add tests for this that would catch the bug. I expect the test to > also pass on android platform (which is also remote). Done, PTAL, thanks!
Please help to test more android devices/browsers as I do not have enough platform to verify if there is any hiccup from this patch. However, it works on Nexus 5x. Thanks!
The CQ bit was checked by achuith@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...
unit tests? https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/int... File telemetry/telemetry/internal/platform/android_platform_backend.py (right): https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/android_platform_backend.py:162: # Android device is connected via adb which is on remote nit: period at the end of comment. https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/int... File telemetry/telemetry/internal/platform/cros_platform_backend.py (right): https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/cros_platform_backend.py:62: # Check if CrOS device is remote nit: period at the end of comment
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Catapult Android Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20An...)
On 2017/07/19 17:50:32, achuithb wrote: > unit tests? > > https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/int... > File telemetry/telemetry/internal/platform/android_platform_backend.py (right): > > https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/int... > telemetry/telemetry/internal/platform/android_platform_backend.py:162: # Android > device is connected via adb which is on remote > nit: period at the end of comment. > > https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/int... > File telemetry/telemetry/internal/platform/cros_platform_backend.py (right): > > https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/int... > telemetry/telemetry/internal/platform/cros_platform_backend.py:62: # Check if > CrOS device is remote > nit: period at the end of comment Android test seems to be failing
On 2017/07/20 02:28:07, nednguyen wrote: > On 2017/07/19 17:50:32, achuithb wrote: > > unit tests? > > > > > https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/int... > > File telemetry/telemetry/internal/platform/android_platform_backend.py > (right): > > > > > https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/int... > > telemetry/telemetry/internal/platform/android_platform_backend.py:162: # > Android > > device is connected via adb which is on remote > > nit: period at the end of comment. > > > > > https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/int... > > File telemetry/telemetry/internal/platform/cros_platform_backend.py (right): > > > > > https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/int... > > telemetry/telemetry/internal/platform/cros_platform_backend.py:62: # Check if > > CrOS device is remote > > nit: period at the end of comment > > Android test seems to be failing Could you provide more details about where it failed? I am not so sure how we really run the test automatically, for my case, I have to manually grant permission of camera, microphone for the first time, and click ACCEPT button for launching Chrome in each subtest? The webrtc itself should be working with the patch, but not for those environment setup.
On 2017/07/20 03:30:17, cywang wrote: > On 2017/07/20 02:28:07, nednguyen wrote: > > On 2017/07/19 17:50:32, achuithb wrote: > > > unit tests? > > > > > > > > > https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/int... > > > File telemetry/telemetry/internal/platform/android_platform_backend.py > > (right): > > > > > > > > > https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/int... > > > telemetry/telemetry/internal/platform/android_platform_backend.py:162: # > > Android > > > device is connected via adb which is on remote > > > nit: period at the end of comment. > > > > > > > > > https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/int... > > > File telemetry/telemetry/internal/platform/cros_platform_backend.py (right): > > > > > > > > > https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/int... > > > telemetry/telemetry/internal/platform/cros_platform_backend.py:62: # Check > if > > > CrOS device is remote > > > nit: period at the end of comment > > > > Android test seems to be failing > Could you provide more details about where it failed? I am not so sure how we > really run the test automatically, for my case, I have to manually grant > permission of camera, microphone for the first time, and click ACCEPT button for > launching Chrome in each subtest? The webrtc itself should be working with the > patch, but not for those environment setup. I meant the "Catapult Android Tryserver " bot. This is from the log: https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20An...
I have run the unit test locally against android browser: 1021 tests passed in 4704.2s, 109 skipped, 0 failures. https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/int... File telemetry/telemetry/internal/platform/android_platform_backend.py (right): https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/android_platform_backend.py:162: # Android device is connected via adb which is on remote On 2017/07/19 17:50:32, achuithb wrote: > nit: period at the end of comment. Done. https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/int... File telemetry/telemetry/internal/platform/cros_platform_backend.py (right): https://codereview.chromium.org/2982743002/diff/80001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/cros_platform_backend.py:62: # Check if CrOS device is remote On 2017/07/19 17:50:32, achuithb wrote: > nit: period at the end of comment Done.
The CQ bit was checked by cywang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org, dtosic@chromium.org, malmnas@google.com Link to the patchset: https://codereview.chromium.org/2982743002/#ps100001 (title: "webrtc: Fix missing port forwarding on CrOS and android")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Linux Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Li...)
lg2me overall. Though can you add a test case that would fail without this fix? I imagine the test case would be similar to whatever is breaking webrtc benchmark on remote CrOs device. You can look into BrowserTest in browser_unittest.py https://codereview.chromium.org/2982743002/diff/100001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/platform_backend.py (right): https://codereview.chromium.org/2982743002/diff/100001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/platform_backend.py:116: def IsDeviceRemote(self): s/IsDeviceRemote/IsRemoteDevice https://codereview.chromium.org/2982743002/diff/100001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/platform_backend.py:117: """Check if target browser is on remote device. The platform concept in Telemetry is agnostic about browser. So the comment here should be "Check if this platform is on a remote device"
Okay, let's focus on ChromeOS first. For unit tests, I probably don't need to add any additional test for this patch as the patch actually fixes the unittest for ChromeOS platform. e.g. Without this fix, the following unittest fails: catapult_build/fetch_telemetry_deps_and_run_tests '--browser=cros-chrome' --remote=_CHROMEBOOK_IP_ telemetry.core.memory_cache_http_server_unittest.MemoryCacheHTTPServerTest.testBasicHostingAndRangeRequests
The CQ bit was checked by cywang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org, dtosic@chromium.org, malmnas@google.com Link to the patchset: https://codereview.chromium.org/2982743002/#ps120001 (title: "webrtc: Fix missing port forwarding on CrOS")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1503891512581900, "parent_rev": "151d2f0d6a405c6a0e55070e8ad3afe4f3e8f0e9", "commit_rev": "1075e5dc8bc2ef383e67391639b0a23eed9f3546"}
Message was sent while issue was closed.
Description was changed from ========== webrtc: Fix missing port forwarding on CrOS There is a local streaming server started in webrtc benchmark for CrOS browser to connect to, however, the http server is not on the CrOS device and is not reachable from the device without remote port forwarding. The patch addresses this issue by creating a remote port forwarder if the local server and device are not on the same machine. BUG=chromium:740900 TEST=run_benchmark --browser=cros-chrome --output-format=chartjson --remote=DUT_IP webrtc Signed-off-by: Chung-yih Wang <cywang@chromium.org> ========== to ========== webrtc: Fix missing port forwarding on CrOS There is a local streaming server started in webrtc benchmark for CrOS browser to connect to, however, the http server is not on the CrOS device and is not reachable from the device without remote port forwarding. The patch addresses this issue by creating a remote port forwarder if the local server and device are not on the same machine. BUG=chromium:740900 TEST=run_benchmark --browser=cros-chrome --output-format=chartjson --remote=DUT_IP webrtc Signed-off-by: Chung-yih Wang <cywang@chromium.org> Review-Url: https://codereview.chromium.org/2982743002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
Patchset #8 (id:140001) has been deleted
Message was sent while issue was closed.
Patchset #8 (id:160001) has been deleted |