LLVM tools: Updated unittests for patch_manager.py

BUG=None
TEST='./patch_manager_unittest.py' passes

Change-Id: Ib33236b725cf9ffad6cae622aac1ff154db09e0f
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/toolchain-utils/+/1796386
Reviewed-by: George Burgess <gbiv@chromium.org>
Reviewed-by: Manoj Gupta <manojgupta@chromium.org>
Tested-by: Salud Lemus <saludlemus@google.com>

1 files changed, 421 insertions, 314 deletions

diff --git a/llvm_tools/patch_manager_unittest.py b/llvm_tools/patch_manager_unittest.py
index d32327d7..90200ab8 100755
--- a/llvm_tools/patch_manager_unittest.py
+++ b/llvm_tools/patch_manager_unittest.py
@@ -1,4 +1,4 @@
-#!/usr/bin/env python2
+#!/usr/bin/env python3
 # -*- coding: utf-8 -*-
 # Copyright 2019 The Chromium OS Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
@@ -9,16 +9,16 @@
 from __future__ import print_function
 
 import json
-import mock
 import os
-import patch_manager
 import subprocess
 import unittest
+import unittest.mock as mock
 
 from failure_modes import FailureModes
 from test_helpers import CallCountsToMockFunctions
 from test_helpers import CreateTemporaryJsonFile
 from test_helpers import WritePrettyJsonFile
+import patch_manager
 
 
 class PatchManagerTest(unittest.TestCase):
@@ -27,14 +27,16 @@ class PatchManagerTest(unittest.TestCase):
   # Simulate behavior of 'os.path.isdir()' when the path is not a directory.
   @mock.patch.object(os.path, 'isdir', return_value=False)
   def testInvalidDirectoryPassedAsCommandLineArgument(self, mock_isdir):
+    test_dir = '/some/path/that/is/not/a/directory'
+
     # Verify the exception is raised when the command line argument for
     # '--filesdir_path' or '--src_path' is not a directory.
     with self.assertRaises(ValueError) as err:
-      patch_manager.is_directory('/some/path/that/is/not/a/directory')
+      patch_manager.is_directory(test_dir)
 
     self.assertEqual(
-        err.exception.message, 'Path is not a directory: '
-        '/some/path/that/is/not/a/directory')
+        str(err.exception), 'Path is not a directory: '
+        '%s' % test_dir)
 
     mock_isdir.assert_called_once()
 
@@ -42,9 +44,9 @@ class PatchManagerTest(unittest.TestCase):
   # passed as the command line argument for '--filesdir_path' or '--src_path'.
   @mock.patch.object(os.path, 'isdir', return_value=True)
   def testValidDirectoryPassedAsCommandLineArgument(self, mock_isdir):
-    self.assertEqual(
-        patch_manager.is_directory('/some/path/that/is/a/directory'),
-        '/some/path/that/is/a/directory')
+    test_dir = '/some/path/that/is/a/directory'
+
+    self.assertEqual(patch_manager.is_directory(test_dir), test_dir)
 
     mock_isdir.assert_called_once()
 
@@ -54,14 +56,16 @@ class PatchManagerTest(unittest.TestCase):
   def testInvalidPathToPatchMetadataFilePassedAsCommandLineArgument(
       self, mock_isfile):
 
+    abs_path_to_patch_file = '/abs/path/to/PATCHES.json'
+
     # Verify the exception is raised when the command line argument for
     # '--patch_metadata_file' does not exist or is not a file.
     with self.assertRaises(ValueError) as err:
-      patch_manager.is_patch_metadata_file('/abs/path/to/PATCHES.json')
+      patch_manager.is_patch_metadata_file(abs_path_to_patch_file)
 
     self.assertEqual(
-        err.exception.message, 'Invalid patch metadata file provided: '
-        '/abs/path/to/PATCHES.json')
+        str(err.exception), 'Invalid patch metadata file provided: '
+        '%s' % abs_path_to_patch_file)
 
     mock_isfile.assert_called_once()
 
@@ -69,15 +73,17 @@ class PatchManagerTest(unittest.TestCase):
   # metadata file exists and is a file.
   @mock.patch.object(os.path, 'isfile', return_value=True)
   def testPatchMetadataFileDoesNotEndInJson(self, mock_isfile):
+    abs_path_to_patch_file = '/abs/path/to/PATCHES'
+
     # Verify the exception is raises when the command line argument for
     # '--patch_metadata_file' exists and is a file but does not end in
     # '.json'.
     with self.assertRaises(ValueError) as err:
-      patch_manager.is_patch_metadata_file('/abs/path/to/PATCHES')
+      patch_manager.is_patch_metadata_file(abs_path_to_patch_file)
 
     self.assertEqual(
-        err.exception.message, 'Patch metadata file does not end in \'.json\': '
-        '/abs/path/to/PATCHES')
+        str(err.exception), 'Patch metadata file does not end in ".json": '
+        '%s' % abs_path_to_patch_file)
 
     mock_isfile.assert_called_once()
 
@@ -85,9 +91,11 @@ class PatchManagerTest(unittest.TestCase):
   # for '--patch_metadata_file' exists and is a file.
   @mock.patch.object(os.path, 'isfile', return_value=True)
   def testValidPatchMetadataFilePassedAsCommandLineArgument(self, mock_isfile):
+    abs_path_to_patch_file = '/abs/path/to/PATCHES.json'
+
     self.assertEqual(
-        patch_manager.is_patch_metadata_file('/abs/path/to/PATCHES.json'),
-        '/abs/path/to/PATCHES.json')
+        patch_manager.is_patch_metadata_file(abs_path_to_patch_file),
+        '%s' % abs_path_to_patch_file)
 
     mock_isfile.assert_called_once()
 
@@ -95,15 +103,18 @@ class PatchManagerTest(unittest.TestCase):
   # does not exist.
   @mock.patch.object(os.path, 'isdir', return_value=False)
   def testInvalidPathToFilesDirWhenConstructingPathToPatch(self, mock_isdir):
+    abs_path_to_filesdir = '/abs/path/to/filesdir'
+
+    rel_patch_path = 'cherry/fixes_stdout.patch'
+
     # Verify the exception is raised when the the absolute path to $FILESDIR of
     # a package is not a directory.
     with self.assertRaises(ValueError) as err:
-      patch_manager.GetPathToPatch('/abs/path/to/filesdir',
-                                   'cherry/fixes_stdout.patch')
+      patch_manager.GetPathToPatch(abs_path_to_filesdir, rel_patch_path)
 
     self.assertEqual(
-        err.exception.message, 'Invalid path to $FILESDIR provided: '
-        '/abs/path/to/filesdir')
+        str(err.exception), 'Invalid path to $FILESDIR provided: '
+        '%s' % abs_path_to_filesdir)
 
     mock_isdir.assert_called_once()
 
@@ -114,16 +125,20 @@ class PatchManagerTest(unittest.TestCase):
   # patch does not exist.
   @mock.patch.object(os.path, 'isfile', return_value=False)
   def testConstructedPathToPatchDoesNotExist(self, mock_isfile, mock_isdir):
+    abs_path_to_filesdir = '/abs/path/to/filesdir'
+
+    rel_patch_path = 'cherry/fixes_stdout.patch'
+
+    abs_patch_path = os.path.join(abs_path_to_filesdir, rel_patch_path)
+
     # Verify the exception is raised when the absolute path to the patch does
     # not exist.
     with self.assertRaises(ValueError) as err:
-      patch_manager.GetPathToPatch('/abs/path/to/filesdir',
-                                   'cherry/fixes_stdout.patch')
+      patch_manager.GetPathToPatch(abs_path_to_filesdir, rel_patch_path)
 
     self.assertEqual(
-        err.exception.message, 'The absolute path /abs/path/to/filesdir/cherry/'
-        'fixes_stdout.patch to the patch '
-        'cherry/fixes_stdout.patch does not exist')
+        str(err.exception), 'The absolute path %s to the patch %s does not '
+        'exist' % (abs_patch_path, rel_patch_path))
 
     mock_isdir.assert_called_once()
 
@@ -136,10 +151,15 @@ class PatchManagerTest(unittest.TestCase):
   # patch exists and is a file.
   @mock.patch.object(os.path, 'isfile', return_value=True)
   def testConstructedPathToPatchSuccessfully(self, mock_isfile, mock_isdir):
+    abs_path_to_filesdir = '/abs/path/to/filesdir'
+
+    rel_patch_path = 'cherry/fixes_stdout.patch'
+
+    abs_patch_path = os.path.join(abs_path_to_filesdir, rel_patch_path)
+
     self.assertEqual(
-        patch_manager.GetPathToPatch('/abs/path/to/filesdir',
-                                     'cherry/fixes_stdout.patch'),
-        '/abs/path/to/filesdir/cherry/fixes_stdout.patch')
+        patch_manager.GetPathToPatch(abs_path_to_filesdir, rel_patch_path),
+        abs_patch_path)
 
     mock_isdir.assert_called_once()
 
@@ -149,8 +169,8 @@ class PatchManagerTest(unittest.TestCase):
     expected_patch_metadata = 0, None, False
 
     test_patch = {
-        "comment": "Redirects output to stdout",
-        "rel_patch_path": "cherry/fixes_stdout.patch"
+        'comment': 'Redirects output to stdout',
+        'rel_patch_path': 'cherry/fixes_stdout.patch'
     }
 
     self.assertEqual(
@@ -160,37 +180,43 @@ class PatchManagerTest(unittest.TestCase):
     expected_patch_metadata = 0, 1000, False
 
     test_patch = {
-        "comment": "Redirects output to stdout",
-        "rel_patch_path": "cherry/fixes_stdout.patch",
-        "end_version": 1000
+        'comment': 'Redirects output to stdout',
+        'rel_patch_path': 'cherry/fixes_stdout.patch',
+        'end_version': 1000
     }
 
     self.assertEqual(
         patch_manager.GetPatchMetadata(test_patch), expected_patch_metadata)
 
   def testFailedToApplyPatchWhenInvalidSrcPathIsPassedIn(self):
+    src_path = '/abs/path/to/src'
+
+    abs_patch_path = '/abs/path/to/filesdir/cherry/fixes_stdout.patch'
+
     # Verify the exception is raised when the absolute path to the unpacked
     # sources of a package is not a directory.
     with self.assertRaises(ValueError) as err:
-      patch_manager.ApplyPatch(
-          '/abs/path/to/src', '/abs/path/to/filesdir/cherry/fixes_stdout.patch')
+      patch_manager.ApplyPatch(src_path, abs_patch_path)
 
-    self.assertEqual(err.exception.message,
-                     'Invalid src path provided: /abs/path/to/src')
+      self.assertEqual(
+          str(err.exception), 'Invalid src path provided: %s' % src_path)
 
   # Simulate behavior of 'os.path.isdir()' when the absolute path to the
   # unpacked sources of the package is valid and exists.
   @mock.patch.object(os.path, 'isdir', return_value=True)
   def testFailedToApplyPatchWhenPatchPathIsInvalid(self, mock_isdir):
+    src_path = '/abs/path/to/src'
+
+    abs_patch_path = '/abs/path/to/filesdir/cherry/fixes_stdout.patch'
+
     # Verify the exception is raised when the absolute path to the patch does
     # not exist or is not a file.
     with self.assertRaises(ValueError) as err:
-      patch_manager.ApplyPatch(
-          '/abs/path/to/src', '/abs/path/to/filesdir/cherry/fixes_stdout.patch')
+      patch_manager.ApplyPatch(src_path, abs_patch_path)
 
     self.assertEqual(
-        err.exception.message, 'Invalid patch file provided: '
-        '/abs/path/to/filesdir/cherry/fixes_stdout.patch')
+        str(err.exception), 'Invalid patch file provided: '
+        '%s' % abs_patch_path)
 
     mock_isdir.assert_called_once()
 
@@ -200,7 +226,7 @@ class PatchManagerTest(unittest.TestCase):
   @mock.patch.object(os.path, 'isfile', return_value=True)
   # Simulate behavior of 'os.path.isfile()' when the absolute path to the
   # patch exists and is a file.
-  @mock.patch.object(subprocess, 'check_output')
+  @mock.patch.object(patch_manager, 'check_output')
   def testFailedToApplyPatchInDryRun(self, mock_dry_run, mock_isfile,
                                      mock_isdir):
 
@@ -213,10 +239,11 @@ class PatchManagerTest(unittest.TestCase):
 
     mock_dry_run.side_effect = FailedToApplyPatch
 
-    self.assertEqual(
-        patch_manager.ApplyPatch(
-            '/abs/path/to/src', '/abs/path/to/filesdir/cherry/'
-            'fixes_stdout.patch'), False)
+    src_path = '/abs/path/to/src'
+
+    abs_patch_path = '/abs/path/to/filesdir/cherry/fixes_stdout.patch'
+
+    self.assertEqual(patch_manager.ApplyPatch(src_path, abs_patch_path), False)
 
     mock_isdir.assert_called_once()
 
@@ -230,13 +257,13 @@ class PatchManagerTest(unittest.TestCase):
   @mock.patch.object(os.path, 'isfile', return_value=True)
   # Simulate behavior of 'os.path.isfile()' when the absolute path to the
   # patch exists and is a file.
-  @mock.patch.object(subprocess, 'check_output')
+  @mock.patch.object(patch_manager, 'check_output')
   def testSuccessfullyAppliedPatch(self, mock_dry_run, mock_isfile, mock_isdir):
+    src_path = '/abs/path/to/src'
 
-    self.assertEqual(
-        patch_manager.ApplyPatch(
-            '/abs/path/to/src', '/abs/path/to/filesdir/cherry/'
-            'fixes_stdout.patch'), True)
+    abs_patch_path = '/abs/path/to/filesdir/cherry/fixes_stdout.patch'
+
+    self.assertEqual(patch_manager.ApplyPatch(src_path, abs_patch_path), True)
 
     mock_isdir.assert_called_once()
 
@@ -246,32 +273,33 @@ class PatchManagerTest(unittest.TestCase):
 
   def testFailedToUpdatePatchMetadataFileWhenPatchFileNotEndInJson(self):
     patch = [{
-        "comment": "Redirects output to stdout",
-        "rel_patch_path": "cherry/fixes_output.patch",
-        "start_version": 10
+        'comment': 'Redirects output to stdout',
+        'rel_patch_path': 'cherry/fixes_output.patch',
+        'start_version': 10
     }]
 
+    abs_patch_path = '/abs/path/to/filesdir/PATCHES'
+
     # Verify the exception is raised when the absolute path to the patch
     # metadata file does not end in '.json'.
     with self.assertRaises(ValueError) as err:
-      patch_manager.UpdatePatchMetadataFile('/abs/path/to/filesdir/PATCHES',
-                                            patch)
+      patch_manager.UpdatePatchMetadataFile(abs_patch_path, patch)
 
     self.assertEqual(
-        err.exception.message, 'File does not end in \'.json\': '
-        '/abs/path/to/filesdir/PATCHES')
+        str(err.exception), 'File does not end in ".json": '
+        '%s' % abs_patch_path)
 
   def testSuccessfullyUpdatedPatchMetadataFile(self):
     test_updated_patch_metadata = [{
-        "comment": "Redirects output to stdout",
-        "rel_patch_path": "cherry/fixes_output.patch",
-        "start_version": 10
+        'comment': 'Redirects output to stdout',
+        'rel_patch_path': 'cherry/fixes_output.patch',
+        'start_version': 10
     }]
 
     expected_patch_metadata = {
-        "comment": "Redirects output to stdout",
-        "rel_patch_path": "cherry/fixes_output.patch",
-        "start_version": 10
+        'comment': 'Redirects output to stdout',
+        'rel_patch_path': 'cherry/fixes_output.patch',
+        'start_version': 10
     }
 
     with CreateTemporaryJsonFile() as json_test_file:
@@ -281,7 +309,7 @@ class PatchManagerTest(unittest.TestCase):
       # Make sure the updated patch metadata was written into the temporary
       # .json file.
       with open(json_test_file) as patch_file:
-        patch_contents = patch_manager._ConvertToASCII(json.load(patch_file))
+        patch_contents = json.load(patch_file)
 
         self.assertEqual(len(patch_contents), 1)
 
@@ -289,19 +317,25 @@ class PatchManagerTest(unittest.TestCase):
 
   @mock.patch.object(patch_manager, 'GetPathToPatch')
   def testExceptionThrownWhenHandlingPatches(self, mock_get_path_to_patch):
+    filesdir_path = '/abs/path/to/filesdir'
+
+    abs_patch_path = '/abs/path/to/filesdir/cherry/fixes_output.patch'
+
+    rel_patch_path = 'cherry/fixes_output.patch'
+
     # Simulate behavior of 'GetPathToPatch()' when the absolute path to the
     # patch does not exist.
     def PathToPatchDoesNotExist(filesdir_path, rel_patch_path):
-      raise ValueError('The absolute path to /abs/path/to/filesdir/cherry/'
-                       'fix_output.patch does not exist')
+      raise ValueError(
+          'The absolute path to %s does not exist' % abs_patch_path)
 
     # Use the test function to simulate the behavior of 'GetPathToPatch()'.
     mock_get_path_to_patch.side_effect = PathToPatchDoesNotExist
 
     test_patch_metadata = [{
-        "comment": "Redirects output to stdout",
-        "rel_patch_path": "cherry/fixes_output.patch",
-        "start_version": 10
+        'comment': 'Redirects output to stdout',
+        'rel_patch_path': rel_patch_path,
+        'start_version': 10
     }]
 
     with CreateTemporaryJsonFile() as json_test_file:
@@ -309,20 +343,22 @@ class PatchManagerTest(unittest.TestCase):
       with open(json_test_file, 'w') as json_file:
         WritePrettyJsonFile(test_patch_metadata, json_file)
 
+      src_path = '/some/path/to/src'
+
+      revision = 1000
+
       # Verify the exception is raised when the absolute path to a patch does
       # not exist.
       with self.assertRaises(ValueError) as err:
-        patch_manager.HandlePatches(1000, json_test_file,
-                                    '/abs/path/to/filesdir',
-                                    '/some/path/to/src', FailureModes.FAIL)
+        patch_manager.HandlePatches(revision, json_test_file, filesdir_path,
+                                    src_path, FailureModes.FAIL)
 
     self.assertEqual(
-        err.exception.message,
-        'The absolute path to /abs/path/to/filesdir/cherry/'
-        'fix_output.patch does not exist')
+        str(err.exception),
+        'The absolute path to %s does not exist' % abs_patch_path)
 
-    mock_get_path_to_patch.assert_called_once_with('/abs/path/to/filesdir',
-                                                   'cherry/fixes_output.patch')
+    mock_get_path_to_patch.assert_called_once_with(filesdir_path,
+                                                   rel_patch_path)
 
   @mock.patch.object(patch_manager, 'GetPathToPatch')
   # Simulate behavior for 'ApplyPatch()' when an applicable patch failed to
@@ -330,15 +366,20 @@ class PatchManagerTest(unittest.TestCase):
   @mock.patch.object(patch_manager, 'ApplyPatch', return_value=False)
   def testExceptionThrownOnAFailedPatchInFailMode(self, mock_apply_patch,
                                                   mock_get_path_to_patch):
+    filesdir_path = '/abs/path/to/filesdir'
+
+    abs_patch_path = '/abs/path/to/filesdir/cherry/fixes_output.patch'
+
+    rel_patch_path = 'cherry/fixes_output.patch'
+
     # Simulate behavior for 'GetPathToPatch()' when successfully constructed the
     # absolute path to the patch and the patch exists.
-    mock_get_path_to_patch.return_value = ('/abs/path/to/filesdir/cherry/'
-                                           'fixes_output.patch')
+    mock_get_path_to_patch.return_value = abs_patch_path
 
     test_patch_metadata = [{
-        "comment": "Redirects output to stdout",
-        "rel_patch_path": "cherry/fixes_output.patch",
-        "start_version": 1000
+        'comment': 'Redirects output to stdout',
+        'rel_patch_path': rel_patch_path,
+        'start_version': 1000
     }]
 
     with CreateTemporaryJsonFile() as json_test_file:
@@ -346,86 +387,91 @@ class PatchManagerTest(unittest.TestCase):
       with open(json_test_file, 'w') as json_file:
         WritePrettyJsonFile(test_patch_metadata, json_file)
 
+      src_path = '/some/path/to/src'
+
+      revision = 1000
+
+      patch_name = 'fixes_output.patch'
+
       # Verify the exception is raised when the mode is 'fail' and an applicable
       # patch fails to apply.
       with self.assertRaises(ValueError) as err:
-        patch_manager.HandlePatches(1000, json_test_file,
-                                    '/abs/path/to/filesdir',
-                                    '/some/path/to/src', FailureModes.FAIL)
+        patch_manager.HandlePatches(revision, json_test_file, filesdir_path,
+                                    src_path, FailureModes.FAIL)
 
-    self.assertEqual(err.exception.message,
-                     'Failed to apply patch: fixes_output.patch')
+        self.assertEqual(
+            str(err.exception), 'Failed to apply patch: %s' % patch_name)
 
-    mock_get_path_to_patch.assert_called_once_with('/abs/path/to/filesdir',
-                                                   'cherry/fixes_output.patch')
+    mock_get_path_to_patch.assert_called_once_with(filesdir_path,
+                                                   rel_patch_path)
 
-    mock_apply_patch.assert_called_once_with(
-        '/some/path/to/src', '/abs/path/to/filesdir/cherry/'
-        'fixes_output.patch')
+    mock_apply_patch.assert_called_once_with(src_path, abs_patch_path)
 
   @mock.patch.object(patch_manager, 'GetPathToPatch')
   @mock.patch.object(patch_manager, 'ApplyPatch')
   def testSomePatchesFailedToApplyInContinueMode(self, mock_apply_patch,
                                                  mock_get_path_to_patch):
-    # Simulate behavior for 'GetPathToPatch()' when successfully constructed the
-    # absolute path to the patch and the patch exists.
-    @CallCountsToMockFunctions
-    def MultipleCallsToGetPatchPath(call_count, filesdir_path, rel_patch_path):
-      # First patch to call 'GetPathToPatch()'.
-      if call_count == 0:
-        self.assertEqual(filesdir_path, '/abs/path/to/filesdir')
-        self.assertEqual(rel_patch_path, 'cherry/fixes_output.patch')
 
-        return '/abs/path/to/filesdir/cherry/fixes_output.patch'
+    test_patch_1 = {
+        'comment': 'Redirects output to stdout',
+        'rel_patch_path': 'cherry/fixes_output.patch',
+        'start_version': 1000,
+        'end_version': 1250
+    }
 
-      # Second patch to call 'GetPathToPatch()'.
-      if call_count == 1:
-        self.assertEqual(filesdir_path, '/abs/path/to/filesdir')
-        self.assertEqual(rel_patch_path, 'cherry/fixes_input.patch')
+    test_patch_2 = {
+        'comment': 'Fixes input',
+        'rel_patch_path': 'cherry/fixes_input.patch',
+        'start_version': 1000
+    }
 
-        return '/abs/path/to/filesdir/cherry/fixes_input.patch'
+    test_patch_3 = {
+        'comment': 'Adds a warning',
+        'rel_patch_path': 'add_warning.patch',
+        'start_version': 750,
+        'end_version': 1500
+    }
 
-      # Third patch to call 'GetPathToPatch()'.
-      if call_count == 2:
-        self.assertEqual(filesdir_path, '/abs/path/to/filesdir')
-        self.assertEqual(rel_patch_path, 'add_warning.patch')
+    test_patch_4 = {
+        'comment': 'Adds a helper function',
+        'rel_patch_path': 'add_helper.patch',
+        'start_version': 20,
+        'end_version': 900
+    }
 
-        return '/abs/path/to/filesdir/add_warning.patch'
+    test_patch_metadata = [
+        test_patch_1, test_patch_2, test_patch_3, test_patch_4
+    ]
 
-      # Fourth (and last) patch to call 'GetPathToPatch()'.
-      if call_count == 3:
-        self.assertEqual(filesdir_path, '/abs/path/to/filesdir')
-        self.assertEqual(rel_patch_path, 'add_helper.patch')
+    abs_path_to_filesdir = '/abs/path/to/filesdir'
 
-        return '/abs/path/to/filesdir/add_helper.patch'
+    # Simulate behavior for 'GetPathToPatch()' when successfully constructed the
+    # absolute path to the patch and the patch exists.
+    @CallCountsToMockFunctions
+    def MultipleCallsToGetPatchPath(call_count, filesdir_path, rel_patch_path):
+      self.assertEqual(filesdir_path, abs_path_to_filesdir)
+
+      if call_count < 4:
+        self.assertEqual(rel_patch_path,
+                         test_patch_metadata[call_count]['rel_patch_path'])
+
+        return os.path.join(abs_path_to_filesdir,
+                            test_patch_metadata[call_count]['rel_patch_path'])
 
-      # 'GetPathToPatch()' was called more times than expected (4 times).
       assert False, 'Unexpectedly called more than 4 times.'
 
     # Simulate behavior for 'ApplyPatch()' when applying multiple applicable
     # patches.
     @CallCountsToMockFunctions
     def MultipleCallsToApplyPatches(call_count, src_path, path_to_patch):
-      # First applicable patch that tries to apply patch.
-      if call_count == 0:
-        self.assertEqual(path_to_patch,
-                         '/abs/path/to/filesdir/cherry/fixes_output.patch')
-
-        return True
-
-      # Second applicable patch that tries to apply patch.
-      if call_count == 1:
-        self.assertEqual(path_to_patch,
-                         '/abs/path/to/filesdir/cherry/fixes_input.patch')
-
-        return False
+      if call_count < 3:
+        self.assertEqual(
+            path_to_patch,
+            os.path.join(abs_path_to_filesdir,
+                         test_patch_metadata[call_count]['rel_patch_path']))
 
-      # Third applicable patch that tries to apply patch.
-      if call_count == 2:
-        self.assertEqual(path_to_patch,
-                         '/abs/path/to/filesdir/add_warning.patch')
-
-        return False
+        # Simulate that the first patch successfully applied.
+        return call_count == 0
 
       # 'ApplyPatch()' was called more times than expected (3 times).
       assert False, 'Unexpectedly called more than 3 times.'
@@ -434,37 +480,6 @@ class PatchManagerTest(unittest.TestCase):
     mock_get_path_to_patch.side_effect = MultipleCallsToGetPatchPath
     mock_apply_patch.side_effect = MultipleCallsToApplyPatches
 
-    test_patch_1 = {
-        "comment": "Redirects output to stdout",
-        "rel_patch_path": "cherry/fixes_output.patch",
-        "start_version": 1000,
-        "end_version": 1250
-    }
-
-    test_patch_2 = {
-        "comment": "Fixes input",
-        "rel_patch_path": "cherry/fixes_input.patch",
-        "start_version": 1000
-    }
-
-    test_patch_3 = {
-        "comment": "Adds a warning",
-        "rel_patch_path": "add_warning.patch",
-        "start_version": 750,
-        "end_version": 1500
-    }
-
-    test_patch_4 = {
-        "comment": "Adds a helper function",
-        "rel_patch_path": "add_helper.patch",
-        "start_version": 20,
-        "end_version": 900
-    }
-
-    test_patch_metadata = [
-        test_patch_1, test_patch_2, test_patch_3, test_patch_4
-    ]
-
     expected_applied_patches = ['fixes_output.patch']
     expected_failed_patches = ['fixes_input.patch', 'add_warning.patch']
     expected_non_applicable_patches = ['add_helper.patch']
@@ -483,9 +498,13 @@ class PatchManagerTest(unittest.TestCase):
       with open(json_test_file, 'w') as json_file:
         WritePrettyJsonFile(test_patch_metadata, json_file)
 
-      patch_info = patch_manager.HandlePatches(
-          1000, json_test_file, '/abs/path/to/filesdir', '/some/path/to/src',
-          FailureModes.CONTINUE)
+      src_path = '/some/path/to/src/'
+
+      revision = 1000
+
+      patch_info = patch_manager.HandlePatches(revision, json_test_file,
+                                               abs_path_to_filesdir, src_path,
+                                               FailureModes.CONTINUE)
 
     self.assertDictEqual(patch_info._asdict(), expected_patch_info_dict)
 
@@ -497,37 +516,52 @@ class PatchManagerTest(unittest.TestCase):
   @mock.patch.object(patch_manager, 'ApplyPatch')
   def testSomePatchesAreDisabled(self, mock_apply_patch,
                                  mock_get_path_to_patch):
-    # Simulate behavior for 'GetPathToPatch()' when successfully constructed the
-    # absolute path to the patch and the patch exists.
-    @CallCountsToMockFunctions
-    def MultipleCallsToGetPatchPath(call_count, filesdir_path, rel_patch_path):
-      # First patch to call 'GetPathToPatch()'.
-      if call_count == 0:
-        self.assertEqual(filesdir_path, '/abs/path/to/filesdir')
-        self.assertEqual(rel_patch_path, 'cherry/fixes_output.patch')
 
-        return '/abs/path/to/filesdir/cherry/fixes_output.patch'
+    test_patch_1 = {
+        'comment': 'Redirects output to stdout',
+        'rel_patch_path': 'cherry/fixes_output.patch',
+        'start_version': 1000,
+        'end_version': 1190
+    }
 
-      # Second patch to call 'GetPathToPatch()'.
-      if call_count == 1:
-        self.assertEqual(filesdir_path, '/abs/path/to/filesdir')
-        self.assertEqual(rel_patch_path, 'cherry/fixes_input.patch')
+    test_patch_2 = {
+        'comment': 'Fixes input',
+        'rel_patch_path': 'cherry/fixes_input.patch',
+        'start_version': 1000
+    }
 
-        return '/abs/path/to/filesdir/cherry/fixes_input.patch'
+    test_patch_3 = {
+        'comment': 'Adds a warning',
+        'rel_patch_path': 'add_warning.patch',
+        'start_version': 750,
+        'end_version': 1500
+    }
 
-      # Third patch to call 'GetPathToPatch()'.
-      if call_count == 2:
-        self.assertEqual(filesdir_path, '/abs/path/to/filesdir')
-        self.assertEqual(rel_patch_path, 'add_warning.patch')
+    test_patch_4 = {
+        'comment': 'Adds a helper function',
+        'rel_patch_path': 'add_helper.patch',
+        'start_version': 20,
+        'end_version': 2000
+    }
 
-        return '/abs/path/to/filesdir/add_warning.patch'
+    test_patch_metadata = [
+        test_patch_1, test_patch_2, test_patch_3, test_patch_4
+    ]
+
+    abs_path_to_filesdir = '/abs/path/to/filesdir'
 
-      # Fourth (and last) patch to call 'GetPathToPatch()'.
-      if call_count == 3:
-        self.assertEqual(filesdir_path, '/abs/path/to/filesdir')
-        self.assertEqual(rel_patch_path, 'add_helper.patch')
+    # Simulate behavior for 'GetPathToPatch()' when successfully constructed the
+    # absolute path to the patch and the patch exists.
+    @CallCountsToMockFunctions
+    def MultipleCallsToGetPatchPath(call_count, filesdir_path, rel_patch_path):
+      self.assertEqual(filesdir_path, abs_path_to_filesdir)
 
-        return '/abs/path/to/filesdir/add_helper.patch'
+      if call_count < 4:
+        self.assertEqual(rel_patch_path,
+                         test_patch_metadata[call_count]['rel_patch_path'])
+
+        return os.path.join(abs_path_to_filesdir,
+                            test_patch_metadata[call_count]['rel_patch_path'])
 
       # 'GetPathToPatch()' was called more times than expected (4 times).
       assert False, 'Unexpectedly called more than 4 times.'
@@ -536,26 +570,14 @@ class PatchManagerTest(unittest.TestCase):
     # patches.
     @CallCountsToMockFunctions
     def MultipleCallsToApplyPatches(call_count, src_path, path_to_patch):
-      # First applicable patch that tries to apply patch.
-      if call_count == 0:
-        self.assertEqual(path_to_patch,
-                         '/abs/path/to/filesdir/cherry/fixes_input.patch')
-
-        return False
-
-      # Second applicable patch that tries to apply patch.
-      if call_count == 1:
-        self.assertEqual(path_to_patch,
-                         '/abs/path/to/filesdir/add_warning.patch')
-
-        return True
+      if call_count < 3:
+        self.assertEqual(
+            path_to_patch,
+            os.path.join(abs_path_to_filesdir,
+                         test_patch_metadata[call_count + 1]['rel_patch_path']))
 
-      # Third applicable patch that tries to apply patch.
-      if call_count == 2:
-        self.assertEqual(path_to_patch,
-                         '/abs/path/to/filesdir/add_helper.patch')
-
-        return False
+        # Simulate that the second patch applied successfully.
+        return call_count == 1
 
       # 'ApplyPatch()' was called more times than expected (3 times).
       assert False, 'Unexpectedly called more than 3 times.'
@@ -564,37 +586,6 @@ class PatchManagerTest(unittest.TestCase):
     mock_get_path_to_patch.side_effect = MultipleCallsToGetPatchPath
     mock_apply_patch.side_effect = MultipleCallsToApplyPatches
 
-    test_patch_1 = {
-        "comment": "Redirects output to stdout",
-        "rel_patch_path": "cherry/fixes_output.patch",
-        "start_version": 1000,
-        "end_version": 1190
-    }
-
-    test_patch_2 = {
-        "comment": "Fixes input",
-        "rel_patch_path": "cherry/fixes_input.patch",
-        "start_version": 1000
-    }
-
-    test_patch_3 = {
-        "comment": "Adds a warning",
-        "rel_patch_path": "add_warning.patch",
-        "start_version": 750,
-        "end_version": 1500
-    }
-
-    test_patch_4 = {
-        "comment": "Adds a helper function",
-        "rel_patch_path": "add_helper.patch",
-        "start_version": 20,
-        "end_version": 2000
-    }
-
-    test_patch_metadata = [
-        test_patch_1, test_patch_2, test_patch_3, test_patch_4
-    ]
-
     expected_applied_patches = ['add_warning.patch']
     expected_failed_patches = ['fixes_input.patch', 'add_helper.patch']
     expected_disabled_patches = ['fixes_input.patch', 'add_helper.patch']
@@ -622,9 +613,13 @@ class PatchManagerTest(unittest.TestCase):
 
       expected_patch_info_dict['modified_metadata'] = json_test_file
 
-      patch_info = patch_manager.HandlePatches(
-          1200, json_test_file, '/abs/path/to/filesdir', '/some/path/to/src',
-          FailureModes.DISABLE_PATCHES)
+      src_path = '/some/path/to/src/'
+
+      revision = 1200
+
+      patch_info = patch_manager.HandlePatches(revision, json_test_file,
+                                               abs_path_to_filesdir, src_path,
+                                               FailureModes.DISABLE_PATCHES)
 
       self.assertDictEqual(patch_info._asdict(), expected_patch_info_dict)
 
@@ -642,13 +637,9 @@ class PatchManagerTest(unittest.TestCase):
       # Make sure the updated patch metadata was written into the temporary
       # .json file.
       with open(json_test_file) as patch_file:
-        new_json_file_contents = patch_manager._ConvertToASCII(
-            json.load(patch_file))
-
-        self.assertEqual(len(new_json_file_contents), 4)
+        new_json_file_contents = json.load(patch_file)
 
-        for i in range(4):
-          self.assertDictEqual(new_json_file_contents[i], expected_json_file[i])
+        self.assertListEqual(new_json_file_contents, expected_json_file)
 
     self.assertEqual(mock_get_path_to_patch.call_count, 4)
 
@@ -657,52 +648,146 @@ class PatchManagerTest(unittest.TestCase):
   @mock.patch.object(patch_manager, 'GetPathToPatch')
   @mock.patch.object(patch_manager, 'ApplyPatch')
   def testSomePatchesAreRemoved(self, mock_apply_patch, mock_get_path_to_patch):
+    # For the 'remove_patches' mode, this patch is expected to be in the
+    # 'non_applicable_patches' list and 'removed_patches' list because
+    # the 'svn_version' (1500) >= 'end_version' (1190).
+    test_patch_1 = {
+        'comment': 'Redirects output to stdout',
+        'rel_patch_path': 'cherry/fixes_output.patch',
+        'start_version': 1000,
+        'end_version': 1190
+    }
+
+    # For the 'remove_patches' mode, this patch is expected to be in the
+    # 'applicable_patches' list (which is the list that the .json file will be
+    # updated with) because the 'svn_version' < 'inf' (this patch does not have
+    # an 'end_version' value which implies 'end_version' == 'inf').
+    test_patch_2 = {
+        'comment': 'Fixes input',
+        'rel_patch_path': 'cherry/fixes_input.patch',
+        'start_version': 1000
+    }
+
+    # For the 'remove_patches' mode, this patch is expected to be in the
+    # 'non_applicable_patches' list and 'removed_patches' list because
+    # the 'svn_version' (1500) >= 'end_version' (1500).
+    test_patch_3 = {
+        'comment': 'Adds a warning',
+        'rel_patch_path': 'add_warning.patch',
+        'start_version': 750,
+        'end_version': 1500
+    }
+
+    # For the 'remove_patches' mode, this patch is expected to be in the
+    # 'non_applicable_patches' list and 'removed_patches' list because
+    # the 'svn_version' (1500) >= 'end_version' (1400).
+    test_patch_4 = {
+        'comment': 'Adds a helper function',
+        'rel_patch_path': 'add_helper.patch',
+        'start_version': 20,
+        'end_version': 1400
+    }
+
+    test_patch_metadata = [
+        test_patch_1, test_patch_2, test_patch_3, test_patch_4
+    ]
+
+    abs_path_to_filesdir = '/abs/path/to/filesdir'
+
     # Simulate behavior for 'GetPathToPatch()' when successfully constructed the
     # absolute path to the patch and the patch exists.
     @CallCountsToMockFunctions
     def MultipleCallsToGetPatchPath(call_count, filesdir_path, rel_patch_path):
-      # First patch to call 'GetPathToPatch()'.
-      if call_count == 0:
-        self.assertEqual(filesdir_path, '/abs/path/to/filesdir')
-        self.assertEqual(rel_patch_path, 'cherry/fixes_output.patch')
+      self.assertEqual(filesdir_path, abs_path_to_filesdir)
 
-        return '/abs/path/to/filesdir/cherry/fixes_output.patch'
+      if call_count < 4:
+        self.assertEqual(rel_patch_path,
+                         test_patch_metadata[call_count]['rel_patch_path'])
 
-      # Second patch to call 'GetPathToPatch()'.
-      if call_count == 1:
-        self.assertEqual(filesdir_path, '/abs/path/to/filesdir')
-        self.assertEqual(rel_patch_path, 'cherry/fixes_input.patch')
+        return os.path.join(abs_path_to_filesdir,
+                            test_patch_metadata[call_count]['rel_patch_path'])
 
-        return '/abs/path/to/filesdir/cherry/fixes_input.patch'
+      assert False, 'Unexpectedly called more than 4 times.'
 
-      # Third patch to call 'GetPathToPatch()'.
-      if call_count == 2:
-        self.assertEqual(filesdir_path, '/abs/path/to/filesdir')
-        self.assertEqual(rel_patch_path, 'add_warning.patch')
+    # Use the test function to simulate behavior of 'GetPathToPatch()'.
+    mock_get_path_to_patch.side_effect = MultipleCallsToGetPatchPath
 
-        return '/abs/path/to/filesdir/add_warning.patch'
+    expected_applied_patches = []
+    expected_failed_patches = []
+    expected_disabled_patches = []
+    expected_non_applicable_patches = [
+        'fixes_output.patch', 'add_warning.patch', 'add_helper.patch'
+    ]
+    expected_removed_patches = [
+        '/abs/path/to/filesdir/cherry/fixes_output.patch',
+        '/abs/path/to/filesdir/add_warning.patch',
+        '/abs/path/to/filesdir/add_helper.patch'
+    ]
 
-      # Fourth (and last) patch to call 'GetPathToPatch()'.
-      if call_count == 3:
-        self.assertEqual(filesdir_path, '/abs/path/to/filesdir')
-        self.assertEqual(rel_patch_path, 'add_helper.patch')
+    # Assigned 'None' for now, but it is expected that the patch metadata file
+    # will be modified, so the 'expected_patch_info_dict's' value for the
+    # key 'modified_metadata' will get updated to the temporary .json file once
+    # the file is created.
+    expected_modified_metadata_file = None
 
-        return '/abs/path/to/filesdir/add_helper.patch'
+    expected_patch_info_dict = {
+        'applied_patches': expected_applied_patches,
+        'failed_patches': expected_failed_patches,
+        'non_applicable_patches': expected_non_applicable_patches,
+        'disabled_patches': expected_disabled_patches,
+        'removed_patches': expected_removed_patches,
+        'modified_metadata': expected_modified_metadata_file
+    }
 
-      # 'GetPathToPatch()' was called more times than expected (4 times).
-      assert False, 'Unexpectedly called more than 4 times.'
+    with CreateTemporaryJsonFile() as json_test_file:
+      # Write the test patch metadata to the temporary .json file.
+      with open(json_test_file, 'w') as json_file:
+        WritePrettyJsonFile(test_patch_metadata, json_file)
 
-    # Use the test function to simulate behavior of 'GetPathToPatch()'.
-    mock_get_path_to_patch.side_effect = MultipleCallsToGetPatchPath
+      expected_patch_info_dict['modified_metadata'] = json_test_file
+
+      abs_path_to_filesdir = '/abs/path/to/filesdir'
+
+      src_path = '/some/path/to/src/'
+
+      revision = 1500
+
+      patch_info = patch_manager.HandlePatches(revision, json_test_file,
+                                               abs_path_to_filesdir, src_path,
+                                               FailureModes.REMOVE_PATCHES)
+
+      self.assertDictEqual(patch_info._asdict(), expected_patch_info_dict)
+
+      # 'test_patch_2' was an applicable patch, so this patch will be the only
+      # patch that is in temporary .json file. The other patches were not
+      # applicable (they failed the applicable check), so they will not be in
+      # the .json file.
+      expected_json_file = [test_patch_2]
+
+      # Make sure the updated patch metadata was written into the temporary
+      # .json file.
+      with open(json_test_file) as patch_file:
+        new_json_file_contents = json.load(patch_file)
+
+        self.assertListEqual(new_json_file_contents, expected_json_file)
+
+    self.assertEqual(mock_get_path_to_patch.call_count, 4)
+
+    mock_apply_patch.assert_not_called()
+
+  @mock.patch.object(patch_manager, 'GetPathToPatch')
+  @mock.patch.object(patch_manager, 'ApplyPatch')
+  def testSuccessfullyDidNotRemoveAFuturePatch(self, mock_apply_patch,
+                                               mock_get_path_to_patch):
 
     # For the 'remove_patches' mode, this patch is expected to be in the
     # 'non_applicable_patches' list and 'removed_patches' list because
-    # the 'svn_version' (1500) >= 'end_version' (1190).
+    # the 'svn_version' (1200) >= 'end_version' (1190).
     test_patch_1 = {
-        "comment": "Redirects output to stdout",
-        "rel_patch_path": "cherry/fixes_output.patch",
-        "start_version": 1000,
-        "end_version": 1190
+        'comment': 'Redirects output to stdout',
+        'rel_patch_path': 'cherry/fixes_output.patch',
+        'start_version': 1000,
+        'end_version': 1190
     }
 
     # For the 'remove_patches' mode, this patch is expected to be in the
@@ -710,45 +795,66 @@ class PatchManagerTest(unittest.TestCase):
     # updated with) because the 'svn_version' < 'inf' (this patch does not have
     # an 'end_version' value which implies 'end_version' == 'inf').
     test_patch_2 = {
-        "comment": "Fixes input",
-        "rel_patch_path": "cherry/fixes_input.patch",
-        "start_version": 1000
+        'comment': 'Fixes input',
+        'rel_patch_path': 'cherry/fixes_input.patch',
+        'start_version': 1000
     }
 
     # For the 'remove_patches' mode, this patch is expected to be in the
-    # 'non_applicable_patches' list and 'removed_patches' list because
-    # the 'svn_version' (1500) >= 'end_version' (1500).
+    # 'applicable_patches' list because 'svn_version' >= 'start_version' and
+    # 'svn_version' < 'end_version'.
     test_patch_3 = {
-        "comment": "Adds a warning",
-        "rel_patch_path": "add_warning.patch",
-        "start_version": 750,
-        "end_version": 1500
+        'comment': 'Adds a warning',
+        'rel_patch_path': 'add_warning.patch',
+        'start_version': 750,
+        'end_version': 1500
     }
 
     # For the 'remove_patches' mode, this patch is expected to be in the
-    # 'non_applicable_patches' list and 'removed_patches' list because
-    # the 'svn_version' (1500) >= 'end_version' (1400).
+    # 'applicable_patches' list because the patch is from the future (e.g.
+    # 'start_version' > 'svn_version' (1200), so it should NOT be removed.
     test_patch_4 = {
-        "comment": "Adds a helper function",
-        "rel_patch_path": "add_helper.patch",
-        "start_version": 20,
-        "end_version": 1400
+        'comment': 'Adds a helper function',
+        'rel_patch_path': 'add_helper.patch',
+        'start_version': 1600,
+        'end_version': 2000
     }
 
     test_patch_metadata = [
         test_patch_1, test_patch_2, test_patch_3, test_patch_4
     ]
 
+    abs_path_to_filesdir = '/abs/path/to/filesdir'
+
+    # Simulate behavior for 'GetPathToPatch()' when successfully constructed the
+    # absolute path to the patch and the patch exists.
+    @CallCountsToMockFunctions
+    def MultipleCallsToGetPatchPath(call_count, filesdir_path, rel_patch_path):
+      self.assertEqual(filesdir_path, abs_path_to_filesdir)
+
+      if call_count < 4:
+        self.assertEqual(rel_patch_path,
+                         test_patch_metadata[call_count]['rel_patch_path'])
+
+        return os.path.join(abs_path_to_filesdir,
+                            test_patch_metadata[call_count]['rel_patch_path'])
+
+      # 'GetPathToPatch()' was called more times than expected (4 times).
+      assert False, 'Unexpectedly called more than 4 times.'
+
+    # Use the test function to simulate behavior of 'GetPathToPatch()'.
+    mock_get_path_to_patch.side_effect = MultipleCallsToGetPatchPath
+
     expected_applied_patches = []
     expected_failed_patches = []
     expected_disabled_patches = []
-    expected_non_applicable_patches = [
-        'fixes_output.patch', 'add_warning.patch', 'add_helper.patch'
-    ]
+
+    # 'add_helper.patch' is still a 'non applicable' patch meaning it does not
+    # apply in revision 1200 but it will NOT be removed because it is a future
+    # patch.
+    expected_non_applicable_patches = ['fixes_output.patch', 'add_helper.patch']
     expected_removed_patches = [
-        '/abs/path/to/filesdir/cherry/fixes_output.patch',
-        '/abs/path/to/filesdir/add_warning.patch',
-        '/abs/path/to/filesdir/add_helper.patch'
+        '/abs/path/to/filesdir/cherry/fixes_output.patch'
     ]
 
     # Assigned 'None' for now, but it is expected that the patch metadata file
@@ -773,9 +879,13 @@ class PatchManagerTest(unittest.TestCase):
 
       expected_patch_info_dict['modified_metadata'] = json_test_file
 
-      patch_info = patch_manager.HandlePatches(
-          1500, json_test_file, '/abs/path/to/filesdir', '/some/path/to/src',
-          FailureModes.REMOVE_PATCHES)
+      src_path = '/some/path/to/src/'
+
+      revision = 1200
+
+      patch_info = patch_manager.HandlePatches(revision, json_test_file,
+                                               abs_path_to_filesdir, src_path,
+                                               FailureModes.REMOVE_PATCHES)
 
       self.assertDictEqual(patch_info._asdict(), expected_patch_info_dict)
 
@@ -783,17 +893,14 @@ class PatchManagerTest(unittest.TestCase):
       # patch that is in temporary .json file. The other patches were not
       # applicable (they failed the applicable check), so they will not be in
       # the .json file.
-      expected_json_file = [test_patch_2]
+      expected_json_file = [test_patch_2, test_patch_3, test_patch_4]
 
       # Make sure the updated patch metadata was written into the temporary
       # .json file.
       with open(json_test_file) as patch_file:
-        new_json_file_contents = patch_manager._ConvertToASCII(
-            json.load(patch_file))
-
-        self.assertEqual(len(new_json_file_contents), 1)
+        new_json_file_contents = json.load(patch_file)
 
-        self.assertDictEqual(new_json_file_contents[0], expected_json_file[0])
+        self.assertListEqual(new_json_file_contents, expected_json_file)
 
     self.assertEqual(mock_get_path_to_patch.call_count, 4)