Fix two CheckLicense bugs and improve tests

Some recent "improvements" to CheckLicense were buggy, at least
partially because I did not increase test coverage when adding new code
paths. This change fixes two known mistakes and greatly increases test
coverage.

One bug was that the code assumed that match.groups()[0] would always be
the current year, but that assumption is invalid when a license regex is
passed in.

Another bug is that the new_license_re took away the possibility of the
license ending with */ but that is used in .css files.

The increased test coverage catches these and validates other
assumptions around new files.

The failures were reproduced and then the fixes validated using this
command:

  vpython3 tests/presubmit_unittest.py

Bug: 1098010
Change-Id: Ic91ca9f7a182660aef7b1eead79e841f58e15a3e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4032366
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: Josip Sokcevic <sokcevic@google.com>

presubmit_canned_checks.py

@@ -654,19 +654,19 @@ def CheckLongLines(input_api, output_api, maxlen, source_file_filter=None):
   return []
 
 
-def CheckLicense(input_api, output_api, license_re=None, project_name=None,
-    source_file_filter=None, accept_empty_files=True):
+def CheckLicense(input_api, output_api, license_re_param=None,
+    project_name=None, source_file_filter=None, accept_empty_files=True):
   """Verifies the license header.
   """
 
   # Early-out if the license_re is guaranteed to match everything.
-  if license_re and license_re == '.*':
+  if license_re_param and license_re_param == '.*':
     return []
 
   current_year = int(input_api.time.strftime('%Y'))
 
-  if license_re:
-    new_license_re = license_re
+  if license_re_param:
+    new_license_re = license_re = license_re_param
   else:
     project_name = project_name or 'Chromium'
 
@@ -693,7 +693,7 @@ def CheckLicense(input_api, output_api, license_re=None, project_name=None,
     # On new files don't tolerate any digression from the ideal.
     new_license_re = (r'.*? Copyright %(year)s The %(project)s Authors\n'
                       r'.*? %(key_line)s\n'
-                      r'.*? found in the LICENSE file\.\n') % {
+                      r'.*? found in the LICENSE file\.(?: \*/)?\n') % {
                           'year': years_re,
                           'project': project_name,
                           'key_line': key_line,
@@ -722,8 +722,9 @@ def CheckLicense(input_api, output_api, license_re=None, project_name=None,
       if not match:
         # License is totally wrong.
         bad_new_files.append(f.LocalPath())
-      elif match.groups()[0] != str(current_year):
-        # License does not list this year.
+      elif not license_re_param and match.groups()[0] != str(current_year):
+        # If we're using the built-in license_re on a new file then make sure
+        # the year is correct.
         wrong_year_new_files.append(f.LocalPath())
     elif not license_re.search(contents):
       bad_files.append(f.LocalPath())

tests/presubmit_unittest.py

@@ -2266,12 +2266,19 @@ the current line as well!
         None,
         presubmit.OutputApi.PresubmitPromptWarning)
 
-  def _LicenseCheck(self, text, license_text, committing, expected_result,
-      **kwargs):
+  def _LicenseCheck(self,
+                    text,
+                    license_text,
+                    committing,
+                    expected_result,
+                    new_file=False,
+                    **kwargs):
     change = mock.MagicMock(presubmit.GitChange)
     change.scm = 'svn'
     input_api = self.MockInputApi(change, committing)
     affected_file = mock.MagicMock(presubmit.GitAffectedFile)
+    if new_file:
+      affected_file.Action.return_value = 'A'
     input_api.AffectedSourceFiles.return_value = [affected_file]
     input_api.ReadFile.return_value = text
     if expected_result:
@@ -2300,6 +2307,16 @@ the current line as well!
     )
     self._LicenseCheck(text, license_text, True, None)
 
+  def testCheckLicenseSuccessNew(self):
+    # Make sure the license check works on new files with custom licenses.
+    text = ("#!/bin/python\n"
+            "# Copyright (c) 2037 Nobody.\n"
+            "# All Rights Reserved.\n"
+            "print('foo')\n")
+    license_text = (r".*? Copyright \(c\) 2037 Nobody.\n"
+                    r".*? All Rights Reserved\.\n")
+    self._LicenseCheck(text, license_text, True, None, new_file=True)
+
   def testCheckLicenseFailCommit(self):
     text = (
         "#!/bin/python\n"
@@ -2336,6 +2353,60 @@ the current line as well!
     )
     self._LicenseCheck(text, license_text, True, None, accept_empty_files=True)
 
+  def testCheckLicenseNewFilePass(self):
+    current_year = int(time.strftime('%Y'))
+    text = (
+        "#!/bin/python\n"
+        "# Copyright %d The Chromium Authors\n"
+        "# Use of this source code is governed by a BSD-style license that can "
+        "be\n"
+        "# found in the LICENSE file.\n"
+        "print('foo')\n" % current_year)
+    license_text = None
+    self._LicenseCheck(text, license_text, False, None, new_file=True)
+
+  def testCheckLicenseNewFileFail(self):
+    # Check that we fail on new files with the (c) symbol.
+    current_year = int(time.strftime('%Y'))
+    text = (
+        "#!/bin/python\n"
+        "# Copyright (c) %d The Chromium Authors\n"
+        "# Use of this source code is governed by a BSD-style license that can "
+        "be\n"
+        "# found in the LICENSE file.\n"
+        "print('foo')\n" % current_year)
+    license_text = None
+    self._LicenseCheck(text,
+                       license_text,
+                       False,
+                       presubmit.OutputApi.PresubmitError,
+                       new_file=True)
+
+  def _GetLicenseText(self, current_year):
+    return (
+        "#!/bin/python\n"
+        "# Copyright %d The Chromium Authors\n"
+        "# Use of this source code is governed by a BSD-style license that can "
+        "be\n"
+        "# found in the LICENSE file.\n"
+        "print('foo')\n" % current_year)
+
+  def testCheckLicenseNewFileWarn(self):
+    # Check that we warn on new files with wrong year. Test with first allowed
+    # year.
+    text = self._GetLicenseText(2006)
+    license_text = None
+    self._LicenseCheck(text,
+                       license_text,
+                       False,
+                       presubmit.OutputApi.PresubmitPromptWarning,
+                       new_file=True)
+
+  def testCheckLicenseNewCSSFilePass(self):
+    text = self._GetLicenseText(int(time.strftime('%Y')))
+    license_text = None
+    self._LicenseCheck(text, license_text, False, None, new_file=True)
+
   def testCannedCheckTreeIsOpenOpen(self):
     input_api = self.MockInputApi(None, True)
     input_api.urllib_request.urlopen().read.return_value = 'The tree is open'