[Backward Compatible] Timing and RDB for Lower-level checks

Allows presubmit_support to perform Timing and RDB reporting for each of
the lower-level "CheckXYZ" functions, provided that
ENABLE_NEW_PRESUBMIT_CHECKS is called in the PRESUBMIT.py
file that we are running. I am using the variable to avoid the breakage
caused by the similar previous CL (2332321) which was not backwards
compatible.
In addition, asks for "-realm" with ResultDB so it can maintain the
security boundary.

Bug: 1106943
Change-Id: Ia49e8884e2653bee7de7ed7d254452d55dd5c617
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2341828
Commit-Queue: Saagar Sanghavi <saagarsanghavi@google.com>
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>

git_cl.py

@@ -1273,7 +1273,7 @@ class Changelist(object):
     return args
 
   def RunHook(self, committing, may_prompt, verbose, parallel, upstream,
-              description, all_files, resultdb=False):
+              description, all_files, resultdb=False, realm=None):
     """Calls sys.exit() if the hook fails; returns a HookResults otherwise."""
     args = self._GetCommonPresubmitArgs(verbose, upstream)
     args.append('--commit' if committing else '--upload')
@@ -1292,10 +1292,15 @@ class Changelist(object):
         args.extend(['--description_file', description_file])
 
         start = time_time()
-
         cmd = ['vpython', PRESUBMIT_SUPPORT] + args
-        if resultdb:
-          cmd = ['rdb', 'stream', '-new'] + cmd
+        if resultdb and realm:
+          cmd = ['rdb', 'stream', '-new', '-realm', realm, '--'] + cmd
+        elif resultdb:
+          # TODO (crbug.com/1113463): store realm somewhere and look it up so
+          # it is not required to pass the realm flag
+          print('Note: ResultDB reporting will NOT be performed because --realm'
+                ' was not specified. To enable ResultDB, please run the command'
+                ' again with the --realm argument to specify the LUCI realm.')
 
         p = subprocess2.Popen(cmd)
         exit_code = p.wait()
@@ -1413,7 +1418,8 @@ class Changelist(object):
           upstream=base_branch,
           description=change_desc.description,
           all_files=False,
-          resultdb=options.resultdb)
+          resultdb=options.resultdb,
+          realm=options.realm)
       self.ExtendCC(hook_results['more_cc'])
 
     print_stats(git_diff_args)
@@ -1864,7 +1870,7 @@ class Changelist(object):
     detail = self._GetChangeDetail(['LABELS'])
     return u'Commit-Queue' in detail.get('labels', {})
 
-  def CMDLand(self, force, bypass_hooks, verbose, parallel):
+  def CMDLand(self, force, bypass_hooks, verbose, parallel, resultdb, realm):
     if git_common.is_dirty_git_tree('land'):
       return 1
 
@@ -1905,7 +1911,8 @@ class Changelist(object):
           upstream=upstream,
           description=description,
           all_files=False,
-          resultdb=False)
+          resultdb=resultdb,
+          realm=realm)
 
     self.SubmitIssue(wait_for_merge=True)
     print('Issue %s has been submitted.' % self.GetIssueURL())
@@ -3831,6 +3838,7 @@ def CMDpresubmit(parser, args):
   parser.add_option('--resultdb', action='store_true',
                     help='Run presubmit checks in the ResultSink environment '
                          'and send results to the ResultDB database.')
+  parser.add_option('--realm', help='LUCI realm if reporting to ResultDB')
   options, args = parser.parse_args(args)
 
   if not options.force and git_common.is_dirty_git_tree('presubmit'):
@@ -3857,7 +3865,8 @@ def CMDpresubmit(parser, args):
       upstream=base_branch,
       description=description,
       all_files=options.all,
-      resultdb=options.resultdb)
+      resultdb=options.resultdb,
+      realm=options.realm)
   return 0
 
 
@@ -4071,6 +4080,7 @@ def CMDupload(parser, args):
   parser.add_option('--resultdb', action='store_true',
                     help='Run presubmit checks in the ResultSink environment '
                          'and send results to the ResultDB database.')
+  parser.add_option('--realm', help='LUCI realm if reporting to ResultDB')
 
   orig_args = args
   (options, args) = parser.parse_args(args)
@@ -4217,6 +4227,10 @@ def CMDland(parser, args):
   parser.add_option('--parallel', action='store_true',
                     help='Run all tests specified by input_api.RunTests in all '
                          'PRESUBMIT files in parallel.')
+  parser.add_option('--resultdb', action='store_true',
+                     help='Run presubmit checks in the ResultSink environment '
+                          'and send results to the ResultDB database.')
+  parser.add_option('--realm', help='LUCI realm if reporting to ResultDB')
   (options, args) = parser.parse_args(args)
 
   cl = Changelist()
@@ -4225,8 +4239,8 @@ def CMDland(parser, args):
     DieWithError('You must upload the change first to Gerrit.\n'
                  '  If you would rather have `git cl land` upload '
                  'automatically for you, see http://crbug.com/642759')
-  return cl.CMDLand(options.force, options.bypass_hooks,
-                                     options.verbose, options.parallel)
+  return cl.CMDLand(options.force, options.bypass_hooks, options.verbose,
+                    options.parallel, options.resultdb, options.realm)
 
 
 @subcommand.usage('<patch url or issue id or issue url>')

presubmit_support.py

@@ -1496,7 +1496,6 @@ def DoPostUploadExecuter(change,
 
   return exit_code
 
-
 class PresubmitExecuter(object):
   def __init__(self, change, committing, verbose,
                gerrit_obj, dry_run=None, thread_pool=None, parallel=False):
@@ -1529,6 +1528,7 @@ class PresubmitExecuter(object):
     Return:
       A list of result objects, empty if no problems.
     """
+
     # Change to the presubmit file's directory to support local imports.
     main_path = os.getcwd()
     presubmit_dir = os.path.dirname(presubmit_path)
@@ -1541,47 +1541,81 @@ class PresubmitExecuter(object):
                          parallel=self.parallel)
     output_api = OutputApi(self.committing)
     context = {}
+
+    PRESUBMIT_VERSION = [0]
+    def REQUIRE_PRESUBMIT_VERSION(num):
+      PRESUBMIT_VERSION[0] = num
+
+    context['REQUIRE_PRESUBMIT_VERSION'] = REQUIRE_PRESUBMIT_VERSION
     try:
       exec(compile(script_text, 'PRESUBMIT.py', 'exec', dont_inherit=True),
            context)
     except Exception as e:
       raise PresubmitFailure('"%s" had an exception.\n%s' % (presubmit_path, e))
 
-    # These function names must change if we make substantial changes to
-    # the presubmit API that are not backwards compatible.
-    if self.committing:
-      function_name = 'CheckChangeOnCommit'
-    else:
-      function_name = 'CheckChangeOnUpload'
-    if function_name in context:
-      try:
-        context['__args'] = (input_api, output_api)
-        logging.debug('Running %s in %s', function_name, presubmit_path)
-
-        # TODO (crbug.com/1106943): Dive into each of the individual checks
-
-        # Get path of presubmit directory relative to repository root.
-        # Always use forward slashes, so that path is same in *nix and Windows
-        root = input_api.change.RepositoryRoot()
-        rel_path = os.path.relpath(presubmit_dir, root)
-        rel_path = rel_path.replace(os.path.sep, '/')
-
-        with rdb_wrapper.setup_rdb(function_name, rel_path) as my_status:
-          result = eval(function_name + '(*__args)', context)
-          self._check_result_type(result)
-          if any(res.fatal for res in result):
-            my_status.status = rdb_wrapper.STATUS_FAIL
-        logging.debug('Running %s done.', function_name)
-        self.more_cc.extend(output_api.more_cc)
-      finally:
-        for f in input_api._named_temporary_files:
-          os.remove(f)
-    else:
-      result = ()  # no error since the script doesn't care about current event.
+    context['__args'] = (input_api, output_api)
+
+    # Get path of presubmit directory relative to repository root
+    # Always use forward slashes, so that path is same in *nix and Windows
+    root = input_api.change.RepositoryRoot()
+    rel_path = os.path.relpath(presubmit_dir, root)
+    rel_path = rel_path.replace(os.path.sep, '/')
+
+    # Perform all the desired presubmit checks.
+    results = []
+
+    try:
+      if PRESUBMIT_VERSION[0] > 0:
+        for function_name in context:
+          if not function_name.startswith('Check'):
+            continue
+          if function_name.endswith('Commit') and not self.committing:
+            continue
+          if function_name.endswith('Upload') and self.committing:
+            continue
+          logging.debug('Running %s in %s', function_name, presubmit_path)
+          results.extend(
+              self._run_check_function(function_name, context, rel_path))
+          logging.debug('Running %s done.', function_name)
+          self.more_cc.extend(output_api.more_cc)
+
+      else: # Old format
+        if self.committing:
+          function_name = 'CheckChangeOnCommit'
+        else:
+          function_name = 'CheckChangeOnUpload'
+        if function_name in context:
+            logging.debug('Running %s in %s', function_name, presubmit_path)
+            results.extend(
+                self._run_check_function(function_name, context, rel_path))
+            logging.debug('Running %s done.', function_name)
+            self.more_cc.extend(output_api.more_cc)
+
+    finally:
+      for f in input_api._named_temporary_files:
+        os.remove(f)
 
     # Return the process to the original working directory.
     os.chdir(main_path)
-    return result
+    return results
+
+  def _run_check_function(self, function_name, context, prefix):
+    """Evaluates a presubmit check function, function_name, in the context
+    provided. If LUCI_CONTEXT is enabled, it will send the result to ResultSink.
+    Passes function_name and prefix to rdb_wrapper.setup_rdb. Returns results.
+
+    Args:
+      function_name: a string representing the name of the function to run
+      context: a context dictionary in which the function will be evaluated
+      prefix: a string describing prefix for ResultDB test id
+
+    Returns: Results from evaluating the function call."""
+    with rdb_wrapper.setup_rdb(function_name, prefix) as my_status:
+      result = eval(function_name + '(*__args)', context)
+      self._check_result_type(result)
+      if any(res.fatal for res in result):
+        my_status.status = rdb_wrapper.STATUS_FAIL
+      return result
 
   def _check_result_type(self, result):
     """Helper function which ensures result is a list, and all elements are

tests/git_cl_test.py

@@ -2152,7 +2152,9 @@ class TestGitCl(unittest.TestCase):
     self.assertEqual(0, cl.CMDLand(force=True,
                                    bypass_hooks=True,
                                    verbose=True,
-                                   parallel=False))
+                                   parallel=False,
+                                   resultdb=False,
+                                   realm=None))
     self.assertIn(
         'Issue chromium-review.googlesource.com/123 has been submitted',
         sys.stdout.getvalue())
@@ -2794,11 +2796,12 @@ class ChangelistTest(unittest.TestCase):
         upstream='upstream',
         description='description',
         all_files=False,
-        resultdb=True)
+        resultdb=True,
+        realm='chromium:public')
 
     self.assertEqual(expected_results, results)
     subprocess2.Popen.assert_called_once_with([
-        'rdb', 'stream', '-new',
+        'rdb', 'stream', '-new', '-realm', 'chromium:public', '--',
         'vpython', 'PRESUBMIT_SUPPORT',
         '--root', 'root',
         '--upstream', 'upstream',
@@ -2937,7 +2940,8 @@ class CMDPresubmitTestCase(CMDTestCaseBase):
         upstream='upstream',
         description='fetch description',
         all_files=None,
-        resultdb=None)
+        resultdb=None,
+        realm=None)
 
   def testNoIssue(self):
     git_cl.Changelist.GetIssue.return_value = None
@@ -2950,7 +2954,8 @@ class CMDPresubmitTestCase(CMDTestCaseBase):
         upstream='upstream',
         description='get description',
         all_files=None,
-        resultdb=None)
+        resultdb=None,
+        realm=None)
 
   def testCustomBranch(self):
     self.assertEqual(0, git_cl.main(['presubmit', 'custom_branch']))
@@ -2962,12 +2967,13 @@ class CMDPresubmitTestCase(CMDTestCaseBase):
         upstream='custom_branch',
         description='fetch description',
         all_files=None,
-        resultdb=None)
+        resultdb=None,
+        realm=None)
 
   def testOptions(self):
     self.assertEqual(
         0, git_cl.main(['presubmit', '-v', '-v', '--all', '--parallel', '-u',
-                          '--resultdb']))
+                          '--resultdb', '--realm', 'chromium:public']))
     git_cl.Changelist.RunHook.assert_called_once_with(
         committing=False,
         may_prompt=False,
@@ -2976,7 +2982,8 @@ class CMDPresubmitTestCase(CMDTestCaseBase):
         upstream='upstream',
         description='fetch description',
         all_files=True,
-        resultdb=True)
+        resultdb=True,
+        realm='chromium:public')
 
 class CMDTryResultsTestCase(CMDTestCaseBase):
   _DEFAULT_REQUEST = {

tests/presubmit_unittest.py

@@ -555,7 +555,7 @@ class PresubmitUnittest(PresubmitTestsBase):
     executer = presubmit.PresubmitExecuter(
         self.fake_change, False, None, False)
 
-    self.assertEqual((), executer.ExecPresubmitScript(
+    self.assertEqual([], executer.ExecPresubmitScript(
       ('def CheckChangeOnUpload(input_api, output_api):\n'
        '  if len(input_api._named_temporary_files):\n'
        '    return (output_api.PresubmitError("!!"),)\n'