Have gcl report when unit tests are (or might be) missing from a change....

release/mac/gcl.py

 #!/usr/bin/python
+# Copyright (c) 2006-2009 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.
+
 # Wrapper script around Rietveld's upload.py that groups files into
 # changelists.
 
 import getpass
 import linecache
 import os
 import random
 import re
 import string
 import subprocess
 import sys
 import tempfile
 import upload
 import urllib2
 
 CODEREVIEW_SETTINGS = {
   # Default values.
   "CODE_REVIEW_SERVER": "codereview.chromium.org",
   "CC_LIST": "chromium-reviews@googlegroups.com",
   "VIEW_VC": "http://src.chromium.org/viewvc/chrome?view=rev&revision=",
 }
 
 # Use a shell for subcommands on Windows to get a PATH search, and because svn
 # may be a batch file.
 use_shell = sys.platform.startswith("win")
 
-# globals that store the root of the current repositary and the directory where
+# globals that store the root of the current repository and the directory where
 # we store information about changelists.
 repository_root = ""
 gcl_info_dir = ""
 
 # Filename where we store repository specific information for gcl.
 CODEREVIEW_SETTINGS_FILE = "codereview.settings"
 
+# Warning message when the change appears to be missing tests.
+MISSING_TEST_MSG = "Change contains new or modified methods, but no new tests!"
+
 # Caches whether we read the codereview.settings file yet or not.
 read_gcl_info = False
 
 
 def GetSVNFileInfo(file, field):
   """Returns a field from the svn info output for the given file."""
   output = RunShell(["svn", "info", file])
   for line in output.splitlines():
     search = field + ": "
     if line.startswith(search):
@@ skipping 68 matching lines @@
 
 def IsTreeOpen():
   """Fetches the tree status and returns either True or False."""
   url = GetCodeReviewSetting('STATUS')
   status = ""
   if url:
     status = urllib2.urlopen(url).read()
   return status.find('0') == -1
 
 
-def ErrorExit(msg):
-  """Print an error message to stderr and exit."""
+def Warn(msg):
+  ErrorExit(msg, exit=False)
+
+
+def ErrorExit(msg, exit=True):
+  """Print an error message to stderr and optionally exit."""
   print >>sys.stderr, msg
   sys.exit(1)
 
 
 def RunShellWithReturnCode(command, print_output=False):
   """Executes a command and returns the output and the return code."""
   p = subprocess.Popen(command, stdout=subprocess.PIPE,
                        stderr=subprocess.STDOUT, shell=use_shell,
                        universal_newlines=True)
   if print_output:
@@ skipping 39 matching lines @@
     issue: the Rietveld issue number, of "" if it hasn't been uploaded yet.
     description: the description.
     files: a list of 2 tuple containing (status, filename) of changed files,
            with paths being relative to the top repository directory.
   """
   def __init__(self, name="", issue="", description="", files=[]):
     self.name = name
     self.issue = issue
     self.description = description
     self.files = files
+    self.patch = None
 
   def FileList(self):
     """Returns a list of files."""
     return [file[1] for file in self.files]
 
+  def _NonDeletedFileList(self):
+    """Returns a list of files in this change, not including deleted files."""
+    return [file[1] for file in self.files if file[0] != "D"]
+
   def Save(self):
     """Writes the changelist information to disk."""
     data = SEPARATOR.join([self.issue,
                           "\n".join([f[0] + f[1] for f in self.files]),
                           self.description])
     WriteFile(GetChangelistInfoFile(self.name), data)
 
   def Delete(self):
     """Removes the changelist information from disk."""
     os.remove(GetChangelistInfoFile(self.name))
 
   def CloseIssue(self):
     """Closes the Rietveld issue for this changelist."""
     data = [("description", self.description),]
     ctype, body = upload.EncodeMultipartFormData(data, [])
     SendToRietveld("/" + self.issue + "/close", body, ctype)
 
   def UpdateRietveldDescription(self):
     """Sets the description for an issue on Rietveld."""
     data = [("description", self.description),]
     ctype, body = upload.EncodeMultipartFormData(data, [])
     SendToRietveld("/" + self.issue + "/description", body, ctype)
 
-  
+  def MissingTests(self):
+    """Returns True if the change looks like it needs unit tests but has none.
+
+    A change needs unit tests if it contains any new source files or methods.
+    """
+    SOURCE_SUFFIXES = [".cc", ".cpp", ".c", ".m", ".mm"]
+    # Ignore third_party entirely.
+    files = [file for file in self._NonDeletedFileList()
+             if file.find("third_party") == -1]
+
+    # Any new or modified test files?
+    # A test file's name ends with "test.*" or "tests.*".
+    test_files = [test for test in files
+                  if os.path.splitext(test)[0].rstrip("s").endswith("test")]
+    if len(test_files) > 0:
+      return False
+
+    # Any new source files?
+    source_files = [file for file in files
+                    if os.path.splitext(file)[1] in SOURCE_SUFFIXES]
+    if len(source_files) > 0:
+      return True
+
+    # Do the long test, checking the files for new methods.
+    return self._HasNewMethod()
+
+  def _HasNewMethod(self):
+    """Returns True if the changeset contains any new functions, or if a
+    function signature has been changed.
+
+    A function is identified by starting flush left, containing a "(" before
+    the next flush-left line, and either ending with "{" before the next
+    flush-left line or being followed by an unindented "{".
+
+    Currently this returns True for new methods, new static functions, and
+    methods or functions whose signatures have been changed.
+
+    Inline methods added to header files won't be detected by this. That's
+    acceptable for purposes of determining if a unit test is needed, since
+    inline methods should be trivial.
+    """
+    # To check for methods added to source or header files, we need the diffs.
+    # We'll generate them all, since there aren't likely to be many files
+    # apart from source and headers; besides, we'll want them all if we're
+    # uploading anyway.
+    if self.patch is None:
+      self.patch = GenerateDiff(self.FileList())
+
+    definition = ""
+    for line in self.patch.splitlines():
+      if not line.startswith("+"):
+        continue
+      line = line.strip("+").rstrip(" \t")
+      # Skip empty lines, comments, and preprocessor directives.
+      # TODO(pamg): Handle multiline comments if it turns out to be a problem.
+      if line == "" or line.startswith("/") or line.startswith("#"):
+        continue
+
+      # A possible definition ending with "{" is complete, so check it.
+      if definition.endswith("{"):
+        if definition.find("(") != -1:
+          return True
+        definition = ""
+
+      # A { or an indented line, when we're in a definition, continues it.
+      if (definition != "" and
+          (line == "{" or line.startswith(" ") or line.startswith("\t"))):
+        definition += line
+
+      # A flush-left line starts a new possible function definition.
+      elif not line.startswith(" ") and not line.startswith("\t"):
+        definition = line
+
+    return False
+
+
 SEPARATOR = "\n-----\n"
 # The info files have the following format:
 # issue_id\n
 # SEPARATOR\n
 # filepath1\n
 # filepath2\n
 # .
 # .
 # filepathn\n
 # SEPARATOR\n
@@ skipping 86 matching lines @@
                random.choice(string.ascii_lowercase) +
                random.choice(string.digits))
     if cl_name not in current_cl_names:
       return cl_name
 
 
 def GetModifiedFiles():
   """Returns a set that maps from changelist name to (status,filename) tuples.
 
   Files not in a changelist have an empty changelist name.  Filenames are in
-  relation to the top level directory of the current repositary.  Note that
+  relation to the top level directory of the current repository.  Note that
   only the current directory and subdirectories are scanned, in order to
   improve performance while still being flexible.
   """
   files = {}
-  
+
   # Since the files are normalized to the root folder of the repositary, figure
   # out what we need to add to the paths.
   dir_prefix = os.getcwd()[len(GetRepositoryRoot()):].strip(os.sep)
 
   # Get a list of all files in changelists.
   files_in_cl = {}
   for cl in GetCLs():
     change_info = LoadChangelistInfo(cl)
     for status, filename in change_info.files:
       files_in_cl[filename] = change_info.name
@@ skipping 158 matching lines @@
     if sys.platform.startswith("win"):
       editor = "notepad"
     else:
       editor = "vi"
 
   return editor
 
 
 def GenerateDiff(files):
   """Returns a string containing the diff for the given file list."""
+  previous_cwd = os.getcwd()
+  os.chdir(GetRepositoryRoot())
+
   diff = []
   for file in files:
     # Use svn info output instead of os.path.isdir because the latter fails
     # when the file is deleted.
     if GetSVNFileInfo(file, "Node Kind") == "directory":
       continue
     # If the user specified a custom diff command in their svn config file,
     # then it'll be used when we do svn diff, which we don't want to happen
     # since we want the unified diff.  Using --diff-cmd=diff doesn't always
     # work, since they can have another diff executable in their path that
     # gives different line endings.  So we use a bogus temp directory as the
     # config directory, which gets around these problems.
     if sys.platform.startswith("win"):
       parent_dir = tempfile.gettempdir()
     else:
       parent_dir = sys.path[0]  # tempdir is not secure.
     bogus_dir = os.path.join(parent_dir, "temp_svn_config")
     if not os.path.exists(bogus_dir):
       os.mkdir(bogus_dir)
     diff.append(RunShell(["svn", "diff", "--config-dir", bogus_dir, file]))
+  os.chdir(previous_cwd)
   return "".join(diff)
 
 
 def UploadCL(change_info, args):
   if not change_info.FileList():
     print "Nothing to upload, changelist is empty."
     return
 
   no_try = "--no_try" in args
   if no_try:
     args.remove("--no_try")
 
+  # TODO(pamg): Do something when tests are missing. The plan is to upload a
+  # message to Rietveld and have it shown in the UI attached to this patch.
+
   upload_arg = ["upload.py", "-y"]
   upload_arg.append("--server=" + GetCodeReviewSetting("CODE_REVIEW_SERVER"))
   upload_arg.extend(args)
 
   desc_file = ""
   if change_info.issue:  # Uploading a new patchset.
     found_message = False
     for arg in args:
       if arg.startswith("--message") or arg.startswith("-m"):
         found_message = True
@@ skipping 12 matching lines @@
     upload_arg.append("--description_file=" + desc_file + "")
     if change_info.description:
       subject = change_info.description[:77]
       if subject.find("\r\n") != -1:
         subject = subject[:subject.find("\r\n")]
       if subject.find("\n") != -1:
         subject = subject[:subject.find("\n")]
       if len(change_info.description) > 77:
         subject = subject + "..."
       upload_arg.append("--message=" + subject)
-  
+
   # Change the current working directory before calling upload.py so that it
   # shows the correct base.
+  previous_cwd = os.getcwd()
   os.chdir(GetRepositoryRoot())
 
   # If we have a lot of files with long paths, then we won't be able to fit
   # the command to "svn diff".  Instead, we generate the diff manually for
   # each file and concatenate them before passing it to upload.py.
-  issue, patchset = upload.RealMain(upload_arg,
-                                    GenerateDiff(change_info.FileList()))
+  if change_info.patch is None:
+    change_info.patch = GenerateDiff(change_info.FileList())
+  issue, patchset = upload.RealMain(upload_arg, change_info.patch)
   if issue and issue != change_info.issue:
     change_info.issue = issue
     change_info.Save()
 
   if desc_file:
     os.remove(desc_file)
 
   # Do background work on Rietveld to lint the file so that the results are
   # ready when the issue is viewed.
   SendToRietveld("/lint/issue%s_%s" % (issue, patchset), timeout=0.5)
 
   # Once uploaded to Rietveld, send it to the try server.
   if not no_try and GetCodeReviewSetting('TRY_ON_UPLOAD').lower() == 'true':
     # Use the local diff.
     TryChange(change_info, [], True)
 
+  os.chdir(previous_cwd)
+
 
 def TryChange(change_info, args, swallow_exception=False, patchset=None):
   """Create a diff file of change_info and send it to the try server."""
   try:
     import trychange
   except ImportError:
     if swallow_exception:
       return
     ErrorExit("You need to install trychange.py to use the try server.")
 
@@ skipping 31 matching lines @@
   os.write(handle, commit_message)
   os.close(handle)
 
   handle, targets_filename = tempfile.mkstemp(text=True)
   os.write(handle, "\n".join(change_info.FileList()))
   os.close(handle)
 
   commit_cmd += ['--file=' + commit_filename]
   commit_cmd += ['--targets=' + targets_filename]
   # Change the current working directory before calling commit.
+  previous_cwd = os.getcwd()
   os.chdir(GetRepositoryRoot())
   output = RunShell(commit_cmd, True)
   os.remove(commit_filename)
   os.remove(targets_filename)
   if output.find("Committed revision") != -1:
     change_info.Delete()
 
     if change_info.issue:
       revision = re.compile(".*?\nCommitted revision (\d+)",
                             re.DOTALL).match(output).group(1)
       viewvc_url = GetCodeReviewSetting("VIEW_VC")
       change_info.description = (change_info.description +
                                  "\n\nCommitted: " + viewvc_url + revision)
       change_info.CloseIssue()
+  os.chdir(previous_cwd)
 
 
 def Change(change_info):
   """Creates/edits a changelist."""
   if change_info.issue:
     try:
       description = GetIssueDescription(change_info.issue)
     except urllib2.HTTPError, err:
       if err.code == 404:
         # The user deleted the issue in Rietveld, so forget the old issue id.
@@ skipping 45 matching lines @@
       continue
     if line.startswith("---"):
       break
     status = line[:7]
     file = line[7:]
     new_cl_files.append((status, file))
   change_info.files = new_cl_files
 
   change_info.Save()
   print change_info.name + " changelist saved."
+  if change_info.MissingTests():
+    Warn("WARNING: " + MISSING_TEST_MSG)
 
 # We don't lint files in these path prefixes.
 IGNORE_PATHS = ("webkit",)
 
 # Valid extensions for files we want to lint.
 CPP_EXTENSIONS = ("cpp", "cc", "h")
 
 def Lint(change_info, args):
   """Runs cpplint.py on all the files in |change_info|"""
   try:
     import cpplint
   except ImportError:
     ErrorExit("You need to install cpplint.py to lint C++ files.")
 
   # Change the current working directory before calling lint so that it
   # shows the correct base.
+  previous_cwd = os.getcwd()
   os.chdir(GetRepositoryRoot())
 
   # Process cpplints arguments if any.
   filenames = cpplint.ParseArguments(args + change_info.FileList())
 
   for file in filenames:
     if len([file for suffix in CPP_EXTENSIONS if file.endswith(suffix)]):
       if len([file for prefix in IGNORE_PATHS if file.startswith(prefix)]):
         print "Ignoring non-Google styled file %s" % file
       else:
         cpplint.ProcessFile(file, cpplint._cpplint_state.verbose_level)
 
   print "Total errors found: %d\n" % cpplint._cpplint_state.error_count
+  os.chdir(previous_cwd)
 
 
 def Changes():
-  """Print all the changlists and their files."""
+  """Print all the changelists and their files."""
   for cl in GetCLs():
     change_info = LoadChangelistInfo(cl, True, True)
     print "\n--- Changelist " + change_info.name + ":"
     for file in change_info.files:
       print "".join(file)
 
 
 def main(argv=None):
   if argv is None:
     argv = sys.argv
@@ skipping 72 matching lines @@
     # the files. This allows commands such as 'gcl diff xxx' to work.
     args =["svn", command]
     root = GetRepositoryRoot()
     args.extend([os.path.join(root, x) for x in change_info.FileList()])
     RunShell(args, True)
   return 0
 
 
 if __name__ == "__main__":
   sys.exit(main())