From eb1bd62b919ac1f7e5e3fdbdc8d6da232111baf1 Mon Sep 17 00:00:00 2001 From: Edward Lesmes Date: Mon, 1 Mar 2021 19:54:07 +0000 Subject: [PATCH] presubmit_support: Add flags for Gerrit project and target branch. Will be used in a follow-up CL to initialize code-owners client. Recipe-Nontrivial-Roll: build Change-Id: Iefe9176320b4d1ae7715e88a8db132e815be76ba Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2717979 Commit-Queue: Edward Lesmes Reviewed-by: Gavin Mak --- git_cl.py | 11 +++---- presubmit_support.py | 33 ++++++++++---------- recipes/recipe_modules/presubmit/api.py | 2 ++ tests/git_cl_test.py | 26 ++++++++++------ tests/presubmit_unittest.py | 40 ++++++++++++++----------- 5 files changed, 66 insertions(+), 46 deletions(-) diff --git a/git_cl.py b/git_cl.py index 141364bc0..e6d203010 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1290,14 +1290,17 @@ class Changelist(object): args.extend(['--verbose'] * verbose) + remote, remote_branch = self.GetRemoteBranch() + target_ref = GetTargetRef(remote, remote_branch, None) + args.extend(['--gerrit_url', self.GetCodereviewServer()]) + args.extend(['--gerrit_project', self.GetGerritProject()]) + args.extend(['--gerrit_branch', target_ref]) + author = self.GetAuthor() - gerrit_url = self.GetCodereviewServer() issue = self.GetIssue() patchset = self.GetPatchset() if author: args.extend(['--author', author]) - if gerrit_url: - args.extend(['--gerrit_url', gerrit_url]) if issue: args.extend(['--issue', str(issue)]) if patchset: @@ -1319,11 +1322,9 @@ class Changelist(object): with gclient_utils.temporary_file() as description_file: with gclient_utils.temporary_file() as json_output: - gclient_utils.FileWrite(description_file, description) args.extend(['--json_output', json_output]) args.extend(['--description_file', description_file]) - args.extend(['--gerrit_project', self.GetGerritProject()]) start = time_time() cmd = ['vpython', PRESUBMIT_SUPPORT] + args diff --git a/presubmit_support.py b/presubmit_support.py index 7b0396276..3a05c4b8e 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -376,8 +376,10 @@ class GerritAccessor(object): To avoid excessive Gerrit calls, caches the results. """ - def __init__(self, host): - self.host = host + def __init__(self, url=None, project=None, branch=None): + self.host = urlparse.urlparse(url).netloc if url else None + self.project = project + self.branch = branch self.cache = {} def _FetchChangeDetail(self, issue): @@ -1507,7 +1509,7 @@ def DoPostUploadExecuter(change, class PresubmitExecuter(object): def __init__(self, change, committing, verbose, gerrit_obj, dry_run=None, - thread_pool=None, parallel=False, gerrit_project=None): + thread_pool=None, parallel=False): """ Args: change: The Change object. @@ -1525,7 +1527,6 @@ class PresubmitExecuter(object): self.more_cc = [] self.thread_pool = thread_pool self.parallel = parallel - self.gerrit_project = gerrit_project def ExecPresubmitScript(self, script_text, presubmit_path): """Executes a single presubmit script. @@ -1567,10 +1568,10 @@ class PresubmitExecuter(object): rel_path = rel_path.replace(os.path.sep, '/') # Get the URL of git remote origin and use it to identify host and project - host = '' - if self.gerrit and self.gerrit.host: - host = self.gerrit.host - project = self.gerrit_project or '' + host = project = '' + if self.gerrit: + host = self.gerrit.host or '' + project = self.gerrit.project or '' # Prefix for test names prefix = 'presubmit:%s/%s:%s/' % (host, project, rel_path) @@ -1669,8 +1670,7 @@ def DoPresubmitChecks(change, gerrit_obj, dry_run=None, parallel=False, - json_output=None, - gerrit_project=None): + json_output=None): """Runs all presubmit checks that apply to the files in the change. This finds all PRESUBMIT.py files in directories enclosing the files in the @@ -1713,7 +1713,7 @@ def DoPresubmitChecks(change, results = [] thread_pool = ThreadPool() executer = PresubmitExecuter(change, committing, verbose, gerrit_obj, - dry_run, thread_pool, parallel, gerrit_project) + dry_run, thread_pool, parallel) if default_presubmit: if verbose: sys.stdout.write('Running default presubmit script.\n') @@ -1874,7 +1874,10 @@ def _parse_gerrit_options(parser, options): """ gerrit_obj = None if options.gerrit_url: - gerrit_obj = GerritAccessor(urlparse.urlparse(options.gerrit_url).netloc) + gerrit_obj = GerritAccessor( + url=options.gerrit_url, + project=options.gerrit_project, + branch=options.gerrit_branch) if not options.gerrit_fetch: return gerrit_obj @@ -1946,6 +1949,8 @@ def main(argv=None): 'to skip multiple canned checks.') parser.add_argument('--dry_run', action='store_true', help=argparse.SUPPRESS) parser.add_argument('--gerrit_url', help=argparse.SUPPRESS) + parser.add_argument('--gerrit_project', help=argparse.SUPPRESS) + parser.add_argument('--gerrit_branch', help=argparse.SUPPRESS) parser.add_argument('--gerrit_fetch', action='store_true', help=argparse.SUPPRESS) parser.add_argument('--parallel', action='store_true', @@ -1959,7 +1964,6 @@ def main(argv=None): help='List of files to be marked as modified when ' 'executing presubmit or post-upload hooks. fnmatch ' 'wildcards can also be used.') - parser.add_argument('--gerrit_project', help=argparse.SUPPRESS) options = parser.parse_args(argv) log_level = logging.ERROR @@ -1992,8 +1996,7 @@ def main(argv=None): gerrit_obj, options.dry_run, options.parallel, - options.json_output, - options.gerrit_project) + options.json_output) except PresubmitFailure as e: print(e, file=sys.stderr) print('Maybe your depot_tools is out of date?', file=sys.stderr) diff --git a/recipes/recipe_modules/presubmit/api.py b/recipes/recipe_modules/presubmit/api.py index 3fbdd2d59..6f780c122 100644 --- a/recipes/recipe_modules/presubmit/api.py +++ b/recipes/recipe_modules/presubmit/api.py @@ -98,6 +98,8 @@ class PresubmitApi(recipe_api.RecipeApi): '--issue', self.m.tryserver.gerrit_change.change, '--patchset', self.m.tryserver.gerrit_change.patchset, '--gerrit_url', 'https://%s' % self.m.tryserver.gerrit_change.host, + '--gerrit_project', self.m.tryserver.gerrit_change.project, + '--gerrit_branch', self.m.tryserver.gerrit_change_target_ref, '--gerrit_fetch', ] if self.m.cq.state == self.m.cq.DRY: diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index fdc884cad..be35fc7c4 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -2958,13 +2958,16 @@ class ChangelistTest(unittest.TestCase): mock.patch('git_cl.Changelist.GetAuthor', return_value='author').start() mock.patch('git_cl.Changelist.GetIssue', return_value=123456).start() mock.patch('git_cl.Changelist.GetPatchset', return_value=7).start() + mock.patch( + 'git_cl.Changelist.GetRemoteBranch', + return_value=('origin', 'refs/remotes/origin/main')).start() mock.patch('git_cl.PRESUBMIT_SUPPORT', 'PRESUBMIT_SUPPORT').start() mock.patch('git_cl.Settings.GetRoot', return_value='root').start() mock.patch('git_cl.time_time').start() mock.patch('metrics.collector').start() mock.patch('subprocess2.Popen').start() - mock.patch('git_cl.Changelist.GetGerritProject', - return_value='https://chromium-review.googlesource.com').start() + mock.patch( + 'git_cl.Changelist.GetGerritProject', return_value='project').start() self.addCleanup(mock.patch.stopall) self.temp_count = 0 @@ -2996,8 +2999,10 @@ class ChangelistTest(unittest.TestCase): '--root', 'root', '--upstream', 'upstream', '--verbose', '--verbose', - '--author', 'author', '--gerrit_url', 'https://chromium-review.googlesource.com', + '--gerrit_project', 'project', + '--gerrit_branch', 'refs/heads/main', + '--author', 'author', '--issue', '123456', '--patchset', '7', '--commit', @@ -3006,7 +3011,6 @@ class ChangelistTest(unittest.TestCase): '--all_files', '--json_output', '/tmp/fake-temp2', '--description_file', '/tmp/fake-temp1', - '--gerrit_project', 'https://chromium-review.googlesource.com', ]) gclient_utils.FileWrite.assert_called_once_with( '/tmp/fake-temp1', 'description') @@ -3030,7 +3034,6 @@ class ChangelistTest(unittest.TestCase): 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( @@ -3048,10 +3051,12 @@ class ChangelistTest(unittest.TestCase): 'vpython', 'PRESUBMIT_SUPPORT', '--root', 'root', '--upstream', 'upstream', + '--gerrit_url', 'https://chromium-review.googlesource.com', + '--gerrit_project', 'project', + '--gerrit_branch', 'refs/heads/main', '--upload', '--json_output', '/tmp/fake-temp2', '--description_file', '/tmp/fake-temp1', - '--gerrit_project', 'https://chromium-review.googlesource.com', ]) gclient_utils.FileWrite.assert_called_once_with( '/tmp/fake-temp1', 'description') @@ -3075,7 +3080,6 @@ class ChangelistTest(unittest.TestCase): 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( @@ -3095,10 +3099,12 @@ class ChangelistTest(unittest.TestCase): 'vpython', 'PRESUBMIT_SUPPORT', '--root', 'root', '--upstream', 'upstream', + '--gerrit_url', 'https://chromium-review.googlesource.com', + '--gerrit_project', 'project', + '--gerrit_branch', 'refs/heads/main', '--upload', '--json_output', '/tmp/fake-temp2', '--description_file', '/tmp/fake-temp1', - '--gerrit_project', 'https://chromium-review.googlesource.com', ]) @mock.patch('sys.exit', side_effect=SystemExitMock) @@ -3131,8 +3137,10 @@ class ChangelistTest(unittest.TestCase): '--root', 'root', '--upstream', 'upstream', '--verbose', '--verbose', - '--author', 'author', '--gerrit_url', 'https://chromium-review.googlesource.com', + '--gerrit_project', 'project', + '--gerrit_branch', 'refs/heads/main', + '--author', 'author', '--issue', '123456', '--patchset', '7', '--post_upload', diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index d745c7c40..fc4e1df40 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -500,7 +500,8 @@ class PresubmitUnittest(PresubmitTestsBase): 0, 0, None) - executer = presubmit.PresubmitExecuter(change, False, None, False) + executer = presubmit.PresubmitExecuter( + change, False, None, presubmit.GerritAccessor()) self.assertFalse(executer.ExecPresubmitScript('', fake_presubmit)) # No error if no on-upload entry point self.assertFalse(executer.ExecPresubmitScript( @@ -509,7 +510,8 @@ class PresubmitUnittest(PresubmitTestsBase): fake_presubmit )) - executer = presubmit.PresubmitExecuter(change, True, None, False) + executer = presubmit.PresubmitExecuter( + change, True, None, presubmit.GerritAccessor()) # No error if no on-commit entry point self.assertFalse(executer.ExecPresubmitScript( ('def CheckChangeOnUpload(input_api, output_api):\n' @@ -556,7 +558,8 @@ class PresubmitUnittest(PresubmitTestsBase): fake_presubmit = os.path.join(self.fake_root_dir, 'PRESUBMIT.py') change = presubmit.Change('mychange', '\n'.join(description_lines), self.fake_root_dir, files, 0, 0, None) - executer = presubmit.PresubmitExecuter(change, True, None, False) + executer = presubmit.PresubmitExecuter( + change, True, None, presubmit.GerritAccessor()) sink = self.rdb_client.__enter__.return_value = mock.MagicMock() # STATUS_PASS on success @@ -591,7 +594,7 @@ class PresubmitUnittest(PresubmitTestsBase): fake_presubmit = os.path.join(self.fake_root_dir, 'PRESUBMIT.py') executer = presubmit.PresubmitExecuter( - self.fake_change, False, None, False) + self.fake_change, False, None, presubmit.GerritAccessor()) self.assertEqual([], executer.ExecPresubmitScript( ('def CheckChangeOnUpload(input_api, output_api):\n' @@ -1156,40 +1159,43 @@ def CheckChangeOnCommit(input_api, output_api): self.assertEqual('author', options.author) self.assertEqual('description', options.description) - @mock.patch('presubmit_support.GerritAccessor', mock.Mock()) def testParseGerritOptions_NoGerritFetch(self): options = mock.Mock( gerrit_url='https://foo-review.googlesource.com/bar', + gerrit_project='project', + gerrit_branch='refs/heads/main', gerrit_fetch=False, author='author', description='description') gerrit_obj = presubmit._parse_gerrit_options(None, options) - presubmit.GerritAccessor.assert_called_once_with( - 'foo-review.googlesource.com') - self.assertEqual(presubmit.GerritAccessor.return_value, gerrit_obj) + self.assertEqual('foo-review.googlesource.com', gerrit_obj.host) + self.assertEqual('project', gerrit_obj.project) + self.assertEqual('refs/heads/main', gerrit_obj.branch) self.assertEqual('author', options.author) self.assertEqual('description', options.description) - @mock.patch('presubmit_support.GerritAccessor', mock.Mock()) - def testParseGerritOptions_GerritFetch(self): - accessor = mock.Mock() - accessor.GetChangeOwner.return_value = 'new owner' - accessor.GetChangeDescription.return_value = 'new description' - presubmit.GerritAccessor.return_value = accessor + @mock.patch('presubmit_support.GerritAccessor.GetChangeOwner') + @mock.patch('presubmit_support.GerritAccessor.GetChangeDescription') + def testParseGerritOptions_GerritFetch( + self, mockDescription, mockOwner): + mockDescription.return_value = 'new description' + mockOwner.return_value = 'new owner' options = mock.Mock( gerrit_url='https://foo-review.googlesource.com/bar', + gerrit_project='project', + gerrit_branch='refs/heads/main', gerrit_fetch=True, issue=123, patchset=4) gerrit_obj = presubmit._parse_gerrit_options(None, options) - presubmit.GerritAccessor.assert_called_once_with( - 'foo-review.googlesource.com') - self.assertEqual(presubmit.GerritAccessor.return_value, gerrit_obj) + self.assertEqual('foo-review.googlesource.com', gerrit_obj.host) + self.assertEqual('project', gerrit_obj.project) + self.assertEqual('refs/heads/main', gerrit_obj.branch) self.assertEqual('new owner', options.author) self.assertEqual('new description', options.description)