Preliminary changes to git cl and presubmit_support

Added an rdb wrapper and use it to time CheckChangeOn{Commit, Upload} from every PRESUBMIT.py file, then reports the results to ResultSink.

Bug: http://crbug.com/1107588

Change-Id: I9bc8a18ba615a76f3e0611eae5b2c4adc5924276
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2242097
Commit-Queue: Saagar Sanghavi <saagarsanghavi@google.com>
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Garrett Beaty <gbeaty@chromium.org>

.vpython

@@ -11,6 +11,13 @@ wheel: <
   version: "version:0.10.3"
 >
 
+# Used by:
+#    presubmit_support.py
+wheel: <
+  name: "infra/python/wheels/requests-py2_py3"
+  version: "version:2.13.0"
+>
+
 # Used by:
 #   my_activity.py
 wheel: <

.vpython3

@@ -11,6 +11,13 @@ wheel: <
   version: "version:0.13.1"
 >
 
+# Used by:
+#    presubmit_support.py
+wheel: <
+  name: "infra/python/wheels/requests-py2_py3"
+  version: "version:2.13.0"
+>
+
 # Used by:
 #   my_activity.py
 wheel: <

git_cl.py

@@ -1273,9 +1273,8 @@ class Changelist(object):
 
     return args
 
-  def RunHook(
-      self, committing, may_prompt, verbose, parallel, upstream, description,
-      all_files):
+  def RunHook(self, committing, may_prompt, verbose, parallel, upstream,
+              description, all_files, resultdb=False):
     """Calls sys.exit() if the hook fails; returns a HookResults otherwise."""
     args = self._GetCommonPresubmitArgs(verbose, upstream)
     args.append('--commit' if committing else '--upload')
@@ -1294,8 +1293,14 @@ class Changelist(object):
         args.extend(['--description_file', description_file])
 
         start = time_time()
-        p = subprocess2.Popen(['vpython', PRESUBMIT_SUPPORT] + args)
+
+        cmd = ['vpython', PRESUBMIT_SUPPORT] + args
+        if resultdb:
+          cmd = ['rdb', 'stream', '-new'] + cmd
+
+        p = subprocess2.Popen(cmd)
         exit_code = p.wait()
+
         metrics.collector.add_repeated('sub_commands', {
           'command': 'presubmit',
           'execution_time': time_time() - start,
@@ -1408,7 +1413,8 @@ class Changelist(object):
           parallel=options.parallel,
           upstream=base_branch,
           description=change_desc.description,
-          all_files=False)
+          all_files=False,
+          resultdb=options.resultdb)
       self.ExtendCC(hook_results['more_cc'])
 
     print_stats(git_diff_args)
@@ -1899,7 +1905,8 @@ class Changelist(object):
           parallel=parallel,
           upstream=upstream,
           description=description,
-          all_files=False)
+          all_files=False,
+          resultdb=False)
 
     self.SubmitIssue(wait_for_merge=True)
     print('Issue %s has been submitted.' % self.GetIssueURL())
@@ -3820,6 +3827,9 @@ def CMDpresubmit(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.')
   options, args = parser.parse_args(args)
 
   if not options.force and git_common.is_dirty_git_tree('presubmit'):
@@ -3845,7 +3855,8 @@ def CMDpresubmit(parser, args):
       parallel=options.parallel,
       upstream=base_branch,
       description=description,
-      all_files=options.all)
+      all_files=options.all,
+      resultdb=options.resultdb)
   return 0
 
 
@@ -4052,6 +4063,9 @@ def CMDupload(parser, args):
                          'or a new commit is created.')
   parser.add_option('--git-completion-helper', action="store_true",
                     help=optparse.SUPPRESS_HELP)
+  parser.add_option('--resultdb', action='store_true',
+                    help='Run presubmit checks in the ResultSink environment '
+                         'and send results to the ResultDB database.')
 
   orig_args = args
   (options, args) = parser.parse_args(args)

presubmit_support.py

@@ -46,6 +46,7 @@ import gerrit_util
 import owners
 import owners_finder
 import presubmit_canned_checks
+import rdb_wrapper
 import scm
 import subprocess2 as subprocess  # Exposed through the API.
 
@@ -63,7 +64,6 @@ else:
 # Ask for feedback only once in program lifetime.
 _ASKED_FOR_FEEDBACK = False
 
-
 def time_time():
   # Use this so that it can be mocked in tests without interfering with python
   # system machinery.
@@ -1557,20 +1557,23 @@ class PresubmitExecuter(object):
       try:
         context['__args'] = (input_api, output_api)
         logging.debug('Running %s in %s', function_name, presubmit_path)
-        result = eval(function_name + '(*__args)', context)
+
+        # TODO (crbug.com/1106943): Dive into each of the individual checks
+
+        rel_path = os.path.relpath(os.getcwd(), main_path)
+        # Always use forward slashes, so that path is same in *nix and Windows
+        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)
-      if not isinstance(result, (tuple, list)):
-        raise PresubmitFailure(
-          'Presubmit functions must return a tuple or list')
-      for item in result:
-        if not isinstance(item, OutputApi.PresubmitResult):
-          raise PresubmitFailure(
-            'All presubmit results must be of types derived from '
-            'output_api.PresubmitResult')
     else:
       result = ()  # no error since the script doesn't care about current event.
 
@@ -1578,6 +1581,17 @@ class PresubmitExecuter(object):
     os.chdir(main_path)
     return result
 
+  def _check_result_type(self, result):
+    """Helper function which ensures result is a list, and all elements are
+    instances of OutputApi.PresubmitResult"""
+    if not isinstance(result, (tuple, list)):
+      raise PresubmitFailure('Presubmit functions must return a tuple or list')
+    if not all(isinstance(res, OutputApi.PresubmitResult) for res in result):
+      raise PresubmitFailure(
+        'All presubmit results must be of types derived from '
+        'output_api.PresubmitResult')
+
+
 def DoPresubmitChecks(change,
                       committing,
                       verbose,
@@ -1642,7 +1656,6 @@ def DoPresubmitChecks(change,
       # Accept CRLF presubmit script.
       presubmit_script = gclient_utils.FileRead(filename, 'rU')
       results += executer.ExecPresubmitScript(presubmit_script, filename)
-
     results += thread_pool.RunAsync()
 
     messages = {}

rdb_wrapper.py

+#!/usr/bin/env vpython
+# Copyright (c) 2020 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.
+
+import contextlib
+import json
+import os
+import requests
+import time
+
+# Constants describing TestStatus for ResultDB
+STATUS_PASS = 'PASS'
+STATUS_FAIL = 'FAIL'
+STATUS_CRASH = 'CRASH'
+STATUS_ABORT = 'ABORT'
+STATUS_SKIP = 'SKIP'
+
+class ResultSinkStatus(object):
+  def __init__(self):
+    self.status = STATUS_PASS
+
+@contextlib.contextmanager
+def setup_rdb(function_name, rel_path):
+  """Context Manager function for ResultDB reporting.
+
+  Args:
+    function_name (str): The name of the function we are about to run.
+    rel_path (str): The relative path from the root of the repository to the
+      directory defining the check being executed.
+  """
+  sink = None
+  if 'LUCI_CONTEXT' in os.environ:
+    with open(os.environ['LUCI_CONTEXT']) as f:
+      j = json.load(f)
+      if 'result_sink' in j:
+        sink = j['result_sink']
+
+  my_status = ResultSinkStatus()
+  start_time = time.time()
+  try:
+    yield my_status
+  except Exception:
+    my_status.status = STATUS_FAIL
+    raise
+  finally:
+    end_time = time.time()
+    elapsed_time = end_time - start_time
+    if sink != None:
+      tr = {
+          'testId': '{0}/:{1}'.format(rel_path, function_name),
+          'status': my_status.status,
+          'expected': (my_status.status == STATUS_PASS),
+          'duration': '{:.9f}s'.format(elapsed_time)
+      }
+      requests.post(
+          url='http://{0}/prpc/luci.resultsink.v1.Sink/ReportTestResults'
+                  .format(sink['address']),
+          headers={
+            'Content-Type': 'application/json',
+            'Accept': 'application/json',
+            'Authorization': 'ResultSink {0}'.format(sink['auth_token'])
+          },
+          data=json.dumps({'testResults': [tr]})
+    )

tests/git_cl_test.py

@@ -1004,7 +1004,7 @@ class TestGitCl(unittest.TestCase):
     mock.patch('git_cl.gclient_utils.RunEditor',
               lambda *_, **__: self._mocked_call(['RunEditor'])).start()
     mock.patch('git_cl.DownloadGerritHook', lambda force: self._mocked_call(
-      'DownloadGerritHook', force)).start()
+               'DownloadGerritHook', force)).start()
     mock.patch('git_cl.gclient_utils.FileRead',
               lambda path: self._mocked_call(['FileRead', path])).start()
     mock.patch('git_cl.gclient_utils.FileWrite',
@@ -2691,7 +2691,8 @@ class ChangelistTest(unittest.TestCase):
         parallel=True,
         upstream='upstream',
         description='description',
-        all_files=True)
+        all_files=True,
+        resultdb=False)
 
     self.assertEqual(expected_results, results)
     subprocess2.Popen.assert_called_once_with([
@@ -2742,7 +2743,8 @@ class ChangelistTest(unittest.TestCase):
         parallel=False,
         upstream='upstream',
         description='description',
-        all_files=False)
+        all_files=False,
+        resultdb=False)
 
     self.assertEqual(expected_results, results)
     subprocess2.Popen.assert_called_once_with([
@@ -2761,6 +2763,44 @@ class ChangelistTest(unittest.TestCase):
       'exit_code': 0,
     })
 
+  def testRunHook_FewerOptionsResultDB(self):
+    expected_results = {
+      'more_cc': ['more@example.com', 'cc@example.com'],
+      'should_continue': True,
+    }
+    gclient_utils.FileRead.return_value = json.dumps(expected_results)
+    git_cl.time_time.side_effect = [100, 200]
+    mockProcess = mock.Mock()
+    mockProcess.wait.return_value = 0
+    subprocess2.Popen.return_value = mockProcess
+
+    git_cl.Changelist.GetAuthor.return_value = None
+    git_cl.Changelist.GetIssue.return_value = None
+    git_cl.Changelist.GetPatchset.return_value = None
+    git_cl.Changelist.GetCodereviewServer.return_value = None
+
+    cl = git_cl.Changelist()
+    results = cl.RunHook(
+        committing=False,
+        may_prompt=False,
+        verbose=0,
+        parallel=False,
+        upstream='upstream',
+        description='description',
+        all_files=False,
+        resultdb=True)
+
+    self.assertEqual(expected_results, results)
+    subprocess2.Popen.assert_called_once_with([
+        'rdb', 'stream', '-new',
+        'vpython', 'PRESUBMIT_SUPPORT',
+        '--root', 'root',
+        '--upstream', 'upstream',
+        '--upload',
+        '--json_output', '/tmp/fake-temp2',
+        '--description_file', '/tmp/fake-temp1',
+    ])
+
   @mock.patch('sys.exit', side_effect=SystemExitMock)
   def testRunHook_Failure(self, _mock):
     git_cl.time_time.side_effect = [100, 200]
@@ -2777,7 +2817,8 @@ class ChangelistTest(unittest.TestCase):
           parallel=True,
           upstream='upstream',
           description='description',
-          all_files=True)
+          all_files=True,
+          resultdb=False)
 
     sys.exit.assert_called_once_with(2)
 
@@ -2889,7 +2930,8 @@ class CMDPresubmitTestCase(CMDTestCaseBase):
         parallel=None,
         upstream='upstream',
         description='fetch description',
-        all_files=None)
+        all_files=None,
+        resultdb=None)
 
   def testNoIssue(self):
     git_cl.Changelist.GetIssue.return_value = None
@@ -2901,7 +2943,8 @@ class CMDPresubmitTestCase(CMDTestCaseBase):
         parallel=None,
         upstream='upstream',
         description='get description',
-        all_files=None)
+        all_files=None,
+        resultdb=None)
 
   def testCustomBranch(self):
     self.assertEqual(0, git_cl.main(['presubmit', 'custom_branch']))
@@ -2912,11 +2955,13 @@ class CMDPresubmitTestCase(CMDTestCaseBase):
         parallel=None,
         upstream='custom_branch',
         description='fetch description',
-        all_files=None)
+        all_files=None,
+        resultdb=None)
 
   def testOptions(self):
     self.assertEqual(
-        0, git_cl.main(['presubmit', '-v', '-v', '--all', '--parallel', '-u']))
+        0, git_cl.main(['presubmit', '-v', '-v', '--all', '--parallel', '-u',
+                          '--resultdb']))
     git_cl.Changelist.RunHook.assert_called_once_with(
         committing=False,
         may_prompt=False,
@@ -2924,8 +2969,8 @@ class CMDPresubmitTestCase(CMDTestCaseBase):
         parallel=True,
         upstream='upstream',
         description='fetch description',
-        all_files=True)
-
+        all_files=True,
+        resultdb=True)
 
 class CMDTryResultsTestCase(CMDTestCaseBase):
   _DEFAULT_REQUEST = {

tests/presubmit_unittest.py

@@ -179,6 +179,7 @@ index fe3de7b..54ae6e1 100755
     mock.patch('os.path.isfile').start()
     mock.patch('os.remove').start()
     mock.patch('presubmit_support._parse_files').start()
+    mock.patch('presubmit_support.rdb_wrapper.setup_rdb').start()
     mock.patch('presubmit_support.sigint_handler').start()
     mock.patch('presubmit_support.time_time', return_value=0).start()
     mock.patch('presubmit_support.warn').start()
@@ -524,6 +525,20 @@ class PresubmitUnittest(PresubmitTestsBase):
       '  return "foo"',
       fake_presubmit)
 
+    self.assertFalse(executer.ExecPresubmitScript(
+      'def CheckChangeOnCommit(input_api, output_api):\n'
+      '  results = []\n'
+      '  results.extend(input_api.canned_checks.CheckChangeHasBugField(\n'
+      '    input_api, output_api))\n'
+      '  results.extend(input_api.canned_checks.CheckChangeHasNoUnwantedTags(\n'
+      '    input_api, output_api))\n'
+      '  results.extend(input_api.canned_checks.CheckChangeHasDescription(\n'
+      '    input_api, output_api))\n'
+      '  return results\n',
+    fake_presubmit))
+
+    presubmit.rdb_wrapper.setup_rdb.assert_called()
+
     self.assertRaises(presubmit.PresubmitFailure,
       executer.ExecPresubmitScript,
       'def CheckChangeOnCommit(input_api, output_api):\n'

tests/rdb_wrapper_test.py

+#!/usr/bin/env vpython3
+# Copyright (c) 2020 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.
+
+"""Unit tests for rdb_wrapper.py"""
+
+from __future__ import print_function
+
+import json
+import logging
+import os
+import requests
+import sys
+import time
+import unittest
+
+if sys.version_info.major == 2:
+  import mock
+  BUILTIN_OPEN = '__builtin__.open'
+else:
+  from unittest import mock
+  BUILTIN_OPEN = 'builtins.open'
+
+sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
+
+import rdb_wrapper
+
+class TestSetupRDB(unittest.TestCase):
+
+  def setUp(self):
+    super(TestSetupRDB, self).setUp()
+    mock.patch(BUILTIN_OPEN, mock.mock_open(read_data =
+      '''{"result_sink":{"address": "fakeAddr","auth_token" : "p@$$w0rD"}}''')
+      ).start()
+    mock.patch('os.environ', {'LUCI_CONTEXT': 'dummy_file.txt'}).start()
+    mock.patch('requests.post').start()
+    mock.patch('time.time', side_effect=[1.0, 2.0, 3.0, 4.0, 5.0]).start()
+
+  def test_setup_rdb(self):
+    with rdb_wrapper.setup_rdb("_foobar", './my/folder') as my_status_obj:
+      self.assertEqual(my_status_obj.status, rdb_wrapper.STATUS_PASS)
+      my_status_obj.status = rdb_wrapper.STATUS_FAIL
+
+    expectedTr = {
+        'testId'  : './my/folder/:_foobar',
+        'status'  : rdb_wrapper.STATUS_FAIL,
+        'expected': False,
+        'duration': '1.000000000s'
+    }
+
+    requests.post.assert_called_once_with(
+        url='http://fakeAddr/prpc/luci.resultsink.v1.Sink/ReportTestResults',
+        headers={
+            'Content-Type': 'application/json',
+            'Accept': 'application/json',
+            'Authorization': 'ResultSink p@$$w0rD'
+        },
+        data=json.dumps({'testResults': [expectedTr]})
+    )
+
+  def test_setup_rdb_exception(self):
+    with self.assertRaises(Exception):
+      with rdb_wrapper.setup_rdb("_foobar", './my/folder'):
+        raise Exception("Generic Error")
+
+    expectedTr = {
+        'testId': './my/folder/:_foobar',
+        'status': rdb_wrapper.STATUS_FAIL,
+        'expected': False,
+        'duration': '1.000000000s'
+    }
+
+    requests.post.assert_called_once_with(
+        url='http://fakeAddr/prpc/luci.resultsink.v1.Sink/ReportTestResults',
+        headers={
+            'Content-Type': 'application/json',
+            'Accept': 'application/json',
+            'Authorization': 'ResultSink p@$$w0rD'
+        },
+        data=json.dumps({'testResults': [expectedTr]})
+    )
+
+if __name__ == '__main__':
+  logging.basicConfig(
+      level=logging.DEBUG if '-v' in sys.argv else logging.ERROR)
+  unittest.main()