Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2705413004:
Implement the Lo-Fi cache related integration tests with ChromeDriver (Closed)
DescriptionImplement the Lo-Fi cache related integration tests with ChromeDriver
Implement both the LoFiCacheDisabled and LoFiProxyDisabled integration
tests in one testLoFiCacheBypass ChromeDriver test. Over half of the
cache tests are identical code for loading the resources into cache and
the existing naming scheme is confusing, so test that turning either
Lo-Fi or the proxy off and refetching the page does not serve cached
transformed resources.
BUG=680569, 680568
Review-Url: https://codereview.chromium.org/2705413004
Cr-Commit-Position: refs/heads/master@{#454037}
Committed: https://chromium.googlesource.com/chromium/src/+/d4337ecc7263a8707a582b006c99ede5fff76ea2
Patch Set 1 #
Total comments: 6
Patch Set 2 : use assertIn #Patch Set 3 : add comment for disabling quic #Patch Set 4 : remove disable-quic #
Depends on Patchset: Messages
Total messages: 25 (8 generated)
Patchset #1 (id:1) has been deleted
megjablon@chromium.org changed reviewers: + ryansturm@chromium.org
PTAL, thanks!
robertogden@chromium.org changed reviewers: + robertogden@chromium.org
lgtm https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/common.py (right): https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/common.py:583: self.assertTrue(int(content_length) > 100) Nit: Is this always true? Asking mostly out of ignorance but I wonder if there is a picture of < 100 bytes.
https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/common.py (right): https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/common.py:583: self.assertTrue(int(content_length) > 100) On 2017/02/23 23:45:09, Robert Ogden wrote: > Nit: Is this always true? Asking mostly out of ignorance but I wonder if there > is a picture of < 100 bytes. For our test pages it will always be true, but if we change them then we'll have to adjust this.
tbansal@chromium.org changed reviewers: + tbansal@chromium.org
https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/lofi.py (right): https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/lofi.py:87: test_driver.AddChromeArg('--disable-quic') rm disable-quic.
https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/lofi.py (right): https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/lofi.py:87: test_driver.AddChromeArg('--disable-quic') On 2017/02/23 23:55:49, tbansal1 wrote: > rm disable-quic. If I remove this from either this or testLoFi they fail (but removing it is ok for the Lite Page test). Do you know why?
https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/lofi.py (right): https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/lofi.py:87: test_driver.AddChromeArg('--disable-quic') On 2017/02/24 00:07:14, megjablon wrote: > On 2017/02/23 23:55:49, tbansal1 wrote: > > rm disable-quic. > > If I remove this from either this or testLoFi they fail (but removing it is ok > for the Lite Page test). Do you know why? No, I do not know at the top of my head. Do you know where it fails?
https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/lofi.py (right): https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/lofi.py:87: test_driver.AddChromeArg('--disable-quic') On 2017/02/24 00:09:30, tbansal1 wrote: > On 2017/02/24 00:07:14, megjablon wrote: > > On 2017/02/23 23:55:49, tbansal1 wrote: > > > rm disable-quic. > > > > If I remove this from either this or testLoFi they fail (but removing it is ok > > for the Lite Page test). Do you know why? > > No, I do not know at the top of my head. Do you know where it fails? It looks like when quic is enabled for Lo-Fi requests there aren't request headers using the test framework's response.request_headers
On 2017/02/24 00:28:53, megjablon wrote: > https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... > File tools/chrome_proxy/webdriver/lofi.py (right): > > https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... > tools/chrome_proxy/webdriver/lofi.py:87: > test_driver.AddChromeArg('--disable-quic') > On 2017/02/24 00:09:30, tbansal1 wrote: > > On 2017/02/24 00:07:14, megjablon wrote: > > > On 2017/02/23 23:55:49, tbansal1 wrote: > > > > rm disable-quic. > > > > > > If I remove this from either this or testLoFi they fail (but removing it is > ok > > > for the Lite Page test). Do you know why? > > > > No, I do not know at the top of my head. Do you know where it fails? > It looks like when quic is enabled for Lo-Fi requests there aren't request > headers using the test framework's response.request_headers It is interesting that the request headers are visible in the other Lite page test. Can we figure out what's the difference here? Thanks. Since QUIC is now enabled 100% on stable channel, we should try our best to run our integration tests with QUIC enabled.
On 2017/02/24 18:52:29, tbansal1 wrote: > On 2017/02/24 00:28:53, megjablon wrote: > > > https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... > > File tools/chrome_proxy/webdriver/lofi.py (right): > > > > > https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... > > tools/chrome_proxy/webdriver/lofi.py:87: > > test_driver.AddChromeArg('--disable-quic') > > On 2017/02/24 00:09:30, tbansal1 wrote: > > > On 2017/02/24 00:07:14, megjablon wrote: > > > > On 2017/02/23 23:55:49, tbansal1 wrote: > > > > > rm disable-quic. > > > > > > > > If I remove this from either this or testLoFi they fail (but removing it > is > > ok > > > > for the Lite Page test). Do you know why? > > > > > > No, I do not know at the top of my head. Do you know where it fails? > > It looks like when quic is enabled for Lo-Fi requests there aren't request > > headers using the test framework's response.request_headers > > It is interesting that the request headers are visible in the other Lite > page test. Can we figure out what's the difference here? Thanks. > Since QUIC is now enabled 100% on stable channel, we should try our best > to run our integration tests with QUIC enabled. Found the difference. When QUIC is on we have the request headers for the main frame, but not subresources. On the lite page test, we're only testing the directive on the main frame, but for Lo-Fi we're looking for the directive on the subresources.
On 2017/02/24 20:19:31, megjablon wrote: > On 2017/02/24 18:52:29, tbansal1 wrote: > > On 2017/02/24 00:28:53, megjablon wrote: > > > > > > https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... > > > File tools/chrome_proxy/webdriver/lofi.py (right): > > > > > > > > > https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... > > > tools/chrome_proxy/webdriver/lofi.py:87: > > > test_driver.AddChromeArg('--disable-quic') > > > On 2017/02/24 00:09:30, tbansal1 wrote: > > > > On 2017/02/24 00:07:14, megjablon wrote: > > > > > On 2017/02/23 23:55:49, tbansal1 wrote: > > > > > > rm disable-quic. > > > > > > > > > > If I remove this from either this or testLoFi they fail (but removing it > > is > > > ok > > > > > for the Lite Page test). Do you know why? > > > > > > > > No, I do not know at the top of my head. Do you know where it fails? > > > It looks like when quic is enabled for Lo-Fi requests there aren't request > > > headers using the test framework's response.request_headers > > > > It is interesting that the request headers are visible in the other Lite > > page test. Can we figure out what's the difference here? Thanks. > > Since QUIC is now enabled 100% on stable channel, we should try our best > > to run our integration tests with QUIC enabled. > > Found the difference. When QUIC is on we have the request headers for the main > frame, but not subresources. On the lite page test, we're only testing the > directive on the main frame, but for Lo-Fi we're looking for the directive on > the subresources. Thanks for investigating. Can you add that as a comment.
On 2017/02/24 21:17:05, tbansal1 wrote: > On 2017/02/24 20:19:31, megjablon wrote: > > On 2017/02/24 18:52:29, tbansal1 wrote: > > > On 2017/02/24 00:28:53, megjablon wrote: > > > > > > > > > > https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... > > > > File tools/chrome_proxy/webdriver/lofi.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... > > > > tools/chrome_proxy/webdriver/lofi.py:87: > > > > test_driver.AddChromeArg('--disable-quic') > > > > On 2017/02/24 00:09:30, tbansal1 wrote: > > > > > On 2017/02/24 00:07:14, megjablon wrote: > > > > > > On 2017/02/23 23:55:49, tbansal1 wrote: > > > > > > > rm disable-quic. > > > > > > > > > > > > If I remove this from either this or testLoFi they fail (but removing > it > > > is > > > > ok > > > > > > for the Lite Page test). Do you know why? > > > > > > > > > > No, I do not know at the top of my head. Do you know where it fails? > > > > It looks like when quic is enabled for Lo-Fi requests there aren't request > > > > headers using the test framework's response.request_headers > > > > > > It is interesting that the request headers are visible in the other Lite > > > page test. Can we figure out what's the difference here? Thanks. > > > Since QUIC is now enabled 100% on stable channel, we should try our best > > > to run our integration tests with QUIC enabled. > > > > Found the difference. When QUIC is on we have the request headers for the main > > frame, but not subresources. On the lite page test, we're only testing the > > directive on the main frame, but for Lo-Fi we're looking for the directive on > > the subresources. > > Thanks for investigating. Can you add that as a comment. I wrote a patch to fix this issue, can you hold off on landing it until I investigate some more.
On 2017/02/24 21:17:05, tbansal1 wrote: > On 2017/02/24 20:19:31, megjablon wrote: > > On 2017/02/24 18:52:29, tbansal1 wrote: > > > On 2017/02/24 00:28:53, megjablon wrote: > > > > > > > > > > https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... > > > > File tools/chrome_proxy/webdriver/lofi.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... > > > > tools/chrome_proxy/webdriver/lofi.py:87: > > > > test_driver.AddChromeArg('--disable-quic') > > > > On 2017/02/24 00:09:30, tbansal1 wrote: > > > > > On 2017/02/24 00:07:14, megjablon wrote: > > > > > > On 2017/02/23 23:55:49, tbansal1 wrote: > > > > > > > rm disable-quic. > > > > > > > > > > > > If I remove this from either this or testLoFi they fail (but removing > it > > > is > > > > ok > > > > > > for the Lite Page test). Do you know why? > > > > > > > > > > No, I do not know at the top of my head. Do you know where it fails? > > > > It looks like when quic is enabled for Lo-Fi requests there aren't request > > > > headers using the test framework's response.request_headers > > > > > > It is interesting that the request headers are visible in the other Lite > > > page test. Can we figure out what's the difference here? Thanks. > > > Since QUIC is now enabled 100% on stable channel, we should try our best > > > to run our integration tests with QUIC enabled. > > > > Found the difference. When QUIC is on we have the request headers for the main > > frame, but not subresources. On the lite page test, we're only testing the > > directive on the main frame, but for Lo-Fi we're looking for the directive on > > the subresources. > > Thanks for investigating. Can you add that as a comment. Done!
On 2017/02/24 21:35:22, megjablon wrote: > On 2017/02/24 21:17:05, tbansal1 wrote: > > On 2017/02/24 20:19:31, megjablon wrote: > > > On 2017/02/24 18:52:29, tbansal1 wrote: > > > > On 2017/02/24 00:28:53, megjablon wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... > > > > > File tools/chrome_proxy/webdriver/lofi.py (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... > > > > > tools/chrome_proxy/webdriver/lofi.py:87: > > > > > test_driver.AddChromeArg('--disable-quic') > > > > > On 2017/02/24 00:09:30, tbansal1 wrote: > > > > > > On 2017/02/24 00:07:14, megjablon wrote: > > > > > > > On 2017/02/23 23:55:49, tbansal1 wrote: > > > > > > > > rm disable-quic. > > > > > > > > > > > > > > If I remove this from either this or testLoFi they fail (but > removing > > it > > > > is > > > > > ok > > > > > > > for the Lite Page test). Do you know why? > > > > > > > > > > > > No, I do not know at the top of my head. Do you know where it fails? > > > > > It looks like when quic is enabled for Lo-Fi requests there aren't > request > > > > > headers using the test framework's response.request_headers > > > > > > > > It is interesting that the request headers are visible in the other Lite > > > > page test. Can we figure out what's the difference here? Thanks. > > > > Since QUIC is now enabled 100% on stable channel, we should try our best > > > > to run our integration tests with QUIC enabled. > > > > > > Found the difference. When QUIC is on we have the request headers for the > main > > > frame, but not subresources. On the lite page test, we're only testing the > > > directive on the main frame, but for Lo-Fi we're looking for the directive > on > > > the subresources. > > > > Thanks for investigating. Can you add that as a comment. > > Done! Ryan has a fix for this https://codereview.chromium.org/2710193007/
On 2017/02/24 22:22:16, megjablon wrote: > On 2017/02/24 21:35:22, megjablon wrote: > > On 2017/02/24 21:17:05, tbansal1 wrote: > > > On 2017/02/24 20:19:31, megjablon wrote: > > > > On 2017/02/24 18:52:29, tbansal1 wrote: > > > > > On 2017/02/24 00:28:53, megjablon wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... > > > > > > File tools/chrome_proxy/webdriver/lofi.py (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2705413004/diff/20001/tools/chrome_proxy/webd... > > > > > > tools/chrome_proxy/webdriver/lofi.py:87: > > > > > > test_driver.AddChromeArg('--disable-quic') > > > > > > On 2017/02/24 00:09:30, tbansal1 wrote: > > > > > > > On 2017/02/24 00:07:14, megjablon wrote: > > > > > > > > On 2017/02/23 23:55:49, tbansal1 wrote: > > > > > > > > > rm disable-quic. > > > > > > > > > > > > > > > > If I remove this from either this or testLoFi they fail (but > > removing > > > it > > > > > is > > > > > > ok > > > > > > > > for the Lite Page test). Do you know why? > > > > > > > > > > > > > > No, I do not know at the top of my head. Do you know where it fails? > > > > > > It looks like when quic is enabled for Lo-Fi requests there aren't > > request > > > > > > headers using the test framework's response.request_headers > > > > > > > > > > It is interesting that the request headers are visible in the other Lite > > > > > page test. Can we figure out what's the difference here? Thanks. > > > > > Since QUIC is now enabled 100% on stable channel, we should try our best > > > > > to run our integration tests with QUIC enabled. > > > > > > > > Found the difference. When QUIC is on we have the request headers for the > > main > > > > frame, but not subresources. On the lite page test, we're only testing the > > > > directive on the main frame, but for Lo-Fi we're looking for the directive > > on > > > > the subresources. > > > > > > Thanks for investigating. Can you add that as a comment. > > > > Done! > > Ryan has a fix for this https://codereview.chromium.org/2710193007/ Ping. Can you take a look at this Ryan?
I was just looking when you pinged me :) lgtm, thanks for waiting for the fix to net log!
The CQ bit was checked by megjablon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robertogden@chromium.org Link to the patchset: https://codereview.chromium.org/2705413004/#ps80001 (title: "remove disable-quic")
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": 80001, "attempt_start_ts": 1488402645877710,
"parent_rev": "783059297a429d72f45dc835a738233139d822f7", "commit_rev":
"d4337ecc7263a8707a582b006c99ede5fff76ea2"}
Message was sent while issue was closed.
Description was changed from ========== Implement the Lo-Fi cache related integration tests with ChromeDriver Implement both the LoFiCacheDisabled and LoFiProxyDisabled integration tests in one testLoFiCacheBypass ChromeDriver test. Over half of the cache tests are identical code for loading the resources into cache and the existing naming scheme is confusing, so test that turning either Lo-Fi or the proxy off and refetching the page does not serve cached transformed resources. BUG=680569, 680568 ========== to ========== Implement the Lo-Fi cache related integration tests with ChromeDriver Implement both the LoFiCacheDisabled and LoFiProxyDisabled integration tests in one testLoFiCacheBypass ChromeDriver test. Over half of the cache tests are identical code for loading the resources into cache and the existing naming scheme is confusing, so test that turning either Lo-Fi or the proxy off and refetching the page does not serve cached transformed resources. BUG=680569, 680568 Review-Url: https://codereview.chromium.org/2705413004 Cr-Commit-Position: refs/heads/master@{#454037} Committed: https://chromium.googlesource.com/chromium/src/+/d4337ecc7263a8707a582b006c99... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/d4337ecc7263a8707a582b006c99... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
