|
|
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. |
DescriptionFix cmd line args override in common.py
In certain cases, it is important to be able to specify different command line arguments to chrome at the time the test is being run rather than what is in code. For example, running against a different proxy than is in the code.
This CL turns on the handling for this functionality and fixes a few bugs with it.
BUG=669956
Committed: https://crrev.com/ec1ab21b1df2c867a746ab50261ea02aad4256bc
Cr-Commit-Position: refs/heads/master@{#435525}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Bug fix #Patch Set 3 : Abandon those changes. Remove TODO #Patch Set 4 : undo changes on wrong branch #Patch Set 5 : Fix style. #Messages
Total messages: 17 (8 generated)
robertogden@chromium.org changed reviewers: + tbansal@chromium.org
tbansal@chromium.org changed reviewers: + ryansturm@chromium.org
Removing myself, and adding ryansturm@ as the reviewer. Just a quick comment: It might be useful to file a tracking bug in crbug, and use that bug in all the webdriver CLs. That would make it easier to track all the related CLs in a single place.
Description was changed from ========== Fix cmd line args override in common.py BUG= ========== to ========== Fix cmd line args override in common.py BUG=669956 ==========
https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/common.py (right): https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:80: Overrides any given arguments in the code with those given on the command I'm a little confused about how this is used. Can you add a better CL description? The way I understand this: Tests will be written with various flags using AddChromeArg within the code, but if the test is run with a command line override, the value of the flag will that is used is the one from the command line. Let me know if that's wrong. Assuming that is correct, it might be a little more clear to parse self._flags.browser_args into a seperate member dict during init, and in the Add/Remove calls only add the new arg if it's key is not in the dict. The reason I suggest this, is I don't understand when _OverrdieChromeArgs would be called and when AddChromeArgs would be called. https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:89: originalArgs = {} s/originalArgs/original_args/ AFAIK naming for python in chromium uses https://google.github.io/styleguide/pyguide.html?showone=Naming#Naming except for a couple cases (specifically your method names are correct for chromium with CamelCase): https://chromium.googlesource.com/chromium/src/+/master/styleguide/styleguide.md https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:93: for overrideArg in shlex.split(self._flags.browser_args): s/overrideArg/override_arg/ https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:94: argKey = GetDictKey(overrideArg) s/argKey/arg_key/
Description was changed from ========== Fix cmd line args override in common.py BUG=669956 ========== to ========== Fix cmd line args override in common.py In certain cases, it is important to be able to specify different command line arguments to chrome at the time the test is being run rather than what is in code. For example, running against a different proxy than is in the code. This CL turns on the handling for this functionality and fixes a few bugs with it. BUG=669956 ==========
https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/common.py (right): https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:80: Overrides any given arguments in the code with those given on the command On 2016/11/30 20:46:36, Ryan Sturm wrote: > I'm a little confused about how this is used. Can you add a better CL > description? > > The way I understand this: > Tests will be written with various flags using AddChromeArg within the code, but > if the test is run with a command line override, the value of the flag will that > is used is the one from the command line. > > Let me know if that's wrong. > > Assuming that is correct, it might be a little more clear to parse > self._flags.browser_args into a seperate member dict during init, and in the > Add/Remove calls only add the new arg if it's key is not in the dict. > > The reason I suggest this, is I don't understand when _OverrdieChromeArgs would > be called and when AddChromeArgs would be called. CL description improved, but yes you were correct. Do you mean turn _chrome_args into a dict? Filling that dict on init and then not letting calls to AddChromeArg would prevent client code from modifying existing flags, so flags couldn't be removed for example. Maybe that's ok, but it gives some weird behavior that I don't think would be expected. If not, then as it is _OverrideChromeArgs is called on _StartDriver so anything the client code did gets wiped away at the very last second before Chrome starts. https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:89: originalArgs = {} On 2016/11/30 20:46:36, Ryan Sturm wrote: > s/originalArgs/original_args/ > > AFAIK naming for python in chromium uses > https://google.github.io/styleguide/pyguide.html?showone=Naming#Naming > > except for a couple cases (specifically your method names are correct for > chromium with CamelCase): > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/styleguide.md Done. https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:93: for overrideArg in shlex.split(self._flags.browser_args): On 2016/11/30 20:46:36, Ryan Sturm wrote: > s/overrideArg/override_arg/ Done. https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:94: argKey = GetDictKey(overrideArg) On 2016/11/30 20:46:36, Ryan Sturm wrote: > s/argKey/arg_key/ Done.
lgtm https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive... File tools/chrome_proxy/webdriver/common.py (right): https://codereview.chromium.org/2538773004/diff/1/tools/chrome_proxy/webdrive... tools/chrome_proxy/webdriver/common.py:80: Overrides any given arguments in the code with those given on the command On 2016/12/01 01:23:04, Robert Ogden wrote: > On 2016/11/30 20:46:36, Ryan Sturm wrote: > > I'm a little confused about how this is used. Can you add a better CL > > description? > > > > The way I understand this: > > Tests will be written with various flags using AddChromeArg within the code, > but > > if the test is run with a command line override, the value of the flag will > that > > is used is the one from the command line. > > > > Let me know if that's wrong. > > > > Assuming that is correct, it might be a little more clear to parse > > self._flags.browser_args into a seperate member dict during init, and in the > > Add/Remove calls only add the new arg if it's key is not in the dict. > > > > The reason I suggest this, is I don't understand when _OverrdieChromeArgs > would > > be called and when AddChromeArgs would be called. > > CL description improved, but yes you were correct. > SGTM. > Do you mean turn _chrome_args into a dict? Filling that dict on init and then > not letting calls to AddChromeArg would prevent client code from modifying > existing flags, so flags couldn't be removed for example. Maybe that's ok, but > it gives some weird behavior that I don't think would be expected. > If not, then as it is _OverrideChromeArgs is called on _StartDriver so anything > the client code did gets wiped away at the very last second before Chrome > starts. That sounds fine then.
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": 80001, "attempt_start_ts": 1480556677683950, "parent_rev": "e1e2e582d3fce9faa84b8debacf173b2f63085ba", "commit_rev": "551d8781f84dc7e32c8a6a7fdf622f235dc50543"}
Message was sent while issue was closed.
Description was changed from ========== Fix cmd line args override in common.py In certain cases, it is important to be able to specify different command line arguments to chrome at the time the test is being run rather than what is in code. For example, running against a different proxy than is in the code. This CL turns on the handling for this functionality and fixes a few bugs with it. BUG=669956 ========== to ========== Fix cmd line args override in common.py In certain cases, it is important to be able to specify different command line arguments to chrome at the time the test is being run rather than what is in code. For example, running against a different proxy than is in the code. This CL turns on the handling for this functionality and fixes a few bugs with it. BUG=669956 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix cmd line args override in common.py In certain cases, it is important to be able to specify different command line arguments to chrome at the time the test is being run rather than what is in code. For example, running against a different proxy than is in the code. This CL turns on the handling for this functionality and fixes a few bugs with it. BUG=669956 ========== to ========== Fix cmd line args override in common.py In certain cases, it is important to be able to specify different command line arguments to chrome at the time the test is being run rather than what is in code. For example, running against a different proxy than is in the code. This CL turns on the handling for this functionality and fixes a few bugs with it. BUG=669956 Committed: https://crrev.com/ec1ab21b1df2c867a746ab50261ea02aad4256bc Cr-Commit-Position: refs/heads/master@{#435525} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ec1ab21b1df2c867a746ab50261ea02aad4256bc Cr-Commit-Position: refs/heads/master@{#435525} |