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 <sokcevic@chromium.org>
Commit-Queue: Peter Kotwicz <pkotwicz@chromium.org>

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
 

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()