Add get_issue_number function to webkitpy/common/checkout/scm/git.py

third_party/WebKit/Tools/Scripts/webkitpy/common/net/rietveld.py

 # Copyright 2016 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Utility functions to communicate with Rietveld."""
 
 import collections
 import json
 import logging
 import urllib2
-
+import subprocess
 
 _log = logging.getLogger(__name__)
 
 BASE_CODEREVIEW_URL = 'https://codereview.chromium.org/api'
 
 TryJob = collections.namedtuple('TryJob', ('builder_name', 'build_number'))
 
 def latest_try_jobs(issue_number, builder_names, web, patchset_number=None):
     """Returns a list of TryJob objects for jobs on the latest patchset.
 
@@ skipping 44 matching lines @@
         _log.error('Request failed to URL: %s' % url)
         raise
     try:
         return json.loads(contents)
     except ValueError:
         _log.error('Invalid JSON: %s' % contents)
         raise
 
 
 def _issue_url(issue_number):
-    return '%s/%s' % (BASE_CODEREVIEW_URL, issue_number)
-
+    if issue_number == 'None':
+        raise Exception('No issue number associated with this branch')
+    else:

[Dirk Pranke, 2016/07/06 21:22:37]

I would enforce that the issue_number is legit as early in the public entry
points as possible, i.e., check them as the first thing in latest_try_jobs() and
get_latest_try_job_results(), rather than checking as part of a private
function.

But, given what I wrote below, it's probably better to just leave this file
unmodified.

+        return '%s/%s' % (BASE_CODEREVIEW_URL, issue_number)
 
 def _patchset_url(issue_number, patchset_number):
     return '%s/%s' % (_issue_url(issue_number), patchset_number)
 
 
 def get_latest_try_job_results(issue_number, web):
     url = _latest_patchset_url(issue_number, web)
     patchset_data = _get_json(url, web)
     results = {}
     for job in patchset_data['try_job_results']:
         results[job['builder']] = job['result']
     return results
+
+
+def get_cl_issue_number():

[Dirk Pranke, 2016/07/06 21:22:37]

This method doesn't belong on this object, since you're talking to git, not to
Rietveld.

You should add a method to webkitpy/common/scm/git.py instead.

Also, if you use a pipe command and shell=True, this won't work on Windows. It's
easy enough to just get the output from `git cl issue` and parse it in python,
e.g. add a method to the Git class like:

  def issue_number(self):
    self._run_git(['cl', 'issue']).split()[2]

[qyearsley, 2016/07/06 21:38:56]

1. In other places in webkitpy, an Executive object (from
webkitpy.common.system.executive) is used; even if that module doesn't do
anything extra for us, there are two advantages to using it:
 - easily replaced with executive_mock.MockExecutive for tests
 - consistency with other code in this codebase

To use executive, you could make this function take an Executive object, and use
it when invoking a command. When testing this function, a MockExecutive would be
passed, and then you could assert that the command was called. Note, currently
there are two MockExecutive classes in
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...
MockExecutive2 looks more useful because we can specify command output in the
test.

For example:

def get_cl_issue_number(executive):
    out = executive.run_command(['git', 'cl', 'issue'])
    ...

And then in the unit test:

    def test_get_cl_number(self):
        executive = MockExecutive2(output='Issue number: 12345
(http://crrev.com/12345)')
        issue_number = get_cl_issue_number(executive)
        self.assertEqual(issue_number, 12345)
        self.assertEqual(executive.calls[0], ['git', 'cl', 'issue'])

[dcampb, 2016/07/06 22:13:06]

per dpranke@ comment, I moved the function to the git module. However, there is
no git_unittest.py. Should I add the unit test to the SCM unittest file and
import the git module?

[Dirk Pranke, 2016/07/06 22:33:29]

Actually mocking all of that stuff up in scm_unittest.py for this to be properly
testable is more trouble than it's worth, so I wouldn't bother trying to write a
test for it.

+    issue_number = subprocess.check_output('git cl issue | cut -d" " -f3', shell=True)

[qyearsley, 2016/07/06 21:38:56]

As I think you mentioned, this command using `cut` probably won't work on all
platforms.

So, for portability, we probably want process the output in Python itself.
Regular expressions would be one way of doing this:

  out = subprocess.check_output('git cl issue', shell=True)
  issue_number = re.search(r'\d+').group()
  return int(issue_number)

In the above, re.search(r'\d+') searches for the first substring the matches the
pattern \d+ (one or more digits), and the method group() returns the substring
that was found. 

Another way would be to use other string methods, e.g. `issue_number =
out.split()[2]`, but this is less flexible in case the output format changes.

+    issue_number = issue_number[:-1]
+    return issue_number