Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2528483004:
Adds skeleton of new integration test framework. (Closed)
|
Created:
4 years ago by Robert Ogden Modified:
4 years ago CC:
chromium-reviews, tbansal+watch-data-reduction-proxy_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds skeleton of new integration test framework.
Includes a working test case and basic functionality. Much more to come.
BUG=669956
Committed: https://crrev.com/c2a3d73bf201088d52b9e1226f8277d8f011163c
Cr-Commit-Position: refs/heads/master@{#435047}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Fix nits. #
Total comments: 6
Messages
Total messages: 23 (11 generated)
robertogden@chromium.org changed reviewers: + bustamante@chromium.org
A wild integration test framework appeared!
Description was changed from ========== Adds skeleton of new integration test framework. Includes a working test case and basic functionality. Much more to come. BUG= ========== to ========== Adds skeleton of new integration test framework. Includes a working test case and basic functionality. Much more to come. BUG= ==========
robertogden@chromium.org changed reviewers: - bustamante@chromium.org
robertogden@chromium.org changed reviewers: + sclittle@chromium.org
robertogden@chromium.org changed reviewers: + tbansal@chromium.org - sclittle@chromium.org
Third reviewer is the charm. (One of many CLs on this framework to come)
few nits https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/common.py (right): https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:65: self._args = {} Can this be renamed to chrome_args? https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:116: def AddChromeArg(self, arg): Is this being called somewhere? https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:135: def RemoveChromeArg(self, arg): Is this being called somewhere? https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:206: HandleException(e) Pass the name of the test in the HandleException so it can be printed on the screen? https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/simple_smoke.py (right): https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/simple_smoke.py:15: t.AddChromeArgs(['--enable-spdy-proxy-auth', '--disable-quic']) I do not think we need "disable-quic" in the new setup. The older telemetry setup had some problems when QUIC was in use. https://bugs.chromium.org/p/chromium/issues/detail?id=602695 The new setup hopefully does not have similar problems.
https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/common.py (right): https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:65: self._args = {} On 2016/11/29 18:38:53, tbansal1 wrote: > Can this be renamed to chrome_args? Done. https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:116: def AddChromeArg(self, arg): On 2016/11/29 18:38:53, tbansal1 wrote: > Is this being called somewhere? Not yet, but I think it is an important method to expose for convenience for tests that need to change a single argument. https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:135: def RemoveChromeArg(self, arg): On 2016/11/29 18:38:53, tbansal1 wrote: > Is this being called somewhere? Same as above. https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:206: HandleException(e) On 2016/11/29 18:38:53, tbansal1 wrote: > Pass the name of the test in the HandleException so it can be printed on the > screen? Done. https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/simple_smoke.py (right): https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/simple_smoke.py:15: t.AddChromeArgs(['--enable-spdy-proxy-auth', '--disable-quic']) On 2016/11/29 18:38:53, tbansal1 wrote: > I do not think we need "disable-quic" in the new setup. The older telemetry > setup had some problems when QUIC was in use. > https://bugs.chromium.org/p/chromium/issues/detail?id=602695 > > The new setup hopefully does not have similar problems. Done.
lgtm. Thanks for doing this.
The CQ bit was checked by robertogden@chromium.org
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": 20001, "attempt_start_ts": 1480445748692970,
"parent_rev": "b7ad752afd0651e26ac0011bfaac56c2e8e5314f", "commit_rev":
"209cc37c498564330f443ba96fec30a3cc435a6c"}
Message was sent while issue was closed.
Description was changed from ========== Adds skeleton of new integration test framework. Includes a working test case and basic functionality. Much more to come. BUG= ========== to ========== Adds skeleton of new integration test framework. Includes a working test case and basic functionality. Much more to come. BUG= ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Adds skeleton of new integration test framework. Includes a working test case and basic functionality. Much more to come. BUG= ========== to ========== Adds skeleton of new integration test framework. Includes a working test case and basic functionality. Much more to come. BUG= Committed: https://crrev.com/c2a3d73bf201088d52b9e1226f8277d8f011163c Cr-Commit-Position: refs/heads/master@{#435047} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c2a3d73bf201088d52b9e1226f8277d8f011163c Cr-Commit-Position: refs/heads/master@{#435047}
Message was sent while issue was closed.
ryansturm@chromium.org changed reviewers: + ryansturm@chromium.org
Message was sent while issue was closed.
Adding mostly style nits. https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/common.py (right): https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:29: parser.add_argument('--via_header_matches', metavar='via_header', nargs=1, nit: s/via_header_matchers/via_header_value/ https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:33: 'the Chrome or Chromium executable') Can you add that if no chrome_exec is supplied, the default system chrome will be used. Either as a comment below where it is set on capabilites or as part of the help message. https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:80: for a in shlex.split(self._flags.browser_args): nit: s/a/arg/ https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:87: opts = Options() nit: s/opts/options/ https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:88: for a in self._args: nit: s/a/arg/ https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:90: caps = {'loggingPrefs': {'performance': 'INFO'}} nit: s/caps/capabilities/ https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:111: if not self._args: can self._args ever not be a value after __init__? https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:206: HandleException(e) On 2016/11/29 18:53:54, Robert Ogden wrote: > On 2016/11/29 18:38:53, tbansal1 wrote: > > Pass the name of the test in the HandleException so it can be printed on the > > screen? > > Done. I think tbansal meant pass method and the exception. https://codereview.chromium.org/2528483004/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/common.py (right): https://codereview.chromium.org/2528483004/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/common.py:67: self.chrome_args = {} can this be a set instead of a dict? I don't see much value in setting the key pair to value : true when you can just add value instead. https://codereview.chromium.org/2528483004/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/common.py:188: return self._driver.execute_script("return " + script) What happens if LoadPage was not called (i.e., _StartDriver was just called)? https://codereview.chromium.org/2528483004/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/simple_smoke.py (right): https://codereview.chromium.org/2528483004/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/simple_smoke.py:19: time.sleep(5) Why is the sleep(5) necessary?
Message was sent while issue was closed.
On 2016/11/29 19:31:46, Ryan Sturm wrote: > Adding mostly style nits. > > https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... > File tools/chrome_proxy/webdriver/common.py (right): > > https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... > tools/chrome_proxy/webdriver/common.py:29: > parser.add_argument('--via_header_matches', metavar='via_header', nargs=1, > nit: s/via_header_matchers/via_header_value/ > > https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... > tools/chrome_proxy/webdriver/common.py:33: 'the Chrome or Chromium executable') > Can you add that if no chrome_exec is supplied, the default system chrome will > be used. Either as a comment below where it is set on capabilites or as part of > the help message. > > https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... > tools/chrome_proxy/webdriver/common.py:80: for a in > shlex.split(self._flags.browser_args): > nit: s/a/arg/ > > https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... > tools/chrome_proxy/webdriver/common.py:87: opts = Options() > nit: s/opts/options/ > > https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... > tools/chrome_proxy/webdriver/common.py:88: for a in self._args: > nit: s/a/arg/ > > https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... > tools/chrome_proxy/webdriver/common.py:90: caps = {'loggingPrefs': > {'performance': 'INFO'}} > nit: s/caps/capabilities/ > > https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... > tools/chrome_proxy/webdriver/common.py:111: if not self._args: > can self._args ever not be a value after __init__? > > https://codereview.chromium.org/2528483004/diff/1/tools/chrome_proxy/webdrive... > tools/chrome_proxy/webdriver/common.py:206: HandleException(e) > On 2016/11/29 18:53:54, Robert Ogden wrote: > > On 2016/11/29 18:38:53, tbansal1 wrote: > > > Pass the name of the test in the HandleException so it can be printed on the > > > screen? > > > > Done. > > I think tbansal meant pass method and the exception. > > https://codereview.chromium.org/2528483004/diff/20001/tools/chrome_proxy/webd... > File tools/chrome_proxy/webdriver/common.py (right): > > https://codereview.chromium.org/2528483004/diff/20001/tools/chrome_proxy/webd... > tools/chrome_proxy/webdriver/common.py:67: self.chrome_args = {} > can this be a set instead of a dict? I don't see much value in setting the key > pair to value : true when you can just add value instead. > > https://codereview.chromium.org/2528483004/diff/20001/tools/chrome_proxy/webd... > tools/chrome_proxy/webdriver/common.py:188: return > self._driver.execute_script("return " + script) > What happens if LoadPage was not called (i.e., _StartDriver was just called)? > > https://codereview.chromium.org/2528483004/diff/20001/tools/chrome_proxy/webd... > File tools/chrome_proxy/webdriver/simple_smoke.py (right): > > https://codereview.chromium.org/2528483004/diff/20001/tools/chrome_proxy/webd... > tools/chrome_proxy/webdriver/simple_smoke.py:19: time.sleep(5) > Why is the sleep(5) necessary? Just saw you submitted. Fix them later.
Message was sent while issue was closed.
Changes in the work now, I'll add you as a reviewer when done. https://codereview.chromium.org/2528483004/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/common.py (right): https://codereview.chromium.org/2528483004/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/common.py:67: self.chrome_args = {} On 2016/11/29 19:31:46, Ryan Sturm wrote: > can this be a set instead of a dict? I don't see much value in setting the key > pair to value : true when you can just add value instead. Done in follow up CL https://codereview.chromium.org/2528483004/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/common.py:188: return self._driver.execute_script("return " + script) On 2016/11/29 19:31:46, Ryan Sturm wrote: > What happens if LoadPage was not called (i.e., _StartDriver was just called)? Chrome would start but would stay on a blank page. https://codereview.chromium.org/2528483004/diff/20001/tools/chrome_proxy/webd... File tools/chrome_proxy/webdriver/simple_smoke.py (right): https://codereview.chromium.org/2528483004/diff/20001/tools/chrome_proxy/webd... tools/chrome_proxy/webdriver/simple_smoke.py:19: time.sleep(5) On 2016/11/29 19:31:46, Ryan Sturm wrote: > Why is the sleep(5) necessary? Demonstration purposes only. Real tests won't have this. Added comment in follow up CL
Message was sent while issue was closed.
Description was changed from ========== Adds skeleton of new integration test framework. Includes a working test case and basic functionality. Much more to come. BUG= Committed: https://crrev.com/c2a3d73bf201088d52b9e1226f8277d8f011163c Cr-Commit-Position: refs/heads/master@{#435047} ========== to ========== Adds skeleton of new integration test framework. Includes a working test case and basic functionality. Much more to come. BUG=669956 Committed: https://crrev.com/c2a3d73bf201088d52b9e1226f8277d8f011163c Cr-Commit-Position: refs/heads/master@{#435047} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
