From caeef7ba32da0a782b5b719d038a9dcab7104d6d Mon Sep 17 00:00:00 2001 From: Peter Kotwicz Date: Thu, 24 Aug 2023 02:34:52 +0000 Subject: [PATCH] Add tests for split_cl.py This CL: - Adds tests for split_cl.UploadCl() - Splits out logic checking bug links so that it can be tested Bug:None Change-Id: I3c08b129e0cfda67a3d93a2e01acef86d33e92b8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4743773 Reviewed-by: Josip Sokcevic Commit-Queue: Peter Kotwicz --- split_cl.py | 31 +++++++---- tests/split_cl_test.py | 113 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 125 insertions(+), 19 deletions(-) diff --git a/split_cl.py b/split_cl.py index 2888a7c31e..5587954034 100644 --- a/split_cl.py +++ b/split_cl.py @@ -142,7 +142,7 @@ def UploadCl(refactor_branch, refactor_branch_upstream, directories, files, # Upload a CL. upload_args = ['-f'] if reviewers: - upload_args.extend(['-r', ','.join(reviewers)]) + upload_args.extend(['-r', ','.join(sorted(reviewers))]) if cq_dry_run: upload_args.append('--cq-dry-run') if not comment: @@ -262,16 +262,7 @@ def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run, assert refactor_branch_upstream, \ "Branch %s must have an upstream." % refactor_branch - # Verify that the description contains a bug link. Examples: - # Bug: 123 - # Bug: chromium:456 - bug_pattern = re.compile(r"^Bug:\s*(?:[a-zA-Z]+:)?[0-9]+", re.MULTILINE) - matches = re.findall(bug_pattern, description) - answer = 'y' - if not matches: - answer = gclient_utils.AskForData( - 'Description does not include a bug link. Proceed? (y/n):') - if answer.lower() != 'y': + if not CheckDescriptionBugLink(description): return 0 files_split_by_reviewers = SelectReviewersForFiles(cl, author, files, @@ -327,6 +318,24 @@ def SplitCl(description_file, comment_file, changelist, cmd_upload, dry_run, return 0 +def CheckDescriptionBugLink(description): + """Verifies that the description contains a bug link. + + Examples: + Bug: 123 + Bug: chromium:456 + + Prompts user if the description does not contain a bug link. + """ + bug_pattern = re.compile(r"^Bug:\s*(?:[a-zA-Z]+:)?[0-9]+", re.MULTILINE) + matches = re.findall(bug_pattern, description) + answer = 'y' + if not matches: + answer = gclient_utils.AskForData( + 'Description does not include a bug link. Proceed? (y/n):') + return answer.lower() == 'y' + + def SelectReviewersForFiles(cl, author, files, max_depth): """Selects reviewers for passed-in files diff --git a/tests/split_cl_test.py b/tests/split_cl_test.py index 6e78bf1be1..d892daa6db 100755 --- a/tests/split_cl_test.py +++ b/tests/split_cl_test.py @@ -64,27 +64,124 @@ class SplitClTest(unittest.TestCase): EVERYONE="*") cl = mock.Mock(owners_client=owners_client) - files = [("M", "foo/owner1,owner2/a.txt"), ("M", "foo/owner1,owner2/b.txt"), - ("M", "bar/owner1,owner2/c.txt"), ("M", "bax/owner2/d.txt"), - ("M", "baz/owner3/e.txt")] + files = [("M", os.path.join("foo", "owner1,owner2", "a.txt")), + ("M", os.path.join("foo", "owner1,owner2", "b.txt")), + ("M", os.path.join("bar", "owner1,owner2", "c.txt")), + ("M", os.path.join("bax", "owner2", "d.txt")), + ("M", os.path.join("baz", "owner3", "e.txt"))] files_split_by_reviewers = split_cl.SelectReviewersForFiles( cl, "author", files, 0) self.assertEqual(3, len(files_split_by_reviewers.keys())) info1 = files_split_by_reviewers[tuple(["owner1", "owner2"])] - self.assertEqual(info1.files, [("M", "foo/owner1,owner2/a.txt"), - ("M", "foo/owner1,owner2/b.txt"), - ("M", "bar/owner1,owner2/c.txt")]) + self.assertEqual(info1.files, + [("M", os.path.join("foo", "owner1,owner2", "a.txt")), + ("M", os.path.join("foo", "owner1,owner2", "b.txt")), + ("M", os.path.join("bar", "owner1,owner2", "c.txt"))]) self.assertEqual(info1.owners_directories, ["foo/owner1,owner2", "bar/owner1,owner2"]) info2 = files_split_by_reviewers[tuple(["owner2"])] - self.assertEqual(info2.files, [("M", "bax/owner2/d.txt")]) + self.assertEqual(info2.files, + [("M", os.path.join("bax", "owner2", "d.txt"))]) self.assertEqual(info2.owners_directories, ["bax/owner2"]) info3 = files_split_by_reviewers[tuple(["owner3"])] - self.assertEqual(info3.files, [("M", "baz/owner3/e.txt")]) + self.assertEqual(info3.files, + [("M", os.path.join("baz", "owner3", "e.txt"))]) self.assertEqual(info3.owners_directories, ["baz/owner3"]) + class UploadClTester: + """Sets up test environment for testing split_cl.UploadCl()""" + def __init__(self, test): + self.mock_git_branches = self.StartPatcher("git_common.branches", test) + self.mock_git_branches.return_value = [] + self.mock_git_current_branch = self.StartPatcher( + "git_common.current_branch", test) + self.mock_git_current_branch.return_value = "branch_to_upload" + self.mock_git_run = self.StartPatcher("git_common.run", test) + self.mock_temporary_file = self.StartPatcher( + "gclient_utils.temporary_file", test) + self.mock_temporary_file().__enter__.return_value = "temporary_file0" + self.mock_file_writer = self.StartPatcher("gclient_utils.FileWrite", test) + + def StartPatcher(self, target, test): + patcher = mock.patch(target) + test.addCleanup(patcher.stop) + return patcher.start() + + def DoUploadCl(self, directories, files, reviewers, cmd_upload): + split_cl.UploadCl("branch_to_upload", "upstream_branch", + directories, files, "description", None, reviewers, + mock.Mock(), cmd_upload, True, True, "topic", + os.path.sep) + + def testUploadCl(self): + """Tests commands run by UploadCl.""" + + upload_cl_tester = self.UploadClTester(self) + + directories = ["dir0"] + files = [("M", os.path.join("bar", "a.cc")), + ("D", os.path.join("foo", "b.cc"))] + reviewers = {"reviewer1@gmail.com", "reviewer2@gmail.com"} + mock_cmd_upload = mock.Mock() + upload_cl_tester.DoUploadCl(directories, files, reviewers, mock_cmd_upload) + + abs_repository_path = os.path.abspath(os.path.sep) + mock_git_run = upload_cl_tester.mock_git_run + self.assertEqual(mock_git_run.call_count, 4) + mock_git_run.assert_has_calls([ + mock.call("checkout", "-t", "upstream_branch", "-b", + "branch_to_upload_dir0_split"), + mock.call("rm", os.path.join(abs_repository_path, "foo", "b.cc")), + mock.call("checkout", "branch_to_upload", "--", + os.path.join(abs_repository_path, "bar", "a.cc")), + mock.call("commit", "-F", "temporary_file0") + ]) + + expected_upload_args = [ + "-f", "-r", "reviewer1@gmail.com,reviewer2@gmail.com", "--cq-dry-run", + "--send-mail", "--enable-auto-submit", "--topic=topic" + ] + mock_cmd_upload.assert_called_once_with(expected_upload_args) + + def testDontUploadClIfBranchAlreadyExists(self): + """Tests that a CL is not uploaded if split branch already exists""" + + upload_cl_tester = self.UploadClTester(self) + upload_cl_tester.mock_git_branches.return_value = [ + "branch0", "branch_to_upload_dir0_split" + ] + + directories = ["dir0"] + files = [("M", os.path.join("bar", "a.cc")), + ("D", os.path.join("foo", "b.cc"))] + reviewers = {"reviewer1@gmail.com"} + mock_cmd_upload = mock.Mock() + upload_cl_tester.DoUploadCl(directories, files, reviewers, mock_cmd_upload) + + upload_cl_tester.mock_git_run.assert_not_called() + mock_cmd_upload.assert_not_called() + + @mock.patch("gclient_utils.AskForData") + def testCheckDescriptionBugLink(self, mock_ask_for_data): + # Description contains bug link. + self.assertTrue(split_cl.CheckDescriptionBugLink("Bug:1234")) + self.assertEqual(mock_ask_for_data.call_count, 0) + + # Description does not contain bug link. User does not enter 'y' when + # prompted. + mock_ask_for_data.reset_mock() + mock_ask_for_data.return_value = "m" + self.assertFalse(split_cl.CheckDescriptionBugLink("Description")) + self.assertEqual(mock_ask_for_data.call_count, 1) + + # Description does not contain bug link. User enters 'y' when prompted. + mock_ask_for_data.reset_mock() + mock_ask_for_data.return_value = "y" + self.assertTrue(split_cl.CheckDescriptionBugLink("Description")) + self.assertEqual(mock_ask_for_data.call_count, 1) + if __name__ == '__main__': unittest.main()