[Fixed from older branch] Timing/RDB for lower-level checks
presubmit: Report timing to rdb for individual checks
Rather than using CheckChangeOnCommit() or CheckChangeOnUpload() as the
sole entry points for PRESUBMIT.py files, this cl enables presubmit_support.py
to use any CheckXYZ[OnCommit/Upload] function to serve as an entry point.
This way, we can perform timing and RDB reporting for each of the
presubmit check functions and have check-specific data.
Bug: 1106943
Change-Id: Ifdabd68c0904a6d70a828f12de157369c6c1571d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2332321
Commit-Queue: Saagar Sanghavi <saagarsanghavi@google.com>
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@google.com>
diff --git a/git_cl.py b/git_cl.py
index 3878b96..09496db 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -1273,7 +1273,7 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
upstream=base_branch,
description=description,
all_files=options.all,
- resultdb=options.resultdb)
+ resultdb=options.resultdb,
+ realm=options.realm)
return 0
@@ -4071,6 +4080,7 @@
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 @@
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 @@
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>')
diff --git a/presubmit_support.py b/presubmit_support.py
index cd2eb7b..6c41f20 100755
--- a/presubmit_support.py
+++ b/presubmit_support.py
@@ -1529,6 +1529,7 @@
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)
@@ -1547,41 +1548,41 @@
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)
+ 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:
+ 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)
-
- # 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
+ results.extend(result)
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.
+
+ 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 _check_result_type(self, result):
"""Helper function which ensures result is a list, and all elements are
diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py
index 7bcd640..64196c9 100755
--- a/tests/git_cl_test.py
+++ b/tests/git_cl_test.py
@@ -2152,7 +2152,9 @@
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 @@
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 @@
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 @@
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 @@
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 @@
upstream='upstream',
description='fetch description',
all_files=True,
- resultdb=True)
+ resultdb=True,
+ realm='chromium:public')
class CMDTryResultsTestCase(CMDTestCaseBase):
_DEFAULT_REQUEST = {
diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py
index e21c957..d04e6e0 100755
--- a/tests/presubmit_unittest.py
+++ b/tests/presubmit_unittest.py
@@ -555,7 +555,7 @@
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'