Add cleaned up Gerrit props to properties.test_api.tryserver.

recipe_modules/properties/test_api.py

 # Copyright 2015 The LUCI Authors. All rights reserved.
 # Use of this source code is governed under the Apache License, Version 2.0
 # that can be found in the LICENSE file.
 
+import urlparse

[Michael Achenbach, 2016/10/24 09:24:34]

nit: sort

[tandrii(chromium), 2016/10/24 11:23:17]

Done.

+import re
+
 from recipe_engine import recipe_test_api
 
 class PropertiesTestApi(recipe_test_api.RecipeTestApi):
   def __call__(self, **kwargs):
     ret = self.test(None)
     ret.properties.update(kwargs)
     return ret
 
   def generic(self, **kwargs):
     """
@@ skipping 32 matching lines @@
     """
     ret = self.generic(
         branch='master',
         project='',
         repository='https://chromium.googlesource.com/chromium/src.git',
         revision='c14d891d44f0afff64e56ed7c9702df1d807b1ee',
     )
     ret.properties.update(kwargs)
     return ret
 
+  def _gerrit_tryserver(self, **kwargs):
+    # Call it through self.tryserver with (gerrit_project='infra/infra').
+    project = kwargs.pop('gerrit_project')
+    gerrit_url = kwargs.pop('gerrit_url', None)
+    git_url = kwargs.pop('git_url', None)
+    if not gerrit_url and not git_url:
+      gerrit_url = 'https://chromium-review.googlesource.com'
+      git_url = 'https://chromium.googlesource.com/' + project
+    elif gerrit_url and not git_url:
+      parsed = list(urlparse.urlparse(gerrit_url))
+      m = re.match('^((\w+)(-\w+)*)-review.googlesource.com$', parsed[1])

[Michael Achenbach, 2016/10/24 09:24:34]

Don't you need an r'' ?

[Michael Achenbach, 2016/10/24 09:24:34]

Maybe mark the non-capturing groups as you don't need the captured results, I
think e.g.
(?:-\w+)*

Same below

[tandrii(chromium), 2016/10/24 11:23:17]

I don't understand why. I take 1st group, which is outermost.
Your regex isn't clear to me, tbh. Why is there ':'?

[tandrii(chromium), 2016/10/24 11:23:17]

I do, and I'd swear it was there. How does it even work without?

[Michael Achenbach, 2016/10/24 11:51:51]

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 (?:)

+      assert m, ('Can\'t guess git_url from gerrit_url "%s", '

[martiniss, 2016/10/24 16:48:21]

nit: asserts can be disabled in python, turn this into an exception

[tandrii(chromium), 2016/10/24 18:02:19]

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 :)

+                 'specify it as extra kwarg' % parsed[1])
+      parsed[1] = m.group(1) + '.googlesource.com'
+      parsed[2] = project
+      git_url = urlparse.urlunparse(parsed)
+    elif git_url and not gerrit_url:
+      parsed = list(urlparse.urlparse(git_url))
+      m = re.match('^((\w+)(-\w+)*).googlesource.com$', parsed[1])
+      assert m, ('Can\'t guess gerrit_url from git_url "%s", '
+                 ', specify it as extra kwarg' % parsed[1])

[Michael Achenbach, 2016/10/24 09:24:34]

nit: double comma

+      parsed[1] = m.group(1) + '.googlesource.com'

[Michael Achenbach, 2016/10/24 09:24:34]

Don't you need to add "-review" in this case? Thought it is the reverse of the
case above?

[tandrii(chromium), 2016/10/24 11:23:17]

Yep, fixed.

+      gerrit_url = urlparse.urlunparse(parsed[:2] + [''] * len(parsed[2:]))
+    assert project
+    assert git_url
+    assert gerrit_url
+    # Pop old style values from kwargs.
+    patch_issue = int(kwargs.pop('issue', 456789))
+    patch_set = int(kwargs.pop('patchset', 12))
+    # Note that new Gerrit patch properties all start with 'patch_' prefix.
+    ret = self.generic(
+        patch_storage='gerrit',
+        patch_gerrit_url=gerrit_url,

[Michael Achenbach, 2016/10/24 09:24:34]

Why not use equal names (e.g. patch_gerrit_url for property and param). It's
otherwise a bit confusing on the caller side.

[tandrii(chromium), 2016/10/24 11:23:17]

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? :)

[Michael Achenbach, 2016/10/24 11:51:51]

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.

+        patch_project=project,
+        patch_branch='master',
+        patch_issue=patch_issue,
+        patch_set=patch_set,
+        patch_repository_url=git_url,
+        patch_ref='refs/changes/%2d/%d/%d' % (
+            patch_issue % 100, patch_issue, patch_set)
+    )
+    ret.properties.update(kwargs)
+    return ret
+
   def tryserver(self, **kwargs):
     """
     Merge kwargs into a typical buildbot properties blob for a job fired off
     by a rietveld tryjob on the tryserver, and return the blob.
+
+    If gerrit_project is given, generated properties for tryjobs for Gerrit
+    patches as if they were scheduled by CQ. In this case, gerrit_url and
+    git_url could be used to customize expectations.
     """
-    ret = self.generic(
-        branch='',
-        issue=12853011,
-        patchset=1,
-        project='chrome',
-        repository='',
-        requester='commit-bot@chromium.org',
-        revision='HEAD',
-        rietveld='https://codereview.chromium.org',
-        patch_project='chromium',
-    )
+    if kwargs.get('gerrit_project') is not None:
+      return self._gerrit_tryserver(**kwargs)
+    else:
+      ret = self.generic(
+          branch='',
+          issue=12853011,
+          patchset=1,
+          project='chrome',
+          repository='',
+          requester='commit-bot@chromium.org',
+          revision='HEAD',
+          rietveld='https://codereview.chromium.org',
+          patch_project='chromium',
+      )
     ret.properties.update(kwargs)
     return ret
 
   def tryserver_gerrit(self, full_project_name, gerrit_host=None, **kwargs):
     """
+    DEPRECATED. Use tryserver(gerrit_project='infra/infra') instead.
+
     Merge kwargs into a typical buildbot properties blob for a job fired off
     by a gerrit tryjob on the tryserver, and return the blob.
 
     Arguments:
       full_project_name: (required) name of the project in Gerrit.
       gerrit_host: hostname of the gerrit server.
         Example: chromium-review.googlesource.com.
     """
+    # TODO(tandrii): remove this method.
     gerrit_host = gerrit_host or 'chromium-review.googlesource.com'
     parts = gerrit_host.split('.')
     assert parts[0].endswith('-review')
     parts[0] = parts[0][:-len('-review')]
     repository = 'https://%s/%s' % ('.'.join(parts), full_project_name)
 
     ret = self.generic(
         branch='',
         category='cq',
         gerrit='https://%s' % gerrit_host,
         patch_storage='gerrit',
         project=full_project_name,
         patch_project=full_project_name,
         reason='CQ',
         repository=repository,
         requester='commit-bot@chromium.org',
         revision='HEAD',
     )
     ret.properties.update({
         'event.change.id': u'%s~master~Ideadbeaf' %
             (full_project_name.replace('/', '%2F')),
         'event.change.number': 338811,
         'event.change.url': u'https://%s/#/c/338811' % gerrit_host,
         'event.patchSet.ref': u'refs/changes/11/338811/3',
     })
     ret.properties.update(kwargs)
     return ret