Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2852153002:
Fix flaky NQE QUIC test (Closed)
|
Created:
3 years, 7 months ago by tbansal1 Modified:
3 years, 7 months ago Reviewers:
pauljensen CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix flaky NQE QUIC test
Block the test until RTT value is received. This ensures that the test is blocked until the RTT listener actually receives the notification.
BUG=716998
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Review-Url: https://codereview.chromium.org/2852153002
Cr-Commit-Position: refs/heads/master@{#468775}
Committed: https://chromium.googlesource.com/chromium/src/+/b81e1a532d7bdfab27546d5fa6f6d4e664fc494b
Patch Set 1 #
Total comments: 6
Patch Set 2 : pauljensen comments #
Total comments: 4
Patch Set 3 : pauljensen comments #
Messages
Total messages: 24 (17 generated)
Description was changed from ========== Fix flaky NQE test BUG= ========== to ========== Fix flaky NQE test BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
The CQ bit was checked by tbansal@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix flaky NQE test BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Fix flaky NQE QUIC test Block the test until RTT value is received. BUG=716998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
Description was changed from ========== Fix flaky NQE QUIC test Block the test until RTT value is received. BUG=716998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Fix flaky NQE QUIC test Block the test until RTT value is received. This ensures that the test is blocked until the RTT listener actually receives the notification. BUG=716998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
tbansal@chromium.org changed reviewers: + pauljensen@chromium.org
pauljensen: ptal. Thanks.
https://codereview.chromium.org/2852153002/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/TestNetworkQualityRttListener.java (right): https://codereview.chromium.org/2852153002/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/TestNetworkQualityRttListener.java:25: TestNetworkQualityRttListener(Executor executor, ConditionVariable waitForUrlRequestRtt) { Can you add a comment for this function please https://codereview.chromium.org/2852153002/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/TestNetworkQualityRttListener.java:25: TestNetworkQualityRttListener(Executor executor, ConditionVariable waitForUrlRequestRtt) { rather than pass a condition variable (CV) in, why not just have one internally defined, and add a block() function that just calls block on the CV? https://codereview.chromium.org/2852153002/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/TestNetworkQualityRttListener.java:33: if (mWaitForUrlRequestRtt != null && source == 0) { why the "source == 0"?
pauljensen: ptal. Thanks!! https://codereview.chromium.org/2852153002/diff/1/components/cronet/android/t... File components/cronet/android/test/javatests/src/org/chromium/net/TestNetworkQualityRttListener.java (right): https://codereview.chromium.org/2852153002/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/TestNetworkQualityRttListener.java:25: TestNetworkQualityRttListener(Executor executor, ConditionVariable waitForUrlRequestRtt) { On 2017/05/02 12:47:28, pauljensen wrote: > Can you add a comment for this function please Done. https://codereview.chromium.org/2852153002/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/TestNetworkQualityRttListener.java:25: TestNetworkQualityRttListener(Executor executor, ConditionVariable waitForUrlRequestRtt) { On 2017/05/02 12:47:28, pauljensen wrote: > rather than pass a condition variable (CV) in, why not just have one internally > defined, and add a block() function that just calls block on the CV? Good suggestion. Done. Also, done for throughput listener. https://codereview.chromium.org/2852153002/diff/1/components/cronet/android/t... components/cronet/android/test/javatests/src/org/chromium/net/TestNetworkQualityRttListener.java:33: if (mWaitForUrlRequestRtt != null && source == 0) { On 2017/05/02 12:47:28, pauljensen wrote: > why the "source == 0"? Added a comment. ptal.
The CQ bit was checked by tbansal@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2852153002/diff/20001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/TestNetworkQualityRttListener.java (right): https://codereview.chromium.org/2852153002/diff/20001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/TestNetworkQualityRttListener.java:18: // Signals when the first RTT observation at the URL request layer is received.. nit: extra period at the end https://codereview.chromium.org/2852153002/diff/20001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/TestNetworkQualityThroughputListener.java (right): https://codereview.chromium.org/2852153002/diff/20001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/TestNetworkQualityThroughputListener.java:17: // Signals when the first throughput observation is received.. ditto
https://codereview.chromium.org/2852153002/diff/20001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/TestNetworkQualityRttListener.java (right): https://codereview.chromium.org/2852153002/diff/20001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/TestNetworkQualityRttListener.java:18: // Signals when the first RTT observation at the URL request layer is received.. On 2017/05/02 20:27:11, pauljensen wrote: > nit: extra period at the end Done. https://codereview.chromium.org/2852153002/diff/20001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/TestNetworkQualityThroughputListener.java (right): https://codereview.chromium.org/2852153002/diff/20001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/TestNetworkQualityThroughputListener.java:17: // Signals when the first throughput observation is received.. On 2017/05/02 20:27:11, pauljensen wrote: > ditto Done.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pauljensen@chromium.org Link to the patchset: https://codereview.chromium.org/2852153002/#ps40001 (title: "pauljensen comments")
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": 40001, "attempt_start_ts": 1493758679515970,
"parent_rev": "9d3c221bdef03b02ef3ed0fe150433592d960f19", "commit_rev":
"aa1e3734c5987164b83cf7f58bd6c8923a77e506"}
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493758679515970,
"parent_rev": "fb4de5523fdbcc876f7abb4df92f106c7a1a70e1", "commit_rev":
"b81e1a532d7bdfab27546d5fa6f6d4e664fc494b"}
Message was sent while issue was closed.
Description was changed from ========== Fix flaky NQE QUIC test Block the test until RTT value is received. This ensures that the test is blocked until the RTT listener actually receives the notification. BUG=716998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Fix flaky NQE QUIC test Block the test until RTT value is received. This ensures that the test is blocked until the RTT listener actually receives the notification. BUG=716998 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2852153002 Cr-Commit-Position: refs/heads/master@{#468775} Committed: https://chromium.googlesource.com/chromium/src/+/b81e1a532d7bdfab27546d5fa6f6... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/b81e1a532d7bdfab27546d5fa6f6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
