Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2442173003:
Add cleaned up Gerrit props to properties.test_api.tryserver. (Closed)
|
Created:
4 years, 2 months ago by tandrii(chromium) Modified:
4 years, 1 month ago CC:
chromium-reviews, infra-reviews+recipes-py_chromium.org Target Ref:
refs/heads/master Project:
recipe_engine Visibility:
Public. |
DescriptionAdd cleaned up Gerrit props to properties.test_api.tryserver.
R=machenbach@chromium.org,martiniss@chromium.org
BUG=645616
Committed: https://github.com/luci/recipes-py/commit/24a5bf068cee997aec0c60df51ca81bfa5ac5416
Patch Set 1 #Patch Set 2 : better diff #
Total comments: 16
Patch Set 3 : review machenbach@ #
Total comments: 2
Patch Set 4 : raise AssertionError. #Patch Set 5 : Add pragma: no cover #
Total comments: 2
Patch Set 6 : +nit #Depends on Patchset: Messages
Total messages: 36 (21 generated)
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add cleaned up Gerrit props to properties.test_api.tryserver. R=machenbach@chromium.org,martiniss@chromium.org BUG=599931 ========== to ========== Add cleaned up Gerrit props to properties.test_api.tryserver. R=machenbach@chromium.org,martiniss@chromium.org BUG=645616 ==========
https://codereview.chromium.org/2442173003/diff/20001/recipe_modules/properti... File recipe_modules/properties/test_api.py (right): https://codereview.chromium.org/2442173003/diff/20001/recipe_modules/properti... recipe_modules/properties/test_api.py:5: import urlparse nit: sort https://codereview.chromium.org/2442173003/diff/20001/recipe_modules/properti... recipe_modules/properties/test_api.py:70: m = re.match('^((\w+)(-\w+)*)-review.googlesource.com$', parsed[1]) Don't you need an r'' ? https://codereview.chromium.org/2442173003/diff/20001/recipe_modules/properti... recipe_modules/properties/test_api.py:70: m = re.match('^((\w+)(-\w+)*)-review.googlesource.com$', parsed[1]) Maybe mark the non-capturing groups as you don't need the captured results, I think e.g. (?:-\w+)* Same below https://codereview.chromium.org/2442173003/diff/20001/recipe_modules/properti... recipe_modules/properties/test_api.py:80: ', specify it as extra kwarg' % parsed[1]) nit: double comma https://codereview.chromium.org/2442173003/diff/20001/recipe_modules/properti... recipe_modules/properties/test_api.py:81: parsed[1] = m.group(1) + '.googlesource.com' Don't you need to add "-review" in this case? Thought it is the reverse of the case above? https://codereview.chromium.org/2442173003/diff/20001/recipe_modules/properti... recipe_modules/properties/test_api.py:92: patch_gerrit_url=gerrit_url, Why not use equal names (e.g. patch_gerrit_url for property and param). It's otherwise a bit confusing on the caller side.
https://codereview.chromium.org/2442173003/diff/20001/recipe_modules/properti... File recipe_modules/properties/example.expected/buildbot_tryserver_gerrit_override_git.json (right): https://codereview.chromium.org/2442173003/diff/20001/recipe_modules/properti... recipe_modules/properties/example.expected/buildbot_tryserver_gerrit_override_git.json:20: "('patch_gerrit_url', 'https://chrome-internal.googlesource.com')", yep, you are right - missing review here. https://codereview.chromium.org/2442173003/diff/20001/recipe_modules/properti... File recipe_modules/properties/test_api.py (right): https://codereview.chromium.org/2442173003/diff/20001/recipe_modules/properti... recipe_modules/properties/test_api.py:5: import urlparse On 2016/10/24 09:24:34, machenbach (slow) wrote: > nit: sort Done. https://codereview.chromium.org/2442173003/diff/20001/recipe_modules/properti... recipe_modules/properties/test_api.py:70: m = re.match('^((\w+)(-\w+)*)-review.googlesource.com$', parsed[1]) On 2016/10/24 09:24:34, machenbach (slow) wrote: > Don't you need an r'' ? I do, and I'd swear it was there. How does it even work without? https://codereview.chromium.org/2442173003/diff/20001/recipe_modules/properti... recipe_modules/properties/test_api.py:70: m = re.match('^((\w+)(-\w+)*)-review.googlesource.com$', parsed[1]) On 2016/10/24 09:24:34, machenbach (slow) wrote: > Maybe mark the non-capturing groups as you don't need the captured results, I > think e.g. > (?:-\w+)* I don't understand why. I take 1st group, which is outermost. Your regex isn't clear to me, tbh. Why is there ':'? https://codereview.chromium.org/2442173003/diff/20001/recipe_modules/properti... recipe_modules/properties/test_api.py:81: parsed[1] = m.group(1) + '.googlesource.com' On 2016/10/24 09:24:34, machenbach (slow) wrote: > Don't you need to add "-review" in this case? Thought it is the reverse of the > case above? Yep, fixed. https://codereview.chromium.org/2442173003/diff/20001/recipe_modules/properti... recipe_modules/properties/test_api.py:92: patch_gerrit_url=gerrit_url, On 2016/10/24 09:24:34, machenbach (slow) wrote: > Why not use equal names (e.g. patch_gerrit_url for property and param). It's > otherwise a bit confusing on the caller side. I was and am undecided on this one. I didn't want to carry baggage of tryserver_gerrit, which is long and ugly. Thus, I was left with two ways: 1. require two kwargs (patch_storage='gerrit', patch_project=..., ...) and keep .tryserver() semantics the same as before (that is all kwargs given are ending up in result properties dict verbatim). 2. require just patch_project, but it can be named such so as to distinguish from rietveld case. But then semantics of tryserver() change. To encourage using it, I choose the 1st way. Then it followed, I thought, that gerrit_url is shorter to specify than patch_gerrit_url, and since I changed semantics already, why not keep the shorter option? WDYT about all this? :)
lgtm with comments: https://codereview.chromium.org/2442173003/diff/20001/recipe_modules/properti... File recipe_modules/properties/test_api.py (right): https://codereview.chromium.org/2442173003/diff/20001/recipe_modules/properti... recipe_modules/properties/test_api.py:70: m = re.match('^((\w+)(-\w+)*)-review.googlesource.com$', parsed[1]) On 2016/10/24 11:23:17, tandrii(chromium) wrote: > On 2016/10/24 09:24:34, machenbach (slow) wrote: > > Maybe mark the non-capturing groups as you don't need the captured results, I > > think e.g. > > (?:-\w+)* > I don't understand why. I take 1st group, which is outermost. > Your regex isn't clear to me, tbh. Why is there ':'? This would be the syntax, e.g. see: https://docs.python.org/2/howto/regex.html#non-capturing-and-named-groups But never mind. I think just () are easier to read than several (?:) https://codereview.chromium.org/2442173003/diff/20001/recipe_modules/properti... recipe_modules/properties/test_api.py:92: patch_gerrit_url=gerrit_url, On 2016/10/24 11:23:17, tandrii(chromium) wrote: > On 2016/10/24 09:24:34, machenbach (slow) wrote: > > Why not use equal names (e.g. patch_gerrit_url for property and param). It's > > otherwise a bit confusing on the caller side. > > I was and am undecided on this one. I didn't want to carry baggage of > tryserver_gerrit, which is long and ugly. Thus, I was left with two ways: > 1. require two kwargs (patch_storage='gerrit', patch_project=..., ...) and keep > .tryserver() semantics the same as before (that is all kwargs given are ending > up in result properties dict verbatim). > 2. require just patch_project, but it can be named such so as to distinguish > from rietveld case. But then semantics of tryserver() change. > > To encourage using it, I choose the 1st way. Then it followed, I thought, that > gerrit_url is shorter to specify than patch_gerrit_url, and since I changed > semantics already, why not keep the shorter option? > > WDYT about all this? :) Ack. Your choice. I don't have a strong opinion here. Now I know and don't fall into the same pit again. First when I saw the dependent patch's code+test code I got a bit confused. https://codereview.chromium.org/2442173003/diff/40001/recipe_modules/properti... File recipe_modules/properties/test_api.py (right): https://codereview.chromium.org/2442173003/diff/40001/recipe_modules/properti... recipe_modules/properties/test_api.py:70: m = re.match(r'^((\w+)(-\w+)*)-review.googlesource.com$', parsed[1]) I am a little surprised that the dash doesn't need a \- as you want to match a literal dash. Does it make a difference?
Stephen, I'll wait for your review before landing this and 5 follow up CLs. https://codereview.chromium.org/2442173003/diff/40001/recipe_modules/properti... File recipe_modules/properties/test_api.py (right): https://codereview.chromium.org/2442173003/diff/40001/recipe_modules/properti... recipe_modules/properties/test_api.py:70: m = re.match(r'^((\w+)(-\w+)*)-review.googlesource.com$', parsed[1]) On 2016/10/24 11:51:51, machenbach (slow) wrote: > I am a little surprised that the dash doesn't need a \- as you want to match a > literal dash. Does it make a difference? I tested it (with r'' in front! :) ), and it works this way. AFAIR, '-' is only special in [a-z] kind of construct.
can I see the new code? (with machenbach@'s comments addressed) https://codereview.chromium.org/2442173003/diff/20001/recipe_modules/properti... File recipe_modules/properties/test_api.py (right): https://codereview.chromium.org/2442173003/diff/20001/recipe_modules/properti... recipe_modules/properties/test_api.py:71: assert m, ('Can\'t guess git_url from gerrit_url "%s", ' nit: asserts can be disabled in python, turn this into an exception
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Stephen, I think you've reviewed the wrong PatchSet. Can you please TAL? https://codereview.chromium.org/2442173003/diff/20001/recipe_modules/properti... File recipe_modules/properties/test_api.py (right): https://codereview.chromium.org/2442173003/diff/20001/recipe_modules/properti... recipe_modules/properties/test_api.py:71: assert m, ('Can\'t guess git_url from gerrit_url "%s", ' On 2016/10/24 16:48:21, martiniss wrote: > nit: asserts can be disabled in python, turn this into an exception I know, but a) this is a test expectation, so disabling asserts is not very smart b) it'll fail one line below anyway and yet -> done :)
The CQ bit was checked by tandrii@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by tandrii@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
Try jobs failed on following builders: Recipes-py Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/3214405283c91810)
tandrii@chromium.org changed reviewers: + phajdan.jr@chromium.org
+Pawel for owners
LGTM w/nit https://codereview.chromium.org/2442173003/diff/80001/recipe_modules/properti... File recipe_modules/properties/test_api.py (right): https://codereview.chromium.org/2442173003/diff/80001/recipe_modules/properti... recipe_modules/properties/test_api.py:117: else: nit: No need for "else" after return.
The CQ bit was checked by tandrii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from machenbach@chromium.org, phajdan.jr@chromium.org Link to the patchset: https://codereview.chromium.org/2442173003/#ps100001 (title: "+nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2442173003/diff/80001/recipe_modules/properti... File recipe_modules/properties/test_api.py (right): https://codereview.chromium.org/2442173003/diff/80001/recipe_modules/properti... recipe_modules/properties/test_api.py:117: else: On 2016/10/25 10:58:54, Paweł Hajdan Jr. wrote: > nit: No need for "else" after return. Done.
Message was sent while issue was closed.
Description was changed from ========== Add cleaned up Gerrit props to properties.test_api.tryserver. R=machenbach@chromium.org,martiniss@chromium.org BUG=645616 ========== to ========== Add cleaned up Gerrit props to properties.test_api.tryserver. R=machenbach@chromium.org,martiniss@chromium.org BUG=645616 Committed: https://github.com/luci/recipes-py/commit/24a5bf068cee997aec0c60df51ca81bfa5a... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://github.com/luci/recipes-py/commit/24a5bf068cee997aec0c60df51ca81bfa5a... |
