Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2357723002:
Add integration tests for QUIC (Closed)
DescriptionAdd integration tests for QUIC
Two tests have been added: First is a smoke test (only cares
about the end result -- if the page was loaded or not etc.),
second looks more closely to verify if QUIC was actually used
or not.
Also, removed the global --disable-quic flag, and instead
added it to the individual tests that needed it.
Verified that all benchmarks currently pass on Android.
BUG=343579
Committed: https://crrev.com/285e6f35b3b135cb2975a47afb59bed5087225d6
Cr-Commit-Position: refs/heads/master@{#420390}
Patch Set 1 : ps #
Total comments: 8
Patch Set 2 : Addressed comments #
Messages
Total messages: 28 (18 generated)
Description was changed from ========== tests BUG= ========== to ========== Add integration tests for QUIC BUG= ==========
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:80001) has been deleted
Description was changed from ========== Add integration tests for QUIC BUG= ========== to ========== Add integration tests for QUIC BUG=343579 ==========
Description was changed from ========== Add integration tests for QUIC BUG=343579 ========== to ========== Add integration tests for QUIC Two tests have been added: First is a smoke test (only cares about the end result -- if the page was loaded or not etc.), second looks more closely to verify if QUIC was actually used or not. BUG=343579 ==========
Description was changed from ========== Add integration tests for QUIC Two tests have been added: First is a smoke test (only cares about the end result -- if the page was loaded or not etc.), second looks more closely to verify if QUIC was actually used or not. BUG=343579 ========== to ========== Add integration tests for QUIC Two tests have been added: First is a smoke test (only cares about the end result -- if the page was loaded or not etc.), second looks more closely to verify if QUIC was actually used or not. BUG=343579 ==========
Description was changed from ========== Add integration tests for QUIC Two tests have been added: First is a smoke test (only cares about the end result -- if the page was loaded or not etc.), second looks more closely to verify if QUIC was actually used or not. BUG=343579 ========== to ========== Add integration tests for QUIC Two tests have been added: First is a smoke test (only cares about the end result -- if the page was loaded or not etc.), second looks more closely to verify if QUIC was actually used or not. Also, removed the global --disable-quic flag, and instead added it to the individual tests that needed it. Verified that all benchmarks currently pass on Android. BUG=343579 ==========
tbansal@chromium.org changed reviewers: + bustamante@chromium.org, ryansturm@chromium.org
ryansturm, bustamante: ptal. thanks.
I'm excited to see QUIC tests being added, a couple comments below. https://codereview.chromium.org/2357723002/diff/100001/tools/chrome_proxy/com... File tools/chrome_proxy/common/chrome_proxy_measurements.py (left): https://codereview.chromium.org/2357723002/diff/100001/tools/chrome_proxy/com... tools/chrome_proxy/common/chrome_proxy_measurements.py:84: options.AppendExtraBrowserArgs('--disable-quic') I think we need this for every test, since the quic experiment being enabled (which was happening occasionally) blocks header validation. For example, when an integration test runs on staging all tests will validate that the "1.1 Chrome-Compression-Proxy-Testing" via header appears in responses. https://codereview.chromium.org/2357723002/diff/100001/tools/chrome_proxy/int... File tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py (right): https://codereview.chromium.org/2357723002/diff/100001/tools/chrome_proxy/int... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:267: class ChromeProxyQuicSmoke(ChromeProxyBenchmark): Per the issue with header validation on quic, it's worth trying to run this against staging to see if it works. If not we can avoid adding a staging version of the test to the waterfall. ./run_benchmark --use-live-sites --extra-chrome-proxy-via-header='1.1 Chrome-Compression-Proxy-Testing' --extra-browser-args='--data-reduction-proxy-experiment=stg' --browser=android-chrome chrome_proxy_benchmark.quic.smoke
lgtm % comment about sleeping https://codereview.chromium.org/2357723002/diff/100001/tools/chrome_proxy/int... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/2357723002/diff/100001/tools/chrome_proxy/int... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:901: time.sleep(3) Can you add a comment on why this sleep is 3 and why it is necessary? 3 seconds is a large chunk of time for automated tests. Pingback uses an incremental 10 second time that checks every 1 second. I'm not sure if that approach is better for these tests or not though.
https://codereview.chromium.org/2357723002/diff/100001/tools/chrome_proxy/com... File tools/chrome_proxy/common/chrome_proxy_measurements.py (left): https://codereview.chromium.org/2357723002/diff/100001/tools/chrome_proxy/com... tools/chrome_proxy/common/chrome_proxy_measurements.py:84: options.AppendExtraBrowserArgs('--disable-quic') On 2016/09/21 18:50:12, bustamante wrote: > I think we need this for every test, since the quic experiment being enabled > (which was happening occasionally) blocks header validation. For example, when > an integration test runs on staging all tests will validate that the "1.1 > Chrome-Compression-Proxy-Testing" via header appears in responses. I think he adds the command individually to the tests that do header validation.
https://codereview.chromium.org/2357723002/diff/100001/tools/chrome_proxy/com... File tools/chrome_proxy/common/chrome_proxy_measurements.py (left): https://codereview.chromium.org/2357723002/diff/100001/tools/chrome_proxy/com... tools/chrome_proxy/common/chrome_proxy_measurements.py:84: options.AppendExtraBrowserArgs('--disable-quic') On 2016/09/21 18:56:24, Ryan Sturm wrote: > On 2016/09/21 18:50:12, bustamante wrote: > > I think we need this for every test, since the quic experiment being enabled > > (which was happening occasionally) blocks header validation. For example, > when > > an integration test runs on staging all tests will validate that the "1.1 > > Chrome-Compression-Proxy-Testing" via header appears in responses. > > I think he adds the command individually to the tests that do header validation. All tests can do header validation if the --extra-chrome-proxy-via-header flag is used. But also I don't think it's obvious that someone has to know to add a --disable-quic flag if they want to do header validation, so it's going to lead to more flaking down the road - like crbug.com/593331
Patchset #3 (id:140001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:160001) has been deleted
bustamante: ptal. thanks. https://codereview.chromium.org/2357723002/diff/100001/tools/chrome_proxy/com... File tools/chrome_proxy/common/chrome_proxy_measurements.py (left): https://codereview.chromium.org/2357723002/diff/100001/tools/chrome_proxy/com... tools/chrome_proxy/common/chrome_proxy_measurements.py:84: options.AppendExtraBrowserArgs('--disable-quic') On 2016/09/21 19:11:42, bustamante wrote: > On 2016/09/21 18:56:24, Ryan Sturm wrote: > > On 2016/09/21 18:50:12, bustamante wrote: > > > I think we need this for every test, since the quic experiment being enabled > > > (which was happening occasionally) blocks header validation. For example, > > when > > > an integration test runs on staging all tests will validate that the "1.1 > > > Chrome-Compression-Proxy-Testing" via header appears in responses. > > > > I think he adds the command individually to the tests that do header > validation. > > All tests can do header validation if the --extra-chrome-proxy-via-header flag > is used. Looking at AddResultsForExtraViaHeader function, it seems it only checks the response headers (and not the request headers). So, that should work correctly even if QUIC is in use, since usage of QUIC only makes the request headers difficult to look up. > > But also I don't think it's obvious that someone has to know to add a > --disable-quic flag if they want to do header validation, so it's going to lead > to more flaking down the road - like crbug.com/593331 For now, I have put this flag back. However, I have kept the --disable-quic in the individual tests that actually look at the request headers. That way we can easily remove the global --disable-quic flag in the future (if there is a need), without breaking any tests. https://codereview.chromium.org/2357723002/diff/100001/tools/chrome_proxy/int... File tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py (right): https://codereview.chromium.org/2357723002/diff/100001/tools/chrome_proxy/int... tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:267: class ChromeProxyQuicSmoke(ChromeProxyBenchmark): On 2016/09/21 18:50:13, bustamante wrote: > Per the issue with header validation on quic, it's worth trying to run this > against staging to see if it works. If not we can avoid adding a staging > version of the test to the waterfall. > > ./run_benchmark --use-live-sites --extra-chrome-proxy-via-header='1.1 > Chrome-Compression-Proxy-Testing' > --extra-browser-args='--data-reduction-proxy-experiment=stg' > --browser=android-chrome chrome_proxy_benchmark.quic.smoke > IIUC, --extra-chrome-proxy-via-header only affects the checks for the response headers. Using QuIC should only affect the visibility of request headers. So, I think adding --extra-chrome-proxy-via-header='1.1 > Chrome-Compression-Proxy-Testing' should be okay (I verified too). https://codereview.chromium.org/2357723002/diff/100001/tools/chrome_proxy/int... File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): https://codereview.chromium.org/2357723002/diff/100001/tools/chrome_proxy/int... tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:901: time.sleep(3) On 2016/09/21 18:54:39, Ryan Sturm wrote: > Can you add a comment on why this sleep is 3 and why it is necessary? 3 seconds > is a large chunk of time for automated tests. > > Pingback uses an incremental 10 second time that checks every 1 second. I'm not > sure if that approach is better for these tests or not though. Done.
On 2016/09/22 17:25:42, tbansal1 wrote: > bustamante: ptal. thanks. > > https://codereview.chromium.org/2357723002/diff/100001/tools/chrome_proxy/com... > File tools/chrome_proxy/common/chrome_proxy_measurements.py (left): > > https://codereview.chromium.org/2357723002/diff/100001/tools/chrome_proxy/com... > tools/chrome_proxy/common/chrome_proxy_measurements.py:84: > options.AppendExtraBrowserArgs('--disable-quic') > On 2016/09/21 19:11:42, bustamante wrote: > > On 2016/09/21 18:56:24, Ryan Sturm wrote: > > > On 2016/09/21 18:50:12, bustamante wrote: > > > > I think we need this for every test, since the quic experiment being > enabled > > > > (which was happening occasionally) blocks header validation. For example, > > > when > > > > an integration test runs on staging all tests will validate that the "1.1 > > > > Chrome-Compression-Proxy-Testing" via header appears in responses. > > > > > > I think he adds the command individually to the tests that do header > > validation. > > > > All tests can do header validation if the --extra-chrome-proxy-via-header flag > > is used. > Looking at AddResultsForExtraViaHeader function, it seems it only checks the > response headers (and not the request headers). So, that should work correctly > even if QUIC is in use, since usage of QUIC only makes the request headers > difficult to look up. > > > > But also I don't think it's obvious that someone has to know to add a > > --disable-quic flag if they want to do header validation, so it's going to > lead > > to more flaking down the road - like crbug.com/593331 > For now, I have put this flag back. However, I have kept the --disable-quic in > the individual tests that actually look at the request headers. That way we can > easily remove the global --disable-quic flag in the future (if there is a need), > without breaking any tests. > > https://codereview.chromium.org/2357723002/diff/100001/tools/chrome_proxy/int... > File tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py (right): > > https://codereview.chromium.org/2357723002/diff/100001/tools/chrome_proxy/int... > tools/chrome_proxy/integration_tests/chrome_proxy_benchmark.py:267: class > ChromeProxyQuicSmoke(ChromeProxyBenchmark): > On 2016/09/21 18:50:13, bustamante wrote: > > Per the issue with header validation on quic, it's worth trying to run this > > against staging to see if it works. If not we can avoid adding a staging > > version of the test to the waterfall. > > > > ./run_benchmark --use-live-sites --extra-chrome-proxy-via-header='1.1 > > Chrome-Compression-Proxy-Testing' > > --extra-browser-args='--data-reduction-proxy-experiment=stg' > > --browser=android-chrome chrome_proxy_benchmark.quic.smoke > > > IIUC, --extra-chrome-proxy-via-header only affects the checks for the response > headers. > Using QuIC should only affect the visibility of request headers. > So, I think adding --extra-chrome-proxy-via-header='1.1 > > Chrome-Compression-Proxy-Testing' should be okay (I verified too). > > https://codereview.chromium.org/2357723002/diff/100001/tools/chrome_proxy/int... > File tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py (right): > > https://codereview.chromium.org/2357723002/diff/100001/tools/chrome_proxy/int... > tools/chrome_proxy/integration_tests/chrome_proxy_metrics.py:901: time.sleep(3) > On 2016/09/21 18:54:39, Ryan Sturm wrote: > > Can you add a comment on why this sleep is 3 and why it is necessary? 3 > seconds > > is a large chunk of time for automated tests. > > > > Pingback uses an incremental 10 second time that checks every 1 second. I'm > not > > sure if that approach is better for these tests or not though. > > Done. lgtm - sounds good, thanks again for adding the test.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ryansturm@chromium.org Link to the patchset: https://codereview.chromium.org/2357723002/#ps180001 (title: "Addressed comments")
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 ========== Add integration tests for QUIC Two tests have been added: First is a smoke test (only cares about the end result -- if the page was loaded or not etc.), second looks more closely to verify if QUIC was actually used or not. Also, removed the global --disable-quic flag, and instead added it to the individual tests that needed it. Verified that all benchmarks currently pass on Android. BUG=343579 ========== to ========== Add integration tests for QUIC Two tests have been added: First is a smoke test (only cares about the end result -- if the page was loaded or not etc.), second looks more closely to verify if QUIC was actually used or not. Also, removed the global --disable-quic flag, and instead added it to the individual tests that needed it. Verified that all benchmarks currently pass on Android. BUG=343579 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add integration tests for QUIC Two tests have been added: First is a smoke test (only cares about the end result -- if the page was loaded or not etc.), second looks more closely to verify if QUIC was actually used or not. Also, removed the global --disable-quic flag, and instead added it to the individual tests that needed it. Verified that all benchmarks currently pass on Android. BUG=343579 ========== to ========== Add integration tests for QUIC Two tests have been added: First is a smoke test (only cares about the end result -- if the page was loaded or not etc.), second looks more closely to verify if QUIC was actually used or not. Also, removed the global --disable-quic flag, and instead added it to the individual tests that needed it. Verified that all benchmarks currently pass on Android. BUG=343579 Committed: https://crrev.com/285e6f35b3b135cb2975a47afb59bed5087225d6 Cr-Commit-Position: refs/heads/master@{#420390} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/285e6f35b3b135cb2975a47afb59bed5087225d6 Cr-Commit-Position: refs/heads/master@{#420390} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
