Refactoring: Redesign option parsing in push and merge scripts.

tools/push-to-trunk/test_scripts.py

 #!/usr/bin/env python
 # Copyright 2013 the V8 project authors. All rights reserved.
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
 # met:
 #
 #     * Redistributions of source code must retain the above copyright
 #       notice, this list of conditions and the following disclaimer.
 #     * Redistributions in binary form must reproduce the above
 #       copyright notice, this list of conditions and the following
@@ skipping 14 matching lines @@
 # THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
 # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 import os
 import tempfile
 import traceback
 import unittest
 
 import auto_roll
-from auto_roll import AutoRollOptions
 from auto_roll import CheckLastPush
 from auto_roll import FetchLatestRevision
 from auto_roll import SETTINGS_LOCATION
 import common_includes
 from common_includes import *
 import merge_to_branch
 from merge_to_branch import *
 import push_to_trunk
 from push_to_trunk import *
 
@@ skipping 11 matching lines @@
   COMMITMSG_FILE: "/tmp/test-v8-push-to-trunk-tempfile-commitmsg",
   CHROMIUM: "/tmp/test-v8-push-to-trunk-tempfile-chromium",
   DEPS_FILE: "/tmp/test-v8-push-to-trunk-tempfile-chromium/DEPS",
   SETTINGS_LOCATION: None,
   ALREADY_MERGING_SENTINEL_FILE:
       "/tmp/test-merge-to-branch-tempfile-already-merging",
   COMMIT_HASHES_FILE: "/tmp/test-merge-to-branch-tempfile-PATCH_COMMIT_HASHES",
   TEMPORARY_PATCH_FILE: "/tmp/test-merge-to-branch-tempfile-temporary-patch",
 }
 
-
-def MakeOptions(s=0, l=None, f=False, m=True, r=None, c=None, a=None,
-                status_password=None, revert_bleeding_edge=None, p=None):
-  """Convenience wrapper."""
-  class Options(object):
-      pass
-  options = Options()
-  options.s = s
-  options.l = l
-  options.f = f
-  options.m = m
-  options.reviewer = r
-  options.c = c
-  options.a = a
-  options.p = p
-  options.status_password = status_password
-  options.revert_bleeding_edge = revert_bleeding_edge
-  return options
+AUTO_ROLL_ARGS = [
+  "-a", "author@chromium.org",
+  "-c", TEST_CONFIG[CHROMIUM],
+  "-r", "reviewer@chromium.org",
+]
 
 
 class ToplevelTest(unittest.TestCase):
   def testMakeComment(self):
     self.assertEquals("#   Line 1\n#   Line 2\n#",
                       MakeComment("    Line 1\n    Line 2\n"))
     self.assertEquals("#Line 1\n#Line 2",
                       MakeComment("Line 1\n Line 2"))
 
   def testStripComments(self):
@@ skipping 193 matching lines @@
       f.write("  // Some line...\n")
       f.write("\n")
       f.write("#define MAJOR_VERSION    3\n")
       f.write("#define MINOR_VERSION    22\n")
       f.write("#define BUILD_NUMBER     5\n")
       f.write("#define PATCH_LEVEL      0\n")
       f.write("  // Some line...\n")
       f.write("#define IS_CANDIDATE_VERSION 0\n")
     return name
 
-  def MakeStep(self, step_class=Step, state=None, options=None):
+  def MakeStep(self):
     """Convenience wrapper."""
-    options = options or CommonOptions(MakeOptions())
-    state = state if state is not None else self._state
-    return MakeStep(step_class=step_class, number=0, state=state,
-                    config=TEST_CONFIG, options=options,
-                    side_effect_handler=self)
+    options = ScriptsBase(TEST_CONFIG, self, self._state).MakeOptions([])
+    return MakeStep(step_class=Step, state=self._state,
+                    config=TEST_CONFIG, side_effect_handler=self,
+                    options=options)
+
+  def RunStep(self, script=PushToTrunk, step_class=Step, args=None):
+    """Convenience wrapper."""
+    args = args or ["-m"]
+    return script(TEST_CONFIG, self, self._state).RunSteps([step_class], args)
 
   def GitMock(self, cmd, args="", pipe=True):
     print "%s %s" % (cmd, args)
     return self._git_mock.Call(args)
 
   def LogMock(self, cmd, args=""):
     print "Log: %s %s" % (cmd, args)
 
   MOCKS = {
     "git": GitMock,
@@ skipping 161 matching lines @@
       ["log -1 --format=%an rev4", "author4@chromium.org"],
     ])
 
     # The cl for rev4 on rietveld has an updated LOG flag.
     self.ExpectReadURL([
       ["https://codereview.chromium.org/9876543210/description",
        "Title\n\nBUG=456\nLOG=N\n\n"],
     ])
 
     self._state["last_push_bleeding_edge"] = "1234"
-    self.MakeStep(PrepareChangeLog).Run()
+    self.RunStep(PushToTrunk, PrepareChangeLog)
 
     actual_cl = FileToText(TEST_CONFIG[CHANGELOG_ENTRY_FILE])
 
     expected_cl = """1999-07-31: Version 3.22.5
 
         Title text 1.
 
         Title text 3 (Chromium issue 321).
 
         Performance and stability improvements on all platforms.
@@ skipping 26 matching lines @@
     TEST_CONFIG[CHANGELOG_ENTRY_FILE] = self.MakeEmptyTempFile()
     TEST_CONFIG[CHANGELOG_FILE] = self.MakeEmptyTempFile()
     TextToFile("        Original CL", TEST_CONFIG[CHANGELOG_FILE])
     TextToFile("  New  \n\tLines  \n", TEST_CONFIG[CHANGELOG_ENTRY_FILE])
     os.environ["EDITOR"] = "vi"
 
     self.ExpectReadline([
       "",  # Open editor.
     ])
 
-    self.MakeStep(EditChangeLog).Run()
-
+    self.RunStep(PushToTrunk, EditChangeLog)
     self.assertEquals("New\n        Lines\n\n\n        Original CL",
                       FileToText(TEST_CONFIG[CHANGELOG_FILE]))
 
   def testIncrementVersion(self):
     TEST_CONFIG[VERSION_FILE] = self.MakeTempVersionFile()
     self._state["build"] = "5"
 
     self.ExpectReadline([
       "Y",  # Increment build number.
     ])
 
-    self.MakeStep(IncrementVersion).Run()
-
+    self.RunStep(PushToTrunk, IncrementVersion)
     self.assertEquals("3", self._state["new_major"])
     self.assertEquals("22", self._state["new_minor"])
     self.assertEquals("6", self._state["new_build"])
     self.assertEquals("0", self._state["new_patch"])
 
   def testLastChangeLogEntries(self):
     TEST_CONFIG[CHANGELOG_FILE] = self.MakeEmptyTempFile()
     l = """
         Fixed something.
         (issue 1234)\n"""
@@ skipping 14 matching lines @@
       f.write(change_log)
 
     self.ExpectGit([
       ["diff svn/trunk hash1", "patch content"],
       ["svn find-rev hash1", "123455\n"],
     ])
 
     self._state["prepare_commit_hash"] = "hash1"
     self._state["date"] = "1999-11-11"
 
-    self.MakeStep(SquashCommits).Run()
+    self.RunStep(PushToTrunk, SquashCommits)
     self.assertEquals(FileToText(TEST_CONFIG[COMMITMSG_FILE]), expected_msg)
 
     patch = FileToText(TEST_CONFIG[ PATCH_FILE])
     self.assertTrue(re.search(r"patch content", patch))
 
   def testSquashCommitsUnformatted(self):
     change_log = """1999-11-11: Version 3.22.5
 
         Log text 1.
         Chromium issue 12345
@@ skipping 140 matching lines @@
     # Expected keyboard input in semi-automatic mode:
     if not manual and not force:
       self.ExpectReadline([
         "LGTM",  # Enter LGTM for V8 CL.
       ])
 
     # No keyboard input in forced mode:
     if force:
       self.ExpectReadline([])
 
-    options = MakeOptions(f=force, m=manual, a="author@chromium.org",
-                          r="reviewer@chromium.org" if not manual else None,
-                          c = TEST_CONFIG[CHROMIUM])
-    RunPushToTrunk(TEST_CONFIG, PushToTrunkOptions(options), self)
+    args = ["-a", "author@chromium.org", "-c", TEST_CONFIG[CHROMIUM]]
+    if force: args.append("-f")
+    if manual: args.append("-m")
+    else: args += ["-r", "reviewer@chromium.org"]
+    PushToTrunk(TEST_CONFIG, self).Run(args)
 
     deps = FileToText(TEST_CONFIG[DEPS_FILE])
     self.assertTrue(re.search("\"v8_revision\": \"123456\"", deps))
 
     cl = FileToText(TEST_CONFIG[CHANGELOG_FILE])
     self.assertTrue(re.search(r"^\d\d\d\d\-\d+\-\d+: Version 3\.22\.5", cl))
     self.assertTrue(re.search(r"        Log text 1 \(issue 321\).", cl))
     self.assertTrue(re.search(r"1999\-04\-05: Version 3\.22\.4", cl))
 
     # Note: The version file is on build number 5 again in the end of this test
     # since the git command that merges to the bleeding edge branch is mocked
     # out.
 
   def testPushToTrunkManual(self):
     self._PushToTrunk(manual=True)
 
   def testPushToTrunkSemiAutomatic(self):
     self._PushToTrunk()
 
   def testPushToTrunkForced(self):
     self._PushToTrunk(force=True)
 
   def testCheckLastPushRecently(self):
     self.ExpectGit([
       ["svn log -1 --oneline", "r101 | Text"],
       ["svn log -1 --oneline ChangeLog", "r99 | Prepare push to trunk..."],
     ])
 
-    state = {}
-    self.MakeStep(FetchLatestRevision, state=state).Run()
-    self.assertRaises(Exception, self.MakeStep(CheckLastPush, state=state).Run)
+    self.RunStep(auto_roll.AutoRoll, FetchLatestRevision, AUTO_ROLL_ARGS)
+    self.assertRaises(Exception, lambda: self.RunStep(auto_roll.AutoRoll,
+                                                      CheckLastPush,
+                                                      AUTO_ROLL_ARGS))
 
   def testAutoRoll(self):
-    status_password = self.MakeEmptyTempFile()
-    TextToFile("PW", status_password)
+    password = self.MakeEmptyTempFile()
+    TextToFile("PW", password)
     TEST_CONFIG[DOT_GIT_LOCATION] = self.MakeEmptyTempFile()
     TEST_CONFIG[SETTINGS_LOCATION] = "~/.doesnotexist"
 
     self.ExpectReadURL([
       ["https://v8-status.appspot.com/current?format=json",
        "{\"message\": \"Tree is throttled\"}"],
       ["https://v8-status.appspot.com/lkgr", Exception("Network problem")],
       ["https://v8-status.appspot.com/lkgr", "100"],
       ["https://v8-status.appspot.com/status",
        ("username=v8-auto-roll%40chromium.org&"
         "message=Tree+is+closed+%28preparing+to+push%29&password=PW"),
        ""],
       ["https://v8-status.appspot.com/status",
        ("username=v8-auto-roll%40chromium.org&"
         "message=Tree+is+throttled&password=PW"), ""],
     ])
 
     self.ExpectGit([
       ["status -s -uno", ""],
       ["status -s -b -uno", "## some_branch\n"],
       ["svn fetch", ""],
       ["svn log -1 --oneline", "r100 | Text"],
       [("log -1 --format=%H --grep=\""
         "^Version [[:digit:]]*\.[[:digit:]]*\.[[:digit:]]* (based\""
         " svn/trunk"), "push_hash\n"],
       ["svn find-rev push_hash", "65"],
     ])
 
-    auto_roll.RunAutoRoll(TEST_CONFIG, AutoRollOptions(
-        MakeOptions(status_password=status_password)), self)
+    auto_roll.AutoRoll(TEST_CONFIG, self).Run(
+        AUTO_ROLL_ARGS + ["--status-password", password])
 
     state = json.loads(FileToText("%s-state.json"
                                   % TEST_CONFIG[PERSISTFILE_BASENAME]))
 
     self.assertEquals("100", state["lkgr"])
     self.assertEquals("100", state["latest"])
 
   def testAutoRollStoppedBySettings(self):
     TEST_CONFIG[DOT_GIT_LOCATION] = self.MakeEmptyTempFile()
     TEST_CONFIG[SETTINGS_LOCATION] = self.MakeEmptyTempFile()
     TextToFile("{\"enable_auto_roll\": false}", TEST_CONFIG[SETTINGS_LOCATION])
 
     self.ExpectReadURL([])
 
     self.ExpectGit([
       ["status -s -uno", ""],
       ["status -s -b -uno", "## some_branch\n"],
       ["svn fetch", ""],
     ])
 
     def RunAutoRoll():
-      auto_roll.RunAutoRoll(TEST_CONFIG, AutoRollOptions(MakeOptions()), self)
+      auto_roll.AutoRoll(TEST_CONFIG, self).Run(AUTO_ROLL_ARGS)
     self.assertRaises(Exception, RunAutoRoll)
 
   def testAutoRollStoppedByTreeStatus(self):
     TEST_CONFIG[DOT_GIT_LOCATION] = self.MakeEmptyTempFile()
     TEST_CONFIG[SETTINGS_LOCATION] = "~/.doesnotexist"
 
     self.ExpectReadURL([
       ["https://v8-status.appspot.com/current?format=json",
        "{\"message\": \"Tree is throttled (no push)\"}"],
     ])
 
     self.ExpectGit([
       ["status -s -uno", ""],
       ["status -s -b -uno", "## some_branch\n"],
       ["svn fetch", ""],
     ])
 
     def RunAutoRoll():
-      auto_roll.RunAutoRoll(TEST_CONFIG, AutoRollOptions(MakeOptions()), self)
+      auto_roll.AutoRoll(TEST_CONFIG, self).Run(AUTO_ROLL_ARGS)
     self.assertRaises(Exception, RunAutoRoll)
 
   def testMergeToBranch(self):
     TEST_CONFIG[ALREADY_MERGING_SENTINEL_FILE] = self.MakeEmptyTempFile()
     TEST_CONFIG[DOT_GIT_LOCATION] = self.MakeEmptyTempFile()
     TEST_CONFIG[VERSION_FILE] = self.MakeTempVersionFile()
     os.environ["EDITOR"] = "vi"
     extra_patch = self.MakeEmptyTempFile()
 
     def VerifyPatch(patch):
@@ skipping 96 matching lines @@
       ["branch -D %s" % TEST_CONFIG[BRANCHNAME], ""],
     ])
 
     self.ExpectReadline([
       "Y",  # Automatically add corresponding ports (34567, 56789)?
       "Y",  # Automatically increment patch level?
       "reviewer@chromium.org",  # V8 reviewer.
       "LGTM",  # Enter LGTM for V8 CL.
     ])
 
-    options = MakeOptions(p=extra_patch, f=True)
     # r12345 and r34567 are patches. r23456 (included) and r45678 are the MIPS
     # ports of r12345. r56789 is the MIPS port of r34567.
-    args = ["trunk", "12345", "23456", "34567"]
-    self.assertTrue(merge_to_branch.ProcessOptions(options, args))
+    args = ["-f", "-p", extra_patch, "--branch", "trunk", "12345", "23456",
+            "34567"]
 
     # The first run of the script stops because of the svn being down.
     self.assertRaises(GitFailedException,
-        lambda: RunMergeToBranch(TEST_CONFIG,
-                                 MergeToBranchOptions(options, args),
-                                 self))
+        lambda: MergeToBranch(TEST_CONFIG, self).Run(args))
 
     # Test that state recovery after restarting the script works.
-    options.s = 3
-    RunMergeToBranch(TEST_CONFIG, MergeToBranchOptions(options, args), self)
+    args += ["-s", "3"]
+    MergeToBranch(TEST_CONFIG, self).Run(args)
 
 
 class SystemTest(unittest.TestCase):
   def testReload(self):
     step = MakeStep(step_class=PrepareChangeLog, number=0, state={}, config={},
-                    options=CommonOptions(MakeOptions()),
                     side_effect_handler=DEFAULT_SIDE_EFFECT_HANDLER)
     body = step.Reload(
 """------------------------------------------------------------------------
 r17997 | machenbach@chromium.org | 2013-11-22 11:04:04 +0100 (...) | 6 lines
 
 Prepare push to trunk.  Now working on version 3.23.11.
 
 R=danno@chromium.org
 
 Review URL: https://codereview.chromium.org/83173002
 
 ------------------------------------------------------------------------""")
     self.assertEquals(
 """Prepare push to trunk.  Now working on version 3.23.11.
 
 R=danno@chromium.org
 
 Committed: https://code.google.com/p/v8/source/detail?r=17997""", body)