Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2713003003:
Adding a smoke test for DataSaver (Closed)
DescriptionAdding a smoke test to verify DataSaver works in the base case.
BUG=680578
Review-Url: https://codereview.chromium.org/2713003003
Cr-Commit-Position: refs/heads/master@{#456237}
Committed: https://chromium.googlesource.com/chromium/src/+/f6bcc26091efb4ee4cb6bf3d228adcb09d530eb8
Patch Set 1 #Patch Set 2 : Add one more white space #
Total comments: 1
Patch Set 3 : Addressing Richard's code review comment #
Total comments: 1
Patch Set 4 : Addressing Ryan's code review comment #Patch Set 5 : Merge with master branch #Messages
Total messages: 24 (13 generated)
bustamante@chromium.org changed reviewers: + bustamante@chromium.org
Thanks for the test case! It looks good overall, in the CL description can you give a brief summary of what the test does? Like: Adding a smoke test to verify Flywheel works in the base case. Also I added one comment for the code. https://codereview.chromium.org/2713003003/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/smoke.py (right): https://codereview.chromium.org/2713003003/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/smoke.py:39: for response in t.GetHTTPResponses(): Can you also indent this line and the next two more spaces, so it's under "with TestDriver() as t:"
Description was changed from ========== For bug 680578 BUG=680578 ========== to ========== For bug 680578 BUG=680578 Adding a smoke test to verify Flywheel works in the base case. ==========
Description was changed from ========== For bug 680578 BUG=680578 Adding a smoke test to verify Flywheel works in the base case. ========== to ========== For bug 680578 BUG=680578 Adding a smoke test to verify Flywheel works in the base case. ==========
Description was changed from ========== For bug 680578 BUG=680578 Adding a smoke test to verify Flywheel works in the base case. ========== to ========== For bug 680578 BUG=680578 Adding a smoke test to verify Flywheel works in the base case. ==========
On 2017/02/23 23:24:32, bustamante wrote: > Thanks for the test case! It looks good overall, in the CL description can you > give a brief summary of what the test does? Like: > > Adding a smoke test to verify Flywheel works in the base case. > > Also I added one comment for the code. > > https://codereview.chromium.org/2713003003/diff/20001/tools/chrome_proxy/webd... > File tools/chrome_proxy/webdriver/smoke.py (right): > > https://codereview.chromium.org/2713003003/diff/20001/tools/chrome_proxy/webd... > tools/chrome_proxy/webdriver/smoke.py:39: for response in t.GetHTTPResponses(): > Can you also indent this line and the next two more spaces, so it's under "with > TestDriver() as t:" Thank you so much for your comments. I have followed your advice to add description to this CL and better format the codes. Thanks again.
bustamante@chromium.org changed reviewers: + robertogden@chromium.org, ryansturm@chromium.org
Description was changed from ========== For bug 680578 BUG=680578 Adding a smoke test to verify Flywheel works in the base case. ========== to ========== Adding a smoke test to verify Flywheel works in the base case. BUG=680578 ==========
+Robert/Ryan Can you take a look? From a while ago this is simple smoke test added by my TVC working on Paquete who was interested in working on this. It covers the happy path test from the smoke tests in 680578.
Usually, the CL title would be more descriptive than "For bug 680578". Just the top level summary of adding a smoke test for DataSaver. Please remove "Flywheel" from the description as it should be referred to as "DataSaver" in public CLs and bugs. https://codereview.chromium.org/2713003003/diff/40001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/smoke.py (right): https://codereview.chromium.org/2713003003/diff/40001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/smoke.py:39: for response in t.GetHTTPResponses(): Can you check that GetHTTPResonses() is non empty? responses = t.GetHTTPResponses() self.assertNotEqual(0, len(responses)) for response in responses:
Description was changed from ========== Adding a smoke test to verify Flywheel works in the base case. BUG=680578 ========== to ========== Adding a smoke test to verify DataSaver works in the base case. BUG=680578 ==========
On 2017/03/10 19:00:38, Ryan Sturm wrote: > Usually, the CL title would be more descriptive than "For bug 680578". Just the > top level summary of adding a smoke test for DataSaver. > > Please remove "Flywheel" from the description as it should be referred to as > "DataSaver" in public CLs and bugs. > > https://codereview.chromium.org/2713003003/diff/40001/tools/chrome_proxy/webd... > File tools/chrome_proxy/webdriver/smoke.py (right): > > https://codereview.chromium.org/2713003003/diff/40001/tools/chrome_proxy/webd... > tools/chrome_proxy/webdriver/smoke.py:39: for response in t.GetHTTPResponses(): > Can you check that GetHTTPResonses() is non empty? > > responses = t.GetHTTPResponses() > self.assertNotEqual(0, len(responses)) > for response in responses: Thank you so much for your comments. I have followed your advice and updated CL title, description and codes accordingly.
lgtm
lgtm
The CQ bit was checked by jiewu@chromium.org
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
Failed to apply patch for tools/chrome_proxy/webdriver/smoke.py:
While running git apply --index -p1;
<stdin>:10: trailing whitespace.
error: patch failed: tools/chrome_proxy/webdriver/smoke.py:30
error: tools/chrome_proxy/webdriver/smoke.py: patch does not apply
Patch: tools/chrome_proxy/webdriver/smoke.py
Index: tools/chrome_proxy/webdriver/smoke.py
diff --git a/tools/chrome_proxy/webdriver/smoke.py
b/tools/chrome_proxy/webdriver/smoke.py
index
b54dd7fc22424559aff65f2649f7890f10a07180..87568de9f90d223219d4ce56b91259ef1db18552
100644
--- a/tools/chrome_proxy/webdriver/smoke.py
+++ b/tools/chrome_proxy/webdriver/smoke.py
@@ -30,6 +30,16 @@ class Smoke(IntegrationTest):
self.assertEqual(2, len(responses))
for response in responses:
self.assertHasChromeProxyViaHeader(response)
+
+ # Ensure Chrome uses DataSaver in normal mode.
+ def testCheckPageWithNormalMode(self):
+ with TestDriver() as t:
+ t.AddChromeArg('--enable-spdy-proxy-auth')
+ t.LoadURL('http://check.googlezip.net/test.html')
+ responses = t.GetHTTPResponses()
+ self.assertNotEqual(0, len(responses))
+ for response in responses:
+ self.assertHasChromeProxyViaHeader(response)
if __name__ == '__main__':
IntegrationTest.RunAllTests()
The CQ bit was checked by jiewu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robertogden@chromium.org, ryansturm@chromium.org Link to the patchset: https://codereview.chromium.org/2713003003/#ps80001 (title: "Merge with master branch")
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": 1489192872166170,
"parent_rev": "52a6b029471456658415ca04e0b69dfce232bed0", "commit_rev":
"f6bcc26091efb4ee4cb6bf3d228adcb09d530eb8"}
Message was sent while issue was closed.
Description was changed from ========== Adding a smoke test to verify DataSaver works in the base case. BUG=680578 ========== to ========== Adding a smoke test to verify DataSaver works in the base case. BUG=680578 Review-Url: https://codereview.chromium.org/2713003003 Cr-Commit-Position: refs/heads/master@{#456237} Committed: https://chromium.googlesource.com/chromium/src/+/f6bcc26091efb4ee4cb6bf3d228a... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/f6bcc26091efb4ee4cb6bf3d228a... |
