presubmit_support: Fix tests on Python 3.

The problems on crbug.com/1017367 should have been fixed by crrev.com/c/1880013

Bug: 1009814
Change-Id: Ia98304ca983b4e7e293fedb9df1fee589de58ba9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1887714
Reviewed-by: Anthony Polito <apolito@google.com>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

git_cl.py

@@ -15,7 +15,6 @@ import base64
 import collections
 import datetime
 import glob
-import httplib
 import httplib2
 import itertools
 import json
@@ -30,9 +29,6 @@ import sys
 import tempfile
 import textwrap
 import time
-import urllib
-import urllib2
-import urlparse
 import uuid
 import webbrowser
 import zlib
@@ -58,6 +54,17 @@ import subcommand
 import subprocess2
 import watchlists
 
+if sys.version_info.major == 2:
+  import httplib
+  import urllib2 as urllib_request
+  import urllib2 as urllib_error
+  import urlparse
+else:
+  import http.client as httplib
+  import urllib.request as urllib_request
+  import urllib.error as urllib_error
+  import urllib.parse as urlparse
+
 __version__ = '2.0'
 
 # Traces for git push will be stored in a traces directory inside the
@@ -3186,7 +3193,7 @@ def urlretrieve(source, destination):
   This is necessary because urllib is broken for SSL connections via a proxy.
   """
   with open(destination, 'w') as f:
-    f.write(urllib2.urlopen(source).read())
+    f.write(urllib_request.urlopen(source).read())
 
 
 def hasSheBang(fname):
@@ -4659,7 +4666,7 @@ def GetTreeStatus(url=None):
   'unknown' or 'unset'."""
   url = url or settings.GetTreeStatusUrl(error_ok=True)
   if url:
-    status = urllib2.urlopen(url).read().lower()
+    status = urllib_request.urlopen(url).read().lower()
     if status.find('closed') != -1 or status == '0':
       return 'closed'
     elif status.find('open') != -1 or status == '1':
@@ -4673,7 +4680,7 @@ def GetTreeStatusReason():
   with the reason for the tree to be opened or closed."""
   url = settings.GetTreeStatusUrl()
   json_url = urlparse.urljoin(url, '/current?format=json')
-  connection = urllib2.urlopen(json_url)
+  connection = urllib_request.urlopen(json_url)
   status = json.loads(connection.read())
   connection.close()
   return status['message']
@@ -5462,7 +5469,7 @@ def main(argv):
     return dispatcher.execute(OptionParser(), argv)
   except auth.LoginRequiredError as e:
     DieWithError(str(e))
-  except urllib2.HTTPError as e:
+  except urllib_error.HTTPError as e:
     if e.code != 500:
       raise
     DieWithError(

presubmit_canned_checks.py

@@ -251,7 +251,7 @@ def CheckGenderNeutral(input_api, output_api, source_file_filter=None):
   submitted.
   """
   gendered_re = input_api.re.compile(
-      '(^|\s|\(|\[)([Hh]e|[Hh]is|[Hh]ers?|[Hh]im|[Ss]he|[Gg]uys?)\\b')
+      r'(^|\s|\(|\[)([Hh]e|[Hh]is|[Hh]ers?|[Hh]im|[Ss]he|[Gg]uys?)\\b')
 
   errors = []
   for f in input_api.AffectedFiles(include_deletes=False,
@@ -576,7 +576,7 @@ def CheckTreeIsOpen(input_api, output_api,
     return []
   try:
     if json_url:
-      connection = input_api.urllib2.urlopen(json_url)
+      connection = input_api.urllib_request.urlopen(json_url)
       status = input_api.json.loads(connection.read())
       connection.close()
       if not status['can_commit_freely']:
@@ -585,7 +585,7 @@ def CheckTreeIsOpen(input_api, output_api,
         return [output_api.PresubmitError(short_text, long_text=long_text)]
     else:
       # TODO(bradnelson): drop this once all users are gone.
-      connection = input_api.urllib2.urlopen(url)
+      connection = input_api.urllib_request.urlopen(url)
       status = connection.read()
       connection.close()
       if input_api.re.match(closed, status):
@@ -752,10 +752,10 @@ def GetPythonUnitTests(input_api, output_api, unit_tests):
       # We convert to str, since on Windows on Python 2 only strings are allowed
       # as environment variables, but literals are unicode since we're importing
       # unicode_literals from __future__.
-      if env.get(str('PYTHONPATH')):
+      if env.get('PYTHONPATH'):
-        backpath.append(env.get(str('PYTHONPATH')))
+        backpath.append(env.get('PYTHONPATH'))
-      env[str('PYTHONPATH')] = input_api.os_path.pathsep.join((backpath))
+      env['PYTHONPATH'] = input_api.os_path.pathsep.join((backpath))
-      env.pop(str('VPYTHON_CLEAR_PYTHONPATH'), None)
+      env.pop('VPYTHON_CLEAR_PYTHONPATH', None)
     cmd = [input_api.python_executable, '-m', '%s' % unit_test]
     results.append(input_api.Command(
         name=unit_test_name,
@@ -830,7 +830,7 @@ def GetPylint(input_api, output_api, white_list=None, black_list=None,
 
   The default white_list enforces looking only at *.py files.
   """
-  white_list = tuple(white_list or ('.*\.py$',))
+  white_list = tuple(white_list or (r'.*\.py$',))
   black_list = tuple(black_list or input_api.DEFAULT_BLACK_LIST)
   extra_paths_list = extra_paths_list or []
 
@@ -876,8 +876,7 @@ def GetPylint(input_api, output_api, white_list=None, black_list=None,
   input_api.logging.info('Running pylint on %d files', len(files))
   input_api.logging.debug('Running pylint on: %s', files)
   env = input_api.environ.copy()
-  env['PYTHONPATH'] = input_api.os_path.pathsep.join(
+  env['PYTHONPATH'] = input_api.os_path.pathsep.join(extra_paths_list)
-    extra_paths_list).encode('utf8')
   env.pop('VPYTHON_CLEAR_PYTHONPATH', None)
   input_api.logging.debug('  with extra PYTHONPATH: %r', extra_paths_list)
 
@@ -945,7 +944,7 @@ def RunPylint(input_api, *args, **kwargs):
 def CheckBuildbotPendingBuilds(input_api, output_api, url, max_pendings,
     ignored):
   try:
-    connection = input_api.urllib2.urlopen(url)
+    connection = input_api.urllib_request.urlopen(url)
     raw_data = connection.read()
     connection.close()
   except IOError:
@@ -1010,7 +1009,7 @@ def CheckOwners(input_api, output_api, source_file_filter=None):
       input_api.change.RepositoryRoot(),
       owner_email,
       reviewers,
-      fopen=file,
+      fopen=open,
       os_path=input_api.os_path,
       email_postfix='',
       disable_color=True,
@@ -1135,7 +1134,7 @@ def PanProjectChecks(input_api, output_api,
   # 2006-20xx string used on the oldest files. 2006-20xx is deprecated, but
   # tolerated on old files.
   current_year = int(input_api.time.strftime('%Y'))
-  allowed_years = (str(s) for s in reversed(xrange(2006, current_year + 1)))
+  allowed_years = (str(s) for s in reversed(range(2006, current_year + 1)))
   years_re = '(' + '|'.join(allowed_years) + '|2006-2008|2006-2009|2006-2010)'
 
   # The (c) is deprecated, but tolerate it until it's removed from all files.
@@ -1164,7 +1163,10 @@ def PanProjectChecks(input_api, output_api,
   snapshot_memory = []
   def snapshot(msg):
     """Measures & prints performance warning if a rule is running slow."""
+    if input_api.sys.version_info.major == 2:
       dt2 = input_api.time.clock()
+    else:
+      dt2 = input_api.time.process_time()
     if snapshot_memory:
       delta_ms = int(1000*(dt2 - snapshot_memory[0]))
       if delta_ms > 500:
@@ -1397,7 +1399,6 @@ def CheckChangedLUCIConfigs(input_api, output_api):
   import base64
   import json
   import logging
-  import urllib2
 
   import auth
   import git_cl
@@ -1434,16 +1435,16 @@ def CheckChangedLUCIConfigs(input_api, output_api):
   def request(endpoint, body=None):
     api_url = ('https://%s/_ah/api/config/v1/%s'
                % (LUCI_CONFIG_HOST_NAME, endpoint))
-    req = urllib2.Request(api_url)
+    req = input_api.urllib_request.Request(api_url)
     req.add_header('Authorization', 'Bearer %s' % acc_tkn.token)
     if body is not None:
       req.add_header('Content-Type', 'application/json')
       req.add_data(json.dumps(body))
-    return json.load(urllib2.urlopen(req))
+    return json.load(input_api.urllib_request.urlopen(req))
 
   try:
     config_sets = request('config-sets').get('config_sets')
-  except urllib2.HTTPError as e:
+  except input_api.urllib_error.HTTPError as e:
     return [output_api.PresubmitError(
         'Config set request to luci-config failed', long_text=str(e))]
   if not config_sets:
@@ -1481,7 +1482,7 @@ def CheckChangedLUCIConfigs(input_api, output_api):
         cs_to_files[cs].append({
           'path': file_path[len(dr):] if dr != '/' else file_path,
           'content': base64.b64encode(
-              '\n'.join(f.NewContents()).encode('utf-8'))
+              '\n'.join(f.NewContents()).encode('utf-8')).decode('utf-8')
         })
   outputs = []
   for cs, f in cs_to_files.items():
@@ -1489,7 +1490,7 @@ def CheckChangedLUCIConfigs(input_api, output_api):
       # TODO(myjang): parallelize
       res = request(
           'validate-config', body={'config_set': cs, 'files': f})
-    except urllib2.HTTPError as e:
+    except input_api.urllib_error.HTTPError as e:
       return [output_api.PresubmitError(
           'Validation request to luci-config failed', long_text=str(e))]
     for msg in res.get('messages', []):

presubmit_support.py

@@ -33,7 +33,6 @@ import sys  # Parts exposed through API.
 import tempfile  # Exposed through the API.
 import threading
 import time
-import types
 import traceback
 import unittest  # Exposed through the API.
 from warnings import warn
@@ -549,9 +548,11 @@ class InputApi(object):
     self.os_walk = os.walk
     self.re = re
     self.subprocess = subprocess
+    self.sys = sys
     self.tempfile = tempfile
     self.time = time
     self.unittest = unittest
+    if sys.version_info.major == 2:
       self.urllib2 = urllib2
     self.urllib_request = urllib_request
     self.urllib_error = urllib_error
@@ -581,7 +582,7 @@ class InputApi(object):
     # TODO(dpranke): figure out a list of all approved owners for a repo
     # in order to be able to handle wildcard OWNERS files?
     self.owners_db = owners.Database(change.RepositoryRoot(),
-                                     fopen=file, os_path=self.os_path)
+                                     fopen=open, os_path=self.os_path)
     self.owners_finder = owners_finder.OwnersFinder
     self.verbose = verbose
     self.Command = CommandData
@@ -615,9 +616,9 @@ class InputApi(object):
     if len(dir_with_slash) == 1:
       dir_with_slash = ''
 
-    return filter(
+    return list(filter(
         lambda x: normpath(x.AbsoluteLocalPath()).startswith(dir_with_slash),
-        self.change.AffectedFiles(include_deletes, file_filter))
+        self.change.AffectedFiles(include_deletes, file_filter)))
 
   def LocalPaths(self):
     """Returns local paths of input_api.AffectedFiles()."""
@@ -639,8 +640,9 @@ class InputApi(object):
                " is deprecated and ignored" % str(include_deletes),
            category=DeprecationWarning,
            stacklevel=2)
-    return filter(lambda x: x.IsTestableFile(),
+    return list(filter(
-                  self.AffectedFiles(include_deletes=False, **kwargs))
+        lambda x: x.IsTestableFile(),
+        self.AffectedFiles(include_deletes=False, **kwargs)))
 
   def AffectedTextFiles(self, include_deletes=None):
     """An alias to AffectedTestableFiles for backwards compatibility."""
@@ -673,7 +675,7 @@ class InputApi(object):
     """
     if not source_file:
       source_file = self.FilterSourceFile
-    return filter(source_file, self.AffectedTestableFiles())
+    return list(filter(source_file, self.AffectedTestableFiles()))
 
   def RightHandSideLines(self, source_file_filter=None):
     """An iterator over all text lines in "new" version of changed files.
@@ -1100,11 +1102,11 @@ class Change(object):
     Returns:
       [AffectedFile(path, action), AffectedFile(path, action)]
     """
-    affected = filter(file_filter, self._affected_files)
+    affected = list(filter(file_filter, self._affected_files))
 
     if include_deletes:
       return affected
-    return filter(lambda x: x.Action() != 'D', affected)
+    return list(filter(lambda x: x.Action() != 'D', affected))
 
   def AffectedTestableFiles(self, include_deletes=None, **kwargs):
     """Return a list of the existing text files in a change."""
@@ -1113,8 +1115,9 @@ class Change(object):
                " is deprecated and ignored" % str(include_deletes),
            category=DeprecationWarning,
            stacklevel=2)
-    return filter(lambda x: x.IsTestableFile(),
+    return list(filter(
-                  self.AffectedFiles(include_deletes=False, **kwargs))
+        lambda x: x.IsTestableFile(),
+        self.AffectedFiles(include_deletes=False, **kwargs)))
 
   def AffectedTextFiles(self, include_deletes=None):
     """An alias to AffectedTestableFiles for backwards compatibility."""
@@ -1451,9 +1454,9 @@ class PresubmitExecuter(object):
         logging.debug('Running %s done.', function_name)
         self.more_cc.extend(output_api.more_cc)
       finally:
-        map(os.remove, input_api._named_temporary_files)
+        for f in input_api._named_temporary_files:
-      if not (isinstance(result, types.TupleType) or
+          os.remove(f)
-              isinstance(result, types.ListType)):
+      if not isinstance(result, (tuple, list)):
         raise PresubmitFailure(
           'Presubmit functions must return a tuple or list')
       for item in result:
@@ -1513,11 +1516,8 @@ def DoPresubmitChecks(change,
   old_environ = os.environ
   try:
     # Make sure python subprocesses won't generate .pyc files.
-    # We convert to str, since on Windows on Python 2 only strings are allowed
-    # as environment variables, but literals are unicode since we're importing
-    # unicode_literals from __future__.
     os.environ = os.environ.copy()
-    os.environ[str('PYTHONDONTWRITEBYTECODE')] = str('1')
+    os.environ['PYTHONDONTWRITEBYTECODE'] = '1'
 
     output = PresubmitOutput(input_stream, output_stream)
 
@@ -1575,7 +1575,8 @@ def DoPresubmitChecks(change,
         ]
       }
 
-      gclient_utils.FileWrite(json_output, json.dumps(presubmit_results))
+      gclient_utils.FileWrite(
+          json_output, json.dumps(presubmit_results, sort_keys=True))
 
     output.write('\n')
     for name, items in (('Messages', notifications),

tests/presubmit_unittest.py

@@ -1,4 +1,4 @@
-#!/usr/bin/env python
+#!/usr/bin/env vpython3
 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
@@ -7,7 +7,8 @@
 
 # pylint: disable=no-member,E1103
 
-import StringIO
+from __future__ import unicode_literals
+
 import functools
 import itertools
 import logging
@@ -19,7 +20,6 @@ import sys
 import tempfile
 import time
 import unittest
-import urllib2
 
 _ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
 sys.path.insert(0, _ROOT)
@@ -38,6 +38,15 @@ import presubmit_support as presubmit
 import scm
 import subprocess2 as subprocess
 
+if sys.version_info.major == 2:
+  from cStringIO import StringIO
+  import urllib2 as urllib_request
+  BUILTIN_OPEN = '__builtin__.open'
+else:
+  from io import StringIO
+  import urllib.request as urllib_request
+  BUILTIN_OPEN = 'builtins.open'
+
 # Shortcut.
 presubmit_canned_checks = presubmit.presubmit_canned_checks
 
@@ -163,11 +172,14 @@ index fe3de7b..54ae6e1 100755
     mock.patch('scm.determine_scm').start()
     mock.patch('scm.GIT.GenerateDiff').start()
     mock.patch('subprocess2.Popen').start()
-    mock.patch('sys.stderr', StringIO.StringIO()).start()
+    mock.patch('sys.stderr', StringIO()).start()
-    mock.patch('sys.stdout', StringIO.StringIO()).start()
+    mock.patch('sys.stdout', StringIO()).start()
     mock.patch('tempfile.NamedTemporaryFile').start()
     mock.patch('multiprocessing.cpu_count', lambda: 2)
+    if sys.version_info.major == 2:
       mock.patch('urllib2.urlopen').start()
+    else:
+      mock.patch('urllib.request.urlopen').start()
     self.addCleanup(mock.patch.stopall)
 
   def checkstdout(self, value):
@@ -631,7 +643,7 @@ def CheckChangeOnCommit(input_api, output_api):
         ]
     }
 
-    fake_result_json = json.dumps(fake_result)
+    fake_result_json = json.dumps(fake_result, sort_keys=True)
 
     output = presubmit.DoPresubmitChecks(
         change=change, committing=False, verbose=True,
@@ -657,7 +669,7 @@ def CheckChangeOnCommit(input_api, output_api):
     # Make a change with a single warning.
     change = self.ExampleChange(extra_lines=['PROMPT_WARNING=yes'])
 
-    input_buf = StringIO.StringIO('n\n')  # say no to the warning
+    input_buf = StringIO('n\n')  # say no to the warning
     output = presubmit.DoPresubmitChecks(
         change=change, committing=False, verbose=True,
         output_stream=None, input_stream=input_buf,
@@ -666,7 +678,7 @@ def CheckChangeOnCommit(input_api, output_api):
     self.assertFalse(output.should_continue())
     self.assertEqual(output.getvalue().count('??'), 2)
 
-    input_buf = StringIO.StringIO('y\n')  # say yes to the warning
+    input_buf = StringIO('y\n')  # say yes to the warning
     output = presubmit.DoPresubmitChecks(
         change=change, committing=False, verbose=True,
         output_stream=None, input_stream=input_buf,
@@ -740,7 +752,7 @@ def CheckChangeOnCommit(input_api, output_api):
         lambda d: [] if d == self.fake_root_dir else ['PRESUBMIT.py'])
     random.randint.return_value = 0
 
-    input_buf = StringIO.StringIO('y\n')
+    input_buf = StringIO('y\n')
 
     change = self.ExampleChange(extra_lines=['STORY=http://tracker/123'])
     output = presubmit.DoPresubmitChecks(
@@ -820,7 +832,7 @@ def CheckChangeOnCommit(input_api, output_api):
       'try2.cr': {'linux2': set(['defaulttests'])},
     }
     for permutation in itertools.permutations(parts):
-      self.assertEqual(expected, reduce(merge, permutation, {}))
+      self.assertEqual(expected, functools.reduce(merge, permutation, {}))
 
   def testDoGetTryMasters(self):
     root_text = (self.presubmit_trymaster
@@ -844,12 +856,12 @@ def CheckChangeOnCommit(input_api, output_api):
     change = presubmit.Change(
         'mychange', '', self.fake_root_dir, [], 0, 0, None)
 
-    output = StringIO.StringIO()
+    output = StringIO()
     self.assertEqual({'t1.cr': {'win': ['defaulttests']}},
                      presubmit.DoGetTryMasters(change, [filename],
                                                self.fake_root_dir,
                                                None, None, False, output))
-    output = StringIO.StringIO()
+    output = StringIO()
     expected = {
       't1.cr': {'win': ['defaulttests'], 'linux1': ['t1']},
       't2.cr': {'linux2': ['defaulttests']},
@@ -1084,7 +1096,7 @@ class InputApiUnittest(PresubmitTestsBase):
     self.assertEqual(len(input_api.DEFAULT_WHITE_LIST), 24)
     self.assertEqual(len(input_api.DEFAULT_BLACK_LIST), 12)
     for item in files:
-      results = filter(input_api.FilterSourceFile, item[0])
+      results = list(filter(input_api.FilterSourceFile, item[0]))
       for i in range(len(results)):
         self.assertEqual(results[i].LocalPath(),
                           presubmit.normpath(item[1][i]))
@@ -1289,33 +1301,33 @@ class OutputApiUnittest(PresubmitTestsBase):
     self.assertIsNotNone(output.should_continue())
     self.assertIsNotNone(output.getvalue().count('?see?'))
 
-    output = presubmit.PresubmitOutput(input_stream=StringIO.StringIO('y'))
+    output = presubmit.PresubmitOutput(input_stream=StringIO('y'))
     presubmit.OutputApi.PresubmitPromptWarning('???').handle(output)
     output.prompt_yes_no('prompt: ')
     self.assertIsNotNone(output.should_continue())
     self.assertIsNotNone(output.getvalue().count('???'))
 
-    output = presubmit.PresubmitOutput(input_stream=StringIO.StringIO('\n'))
+    output = presubmit.PresubmitOutput(input_stream=StringIO('\n'))
     presubmit.OutputApi.PresubmitPromptWarning('???').handle(output)
     output.prompt_yes_no('prompt: ')
     self.assertFalse(output.should_continue())
     self.assertIsNotNone(output.getvalue().count('???'))
 
     output_api = presubmit.OutputApi(True)
-    output = presubmit.PresubmitOutput(input_stream=StringIO.StringIO('y'))
+    output = presubmit.PresubmitOutput(input_stream=StringIO('y'))
     output_api.PresubmitPromptOrNotify('???').handle(output)
     output.prompt_yes_no('prompt: ')
     self.assertIsNotNone(output.should_continue())
     self.assertIsNotNone(output.getvalue().count('???'))
 
     output_api = presubmit.OutputApi(False)
-    output = presubmit.PresubmitOutput(input_stream=StringIO.StringIO('y'))
+    output = presubmit.PresubmitOutput(input_stream=StringIO('y'))
     output_api.PresubmitPromptOrNotify('???').handle(output)
     self.assertIsNotNone(output.should_continue())
     self.assertIsNotNone(output.getvalue().count('???'))
 
     output_api = presubmit.OutputApi(True)
-    output = presubmit.PresubmitOutput(input_stream=StringIO.StringIO('\n'))
+    output = presubmit.PresubmitOutput(input_stream=StringIO('\n'))
     output_api.PresubmitPromptOrNotify('???').handle(output)
     output.prompt_yes_no('prompt: ')
     self.assertFalse(output.should_continue())
@@ -1351,7 +1363,7 @@ class AffectedFileUnittest(PresubmitTestsBase):
     f_blob = os.path.join(self.fake_root_dir, blob)
     os.path.isfile.side_effect = lambda f: f in [f_blat, f_blob]
 
-    output = filter(lambda x: x.IsTestableFile(), files)
+    output = list(filter(lambda x: x.IsTestableFile(), files))
     self.assertEqual(2, len(output))
     self.assertEqual(files[:2], output[:2])
 
@@ -1432,11 +1444,13 @@ class CannedChecksUnittest(PresubmitTestsBase):
     input_api.os_path = os.path
     input_api.re = presubmit.re
     input_api.gerrit = mock.MagicMock(presubmit.GerritAccessor)
+    if sys.version_info.major == 2:
       input_api.urllib2 = mock.MagicMock(presubmit.urllib2)
     input_api.urllib_request = mock.MagicMock(presubmit.urllib_request)
     input_api.urllib_error = mock.MagicMock(presubmit.urllib_error)
     input_api.unittest = unittest
     input_api.subprocess = subprocess
+    input_api.sys = sys
     class fake_CalledProcessError(Exception):
       def __str__(self):
         return 'foo'
@@ -1660,7 +1674,7 @@ class CannedChecksUnittest(PresubmitTestsBase):
       'config_sets': [{'config_set': 'deadbeef',
                        'location': '%s/+/%s' % (host, branch)}]
     }
-    urllib2.urlopen.return_value = http_resp
+    urllib_request.urlopen.return_value = http_resp
     json.load.return_value = http_resp
 
     mockChangelist().GetRemoteBranch.return_value = ('remote', branch)
@@ -1968,14 +1982,14 @@ the current line as well!
 
   def testCannedCheckTreeIsOpenOpen(self):
     input_api = self.MockInputApi(None, True)
-    input_api.urllib2.urlopen().read.return_value = 'The tree is open'
+    input_api.urllib_request.urlopen().read.return_value = 'The tree is open'
     results = presubmit_canned_checks.CheckTreeIsOpen(
         input_api, presubmit.OutputApi, url='url_to_open', closed='.*closed.*')
     self.assertEqual(results, [])
 
   def testCannedCheckTreeIsOpenClosed(self):
     input_api = self.MockInputApi(None, True)
-    input_api.urllib2.urlopen().read.return_value = (
+    input_api.urllib_request.urlopen().read.return_value = (
         'Tree is closed for maintenance')
     results = presubmit_canned_checks.CheckTreeIsOpen(
         input_api, presubmit.OutputApi,
@@ -1991,7 +2005,7 @@ the current line as well!
         'general_state': 'open',
         'message': 'The tree is open'
     }
-    input_api.urllib2.urlopen().read.return_value = json.dumps(status)
+    input_api.urllib_request.urlopen().read.return_value = json.dumps(status)
     results = presubmit_canned_checks.CheckTreeIsOpen(
         input_api, presubmit.OutputApi, json_url='url_to_open')
     self.assertEqual(results, [])
@@ -2003,7 +2017,7 @@ the current line as well!
         'general_state': 'closed',
         'message': 'The tree is close',
     }
-    input_api.urllib2.urlopen().read.return_value = json.dumps(status)
+    input_api.urllib_request.urlopen().read.return_value = json.dumps(status)
     results = presubmit_canned_checks.CheckTreeIsOpen(
         input_api, presubmit.OutputApi, json_url='url_to_closed')
     self.assertEqual(len(results), 1)
@@ -2121,7 +2135,7 @@ the current line as well!
 
   def testCheckBuildbotPendingBuildsBad(self):
     input_api = self.MockInputApi(None, True)
-    input_api.urllib2.urlopen().read.return_value = 'foo'
+    input_api.urllib_request.urlopen().read.return_value = 'foo'
 
     results = presubmit_canned_checks.CheckBuildbotPendingBuilds(
         input_api, presubmit.OutputApi, 'uurl', 2, ('foo'))
@@ -2131,7 +2145,7 @@ the current line as well!
 
   def testCheckBuildbotPendingBuildsGood(self):
     input_api = self.MockInputApi(None, True)
-    input_api.urllib2.urlopen().read.return_value = """
+    input_api.urllib_request.urlopen().read.return_value = """
     {
       'b1': { 'pending_builds': [0, 1, 2, 3, 4, 5, 6, 7] },
       'foo': { 'pending_builds': [0, 1, 2, 3, 4, 5, 6, 7] },
@@ -2157,7 +2171,7 @@ the current line as well!
 
     os.path.exists = lambda _: True
 
-    owners_file = StringIO.StringIO(owners_content)
+    owners_file = StringIO(owners_content)
     fopen = lambda *args: owners_file
 
     input_api.owners_db = owners.Database('', fopen, os.path)
@@ -2267,7 +2281,7 @@ the current line as well!
         presubmit.OutputApi)
     for result in results:
       result.handle(output)
-    if isinstance(expected_output, re._pattern_type):
+    if expected_output:
       self.assertRegexpMatches(output.getvalue(), expected_output)
     else:
       self.assertEqual(output.getvalue(), expected_output)
@@ -2546,7 +2560,7 @@ the current line as well!
                            is_committing=False,
                            uncovered_files=set())
 
-  @mock.patch('__builtin__.open', mock.mock_open(read_data=''))
+  @mock.patch(BUILTIN_OPEN, mock.mock_open(read_data=''))
   def testCannedRunUnitTests(self):
     change = presubmit.Change(
         'foo1', 'description1', self.fake_root_dir, None, 0, 0, None)
@@ -2589,7 +2603,7 @@ the current line as well!
 
     self.checkstdout('')
 
-  @mock.patch('__builtin__.open', mock.mock_open())
+  @mock.patch(BUILTIN_OPEN, mock.mock_open())
   def testCannedRunUnitTestsPython3(self):
     open().readline.return_value = '#!/usr/bin/env python3'
     change = presubmit.Change(
@@ -2645,7 +2659,7 @@ the current line as well!
 
     self.checkstdout('')
 
-  @mock.patch('__builtin__.open', mock.mock_open())
+  @mock.patch(BUILTIN_OPEN, mock.mock_open())
   def testCannedRunUnitTestsDontRunOnPython2(self):
     open().readline.return_value = '#!/usr/bin/env python3'
     change = presubmit.Change(
@@ -2689,7 +2703,7 @@ the current line as well!
 
     self.checkstdout('')
 
-  @mock.patch('__builtin__.open', mock.mock_open())
+  @mock.patch(BUILTIN_OPEN, mock.mock_open())
   def testCannedRunUnitTestsDontRunOnPython3(self):
     open().readline.return_value = '#!/usr/bin/env python3'
     change = presubmit.Change(