gclient_scm: Fix tests on windows

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

PRESUBMIT.py

@@ -71,7 +71,6 @@ def CommonChecks(input_api, output_api, tests_to_black_list, run_on_python3):
     tests_to_black_list = [
         r'.*auth_test\.py$',
         r'.*download_from_google_storage_unittest\.py$',
-        r'.*gclient_scm_test\.py$',
         r'.*gclient_smoketest\.py$',
         r'.*git_cl_test\.py$',
         r'.*git_common_test\.py$',

gclient_scm.py

@@ -1269,9 +1269,7 @@ class GitWrapper(SCMWrapper):
     # we don't accidentally go corrupting parent git checks too.  See
     # https://crbug.com/1000825 for an example.
     if set_git_dir:
-
       git_dir = os.path.abspath(os.path.join(self.checkout_path, '.git'))
-
       # Depending on how the .gclient file was defined, self.checkout_path
       # might be set to a unicode string, not a regular string; on Windows
       # Python2, we can't set env vars to be unicode strings, so we

testing_support/fake_repos.py

@@ -14,6 +14,7 @@ import errno
 import logging
 import os
 import pprint
+import random
 import re
 import socket
 import sys
@@ -28,6 +29,13 @@ import scm
 import subprocess2
 
 
+# Attempt |MAX_TRY| times to find a free port. Each time select one port at
+# random from the range [|PORT_START|, |PORT_END|].
+MAX_TRY = 10
+PORT_START = 20000
+PORT_END = 65535
+
+
 def write(path, content):
   f = open(path, 'wb')
   f.write(content.encode())
@@ -74,20 +82,20 @@ def commit_git(repo):
   return rev
 
 
-def test_port(host, port):
+def port_is_free(host, port):
   s = socket.socket()
   try:
-    return s.connect_ex((host, port)) == 0
+    return s.connect_ex((host, port)) != 0
   finally:
     s.close()
 
 
-def find_free_port(host, base_port):
+def find_free_port(host):
   """Finds a listening port free to listen to."""
-  while base_port < (2<<16):
+  for _ in range(MAX_TRY):
-    if not test_port(host, base_port):
+    base_port = random.randint(PORT_START, PORT_END)
+    if port_is_free(host, base_port):
       return base_port
-    base_port += 1
   assert False, 'Having issues finding an available port'
 
 
@@ -239,7 +247,7 @@ class FakeReposBase(object):
     self.set_up()
     if self.gitdaemon:
       return True
-    assert self.git_pid_file_name == None
+    assert self.git_pid_file_name == None, self.git_pid_file_name
     try:
       subprocess2.check_output(['git', '--version'])
     except (OSError, subprocess2.CalledProcessError):
@@ -247,12 +255,11 @@ class FakeReposBase(object):
     for repo in ['repo_%d' % r for r in range(1, self.NB_GIT_REPOS + 1)]:
       subprocess2.check_call(['git', 'init', '-q', join(self.git_root, repo)])
       self.git_hashes[repo] = [(None, None)]
-    self.git_port = find_free_port(self.host, 20000)
-    self.git_base = 'git://%s:%d/git/' % (self.host, self.git_port)
-    # Start the daemon.
     git_pid_file = tempfile.NamedTemporaryFile(delete=False)
     self.git_pid_file_name = git_pid_file.name
     git_pid_file.close()
+    self.git_port = find_free_port(self.host)
+    self.git_base = 'git://%s:%d/git/' % (self.host, self.git_port)
     cmd = ['git', 'daemon',
         '--export-all',
         '--reuseaddr',
@@ -261,7 +268,10 @@ class FakeReposBase(object):
         '--port=%d' % self.git_port]
     if self.host == '127.0.0.1':
       cmd.append('--listen=' + self.host)
-    self.check_port_is_free(self.git_port)
+    # Verify that the port is free.
+    if not port_is_free(self.host, self.git_port):
+      return False
+    # Start the daemon.
     self.gitdaemon = subprocess2.Popen(
         cmd,
         cwd=self.root_dir,
@@ -304,17 +314,6 @@ class FakeReposBase(object):
     subprocess2.check_call(
         ['git', 'fast-import', '--quiet'], cwd=repo_root, stdin=data.encode())
 
-  def check_port_is_free(self, port):
-    sock = socket.socket()
-    try:
-      sock.connect((self.host, port))
-      # It worked, throw.
-      assert False, '%d shouldn\'t be bound' % port
-    except (socket.error, EnvironmentError):
-      pass
-    finally:
-      sock.close()
-
   def populateGit(self):
     raise NotImplementedError()
 

tests/gclient_scm_test.py

@@ -7,7 +7,9 @@
 
 # pylint: disable=E1103
 
-from shutil import rmtree
+from __future__ import unicode_literals
+
+
 from subprocess import Popen, PIPE, STDOUT
 
 import json
@@ -30,9 +32,13 @@ from testing_support import fake_repos
 from testing_support import test_case_utils
 
 import gclient_scm
+import gclient_utils
 import git_cache
 import subprocess2
 
+
+GIT = 'git' if sys.platform != 'win32' else 'git.bat'
+
 # Disable global git cache
 git_cache.Mirror.SetCachePath(None)
 
@@ -168,26 +174,26 @@ from :3
   def CreateGitRepo(git_import, path):
     """Do it for real."""
     try:
-      Popen(['git', 'init', '-q'], stdout=PIPE, stderr=STDOUT,
+      Popen([GIT, 'init', '-q'], stdout=PIPE, stderr=STDOUT,
             cwd=path).communicate()
     except OSError:
       # git is not available, skip this test.
       return False
-    Popen(['git', 'fast-import', '--quiet'], stdin=PIPE, stdout=PIPE,
+    Popen([GIT, 'fast-import', '--quiet'], stdin=PIPE, stdout=PIPE,
         stderr=STDOUT, cwd=path).communicate(input=git_import.encode())
-    Popen(['git', 'checkout', '-q'], stdout=PIPE, stderr=STDOUT,
+    Popen([GIT, 'checkout', '-q'], stdout=PIPE, stderr=STDOUT,
         cwd=path).communicate()
-    Popen(['git', 'remote', 'add', '-f', 'origin', '.'], stdout=PIPE,
+    Popen([GIT, 'remote', 'add', '-f', 'origin', '.'], stdout=PIPE,
         stderr=STDOUT, cwd=path).communicate()
-    Popen(['git', 'checkout', '-b', 'new', 'origin/master', '-q'], stdout=PIPE,
+    Popen([GIT, 'checkout', '-b', 'new', 'origin/master', '-q'], stdout=PIPE,
         stderr=STDOUT, cwd=path).communicate()
-    Popen(['git', 'push', 'origin', 'origin/origin:origin/master', '-q'],
+    Popen([GIT, 'push', 'origin', 'origin/origin:origin/master', '-q'],
         stdout=PIPE, stderr=STDOUT, cwd=path).communicate()
-    Popen(['git', 'config', '--unset', 'remote.origin.fetch'], stdout=PIPE,
+    Popen([GIT, 'config', '--unset', 'remote.origin.fetch'], stdout=PIPE,
         stderr=STDOUT, cwd=path).communicate()
-    Popen(['git', 'config', 'user.email', 'someuser@chromium.org'], stdout=PIPE,
+    Popen([GIT, 'config', 'user.email', 'someuser@chromium.org'], stdout=PIPE,
         stderr=STDOUT, cwd=path).communicate()
-    Popen(['git', 'config', 'user.name', 'Some User'], stdout=PIPE,
+    Popen([GIT, 'config', 'user.name', 'Some User'], stdout=PIPE,
         stderr=STDOUT, cwd=path).communicate()
     return True
 
@@ -212,7 +218,7 @@ from :3
                staticmethod(lambda : True)).start()
     mock.patch('sys.stdout', StringIO()).start()
     self.addCleanup(mock.patch.stopall)
-    self.addCleanup(lambda: rmtree(self.root_dir))
+    self.addCleanup(gclient_utils.rmtree, self.root_dir)
 
 
 class ManagedGitWrapperTestCase(BaseGitWrapperTestCase):
@@ -282,7 +288,7 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase):
     file_path = join(self.base_path, 'c')
     with open(file_path, 'w') as f:
       f.writelines('new\n')
-    Popen(['git', 'add', 'c'], stdout=PIPE,
+    Popen([GIT, 'add', 'c'], stdout=PIPE,
           stderr=STDOUT, cwd=self.base_path).communicate()
     file_list = []
     scm.revert(options, self.args, file_list)
@@ -363,8 +369,9 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase):
     # parents instead.
     self.assertEqual(scm._Capture(['rev-parse', 'HEAD:']),
                      'd2e35c10ac24d6c621e14a1fcadceb533155627d')
-    self.assertEqual(scm._Capture(['rev-parse', 'HEAD^1']), rev)
+    parent = 'HEAD^' if sys.platform != 'win32' else 'HEAD^^'
-    self.assertEqual(scm._Capture(['rev-parse', 'HEAD^2']),
+    self.assertEqual(scm._Capture(['rev-parse', parent + '1']), rev)
+    self.assertEqual(scm._Capture(['rev-parse', parent + '2']),
                      scm._Capture(['rev-parse', 'origin/master']))
     sys.stdout.close()
 
@@ -387,7 +394,8 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase):
     # parent instead.
     self.assertEqual(scm._Capture(['rev-parse', 'HEAD:']),
                      'd2e35c10ac24d6c621e14a1fcadceb533155627d')
-    self.assertEqual(scm._Capture(['rev-parse', 'HEAD^']),
+    parent = 'HEAD^' if sys.platform != 'win32' else 'HEAD^^'
+    self.assertEqual(scm._Capture(['rev-parse', parent + '1']),
                      scm._Capture(['rev-parse', 'origin/master']))
     sys.stdout.close()
 
@@ -547,41 +555,6 @@ class ManagedGitWrapperTestCase(BaseGitWrapperTestCase):
     rev_info = scm.revinfo(options, (), None)
     self.assertEqual(rev_info, '069c602044c5388d2d15c3f875b057c852003458')
 
-  def testMirrorPushUrl(self):
-    if not self.enabled:
-      return
-    fakes = fake_repos.FakeRepos()
-    fakes.set_up_git()
-    self.url = fakes.git_base + 'repo_1'
-    self.root_dir = fakes.root_dir
-    self.addCleanup(fake_repos.FakeRepos.tear_down_git, fakes)
-
-    mirror = tempfile.mkdtemp()
-    self.addCleanup(rmtree, mirror)
-
-    # This should never happen, but if it does, it'd render the other assertions
-    # in this test meaningless.
-    self.assertFalse(self.url.startswith(mirror))
-
-    git_cache.Mirror.SetCachePath(mirror)
-    self.addCleanup(git_cache.Mirror.SetCachePath, None)
-
-    options = self.Options()
-    scm = gclient_scm.GitWrapper(self.url, self.root_dir, self.relpath)
-    self.assertIsNotNone(scm._GetMirror(self.url, options))
-    scm.update(options, (), [])
-
-    fetch_url = scm._Capture(['remote', 'get-url', 'origin'])
-    self.assertTrue(
-        fetch_url.startswith(mirror),
-        msg='\n'.join([
-            'Repository fetch url should be in the git cache mirror directory.',
-            '  fetch_url: %s' % fetch_url,
-            '  mirror:    %s' % mirror]))
-    push_url = scm._Capture(['remote', 'get-url', '--push', 'origin'])
-    self.assertEqual(push_url, self.url)
-    sys.stdout.close()
-
 
 class ManagedGitWrapperTestCaseMock(unittest.TestCase):
   class OptionsObject(object):
@@ -736,6 +709,8 @@ class UnmanagedGitWrapperTestCase(BaseGitWrapperTestCase):
     options = self.Options()
 
     origin_root_dir = self.root_dir
+    self.addCleanup(gclient_utils.rmtree, origin_root_dir)
+
     self.root_dir = tempfile.mkdtemp()
     self.relpath = '.'
     self.base_path = join(self.root_dir, self.relpath)
@@ -758,7 +733,6 @@ class UnmanagedGitWrapperTestCase(BaseGitWrapperTestCase):
     self.checkInStdout(
       'Checked out refs/remotes/origin/master to a detached HEAD')
 
-    rmtree(origin_root_dir)
 
   def testUpdateCloneOnCommit(self):
     if not self.enabled:
@@ -766,6 +740,8 @@ class UnmanagedGitWrapperTestCase(BaseGitWrapperTestCase):
     options = self.Options()
 
     origin_root_dir = self.root_dir
+    self.addCleanup(gclient_utils.rmtree, origin_root_dir)
+
     self.root_dir = tempfile.mkdtemp()
     self.relpath = '.'
     self.base_path = join(self.root_dir, self.relpath)
@@ -790,14 +766,14 @@ class UnmanagedGitWrapperTestCase(BaseGitWrapperTestCase):
     self.checkInStdout(
       'Checked out a7142dc9f0009350b96a11f372b6ea658592aa95 to a detached HEAD')
 
-    rmtree(origin_root_dir)
-
   def testUpdateCloneOnBranch(self):
     if not self.enabled:
       return
     options = self.Options()
 
     origin_root_dir = self.root_dir
+    self.addCleanup(gclient_utils.rmtree, origin_root_dir)
+
     self.root_dir = tempfile.mkdtemp()
     self.relpath = '.'
     self.base_path = join(self.root_dir, self.relpath)
@@ -823,14 +799,14 @@ class UnmanagedGitWrapperTestCase(BaseGitWrapperTestCase):
         'Checked out 9a51244740b25fa2ded5252ca00a3178d3f665a9 '
         'to a detached HEAD')
 
-    rmtree(origin_root_dir)
-
   def testUpdateCloneOnFetchedRemoteBranch(self):
     if not self.enabled:
       return
     options = self.Options()
 
     origin_root_dir = self.root_dir
+    self.addCleanup(gclient_utils.rmtree, origin_root_dir)
+
     self.root_dir = tempfile.mkdtemp()
     self.relpath = '.'
     self.base_path = join(self.root_dir, self.relpath)
@@ -855,14 +831,14 @@ class UnmanagedGitWrapperTestCase(BaseGitWrapperTestCase):
     self.checkInStdout(
       'Checked out refs/remotes/origin/feature to a detached HEAD')
 
-    rmtree(origin_root_dir)
-
   def testUpdateCloneOnTrueRemoteBranch(self):
     if not self.enabled:
       return
     options = self.Options()
 
     origin_root_dir = self.root_dir
+    self.addCleanup(gclient_utils.rmtree, origin_root_dir)
+
     self.root_dir = tempfile.mkdtemp()
     self.relpath = '.'
     self.base_path = join(self.root_dir, self.relpath)
@@ -894,8 +870,6 @@ class UnmanagedGitWrapperTestCase(BaseGitWrapperTestCase):
     self.checkInStdout(
       'Checked out refs/remotes/origin/feature to a detached HEAD')
 
-    rmtree(origin_root_dir)
-
   def testUpdateUpdate(self):
     if not self.enabled:
       return
@@ -933,10 +907,8 @@ class CipdWrapperTestCase(unittest.TestCase):
     mock.patch('gclient_scm.CipdRoot.clobber').start()
     mock.patch('gclient_scm.CipdRoot.ensure').start()
     self.addCleanup(mock.patch.stopall)
-
+    self.addCleanup(gclient_utils.rmtree, self._cipd_root_dir)
-  def tearDown(self):
+    self.addCleanup(gclient_utils.rmtree, self._workdir)
-    rmtree(self._cipd_root_dir)
-    rmtree(self._workdir)
 
   def createScmWithPackageThatSatisfies(self, condition):
     return gclient_scm.CipdWrapper(
@@ -1061,11 +1033,13 @@ class GerritChangesTest(fake_repos.FakeReposTestBase):
     self.options = BaseGitWrapperTestCase.OptionsObject()
     self.url = self.git_base + 'repo_1'
     self.mirror = None
+    mock.patch('sys.stdout').start()
+    self.addCleanup(mock.patch.stopall)
 
   def setUpMirror(self):
     self.mirror = tempfile.mkdtemp()
     git_cache.Mirror.SetCachePath(self.mirror)
-    self.addCleanup(rmtree, self.mirror)
+    self.addCleanup(gclient_utils.rmtree, self.mirror)
     self.addCleanup(git_cache.Mirror.SetCachePath, None)
 
   def assertCommits(self, commits):
@@ -1108,6 +1082,25 @@ class GerritChangesTest(fake_repos.FakeReposTestBase):
     self.setUpMirror()
     self.testCanSyncToGerritChange()
 
+  def testMirrorPushUrl(self):
+    self.setUpMirror()
+
+    scm = gclient_scm.GitWrapper(self.url, self.root_dir, '.')
+    file_list = []
+    self.assertIsNotNone(scm._GetMirror(self.url, self.options))
+
+    scm.update(self.options, None, file_list)
+
+    fetch_url = scm._Capture(['remote', 'get-url', 'origin'])
+    self.assertTrue(
+        fetch_url.startswith(self.mirror),
+        msg='\n'.join([
+            'Repository fetch url should be in the git cache mirror directory.',
+            '  fetch_url: %s' % fetch_url,
+            '  mirror:    %s' % self.mirror]))
+    push_url = scm._Capture(['remote', 'get-url', '--push', 'origin'])
+    self.assertEqual(push_url, self.url)
+
   def testAppliesPatchOnTopOfMasterByDefault(self):
     """Test the default case, where we apply a patch on top of master."""
     scm = gclient_scm.GitWrapper(self.url, self.root_dir, '.')