download_from_google_storage: Fix tests and rename

the tests haven't been ran by presumbit for a while because of the plural in
the filename.

At some point some post "gsutil cp" file checking happened, which
broke the tests.  This adds a callback to the fake gsutil cp so
that the expect file is copied over.

This also removes "gsutil version" checking from gsutil.py
and just assume that if the file exists, then it's good, which
should shave about 1-2s off of each gsutil.py call.

Bug: 772740,776311
Change-Id: I4fef62cfd46a849afed1f095dd6a96069376d13d
Reviewed-on: https://chromium-review.googlesource.com/707758
Commit-Queue: Ryan Tseng <hinoka@chromium.org>
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
Reviewed-by: Aaron Gable <agable@chromium.org>

gsutil.py

@@ -72,12 +72,6 @@ def download_gsutil(version, target_dir):
   return target_filename
 
 
-def check_gsutil(gsutil_bin):
-  """Run gsutil version and make sure it runs."""
-  return subprocess.call(
-      [sys.executable, gsutil_bin, 'version'],
-      stdout=subprocess.PIPE, stderr=subprocess.STDOUT) == 0
-
 @contextlib.contextmanager
 def temporary_directory(base):
   tmpdir = tempfile.mkdtemp(prefix='gsutil_py', dir=base)
@@ -90,7 +84,10 @@ def temporary_directory(base):
 def ensure_gsutil(version, target, clean):
   bin_dir = os.path.join(target, 'gsutil_%s' % version)
   gsutil_bin = os.path.join(bin_dir, 'gsutil', 'gsutil')
-  if not clean and os.path.isfile(gsutil_bin) and check_gsutil(gsutil_bin):
+  gsutil_flag = os.path.join(bin_dir, 'gsutil', 'install.flag')
+  # We assume that if gsutil_flag exists, then we have a good version
+  # of the gsutil package.
+  if not clean and os.path.isfile(gsutil_flag):
     # Everything is awesome! we're all done here.
     return gsutil_bin
 
@@ -116,10 +113,13 @@ def ensure_gsutil(version, target, clean):
     except (OSError, IOError):
       # Something else did this in parallel.
       pass
+    # Final check that the gsutil bin exists.  This should never fail.
+    if not os.path.isfile(gsutil_bin):
+      raise InvalidGsutilError()
+    # Drop a flag file.
+    with open(gsutil_flag, 'w') as f:
+      f.write('This flag file is dropped by gsutil.py')
 
-  # Final check that the gsutil bin is okay.  This should never fail.
-  if not check_gsutil(gsutil_bin):
-    raise InvalidGsutilError()
   return gsutil_bin
 
 

tests/download_from_google_storage_unittests.py → tests/download_from_google_storage_unittest.py

@@ -66,6 +66,9 @@ class GsutilMock(object):
       else:
         return (0, '', '')
 
+  def check_call_with_retries(self, *args):
+    return self.check_call(*args)
+
 
 class ChangedWorkingDirectory(object):
   def __init__(self, working_directory):
@@ -135,6 +138,7 @@ class GstoolsUnitTests(unittest.TestCase):
                                                             tar_dir))
 
   def test_gsutil(self):
+    # This will download a real gsutil package from Google Storage.
     gsutil = download_from_google_storage.Gsutil(GSUTIL_DEFAULT_PATH, None)
     self.assertEqual(gsutil.path, GSUTIL_DEFAULT_PATH)
     code, _, err = gsutil.check_call()
@@ -190,7 +194,7 @@ class DownloadTests(unittest.TestCase):
     self.parser = optparse.OptionParser()
     self.queue = Queue.Queue()
     self.ret_codes = Queue.Queue()
-    self.lorem_ipsum = os.path.join(self.base_path, 'lorem_ipsum.txt')
+    self.lorem_ipsum = os.path.join(TEST_DIR, 'gstools', 'lorem_ipsum.txt')
     self.lorem_ipsum_sha1 = '7871c8e24da15bad8b0be2c36edc9dc77e37727f'
     self.maxDiff = None
 
@@ -222,9 +226,12 @@ class DownloadTests(unittest.TestCase):
     self.assertEqual(queue_size, 3)
 
   def test_download_worker_single_file(self):
-    sha1_hash = '7871c8e24da15bad8b0be2c36edc9dc77e37727f'
+    sha1_hash = self.lorem_ipsum_sha1
     input_filename = '%s/%s' % (self.base_url, sha1_hash)
     output_filename = os.path.join(self.base_path, 'uploaded_lorem_ipsum.txt')
+    self.gsutil.add_expected(0, '', '')  # ls
+    self.gsutil.add_expected(0, '', '', lambda: shutil.copyfile(
+        self.lorem_ipsum, output_filename))  # cp
     self.queue.put((sha1_hash, output_filename))
     self.queue.put((None, None))
     stdout_queue = Queue.Queue()
@@ -257,10 +264,8 @@ class DownloadTests(unittest.TestCase):
     download_from_google_storage._downloader_worker_thread(
         0, self.queue, False, self.base_url, self.gsutil,
         stdout_queue, self.ret_codes, True, False)
-    expected_output = [
-        '0> File %s exists and SHA1 matches. Skipping.' % output_filename
-    ]
-    self.assertEqual(list(stdout_queue.queue), expected_output)
+    # dfgs does not output anything in the no-op case.
+    self.assertEqual(list(stdout_queue.queue), [])
     self.assertEqual(self.gsutil.history, [])
 
   def test_download_extract_archive(self):
@@ -354,11 +359,6 @@ class DownloadTests(unittest.TestCase):
         ('check_call',
             ('cp', input_filename, output_filename))
     ]
-    if sys.platform != 'win32':
-      expected_calls.append(
-          ('check_call',
-           ('stat',
-            'gs://sometesturl/7871c8e24da15bad8b0be2c36edc9dc77e37727f')))
     self.assertEqual(self.gsutil.history, expected_calls)
     self.assertEqual(code, 101)
 
@@ -392,6 +392,9 @@ class DownloadTests(unittest.TestCase):
     sha1_hash = '7871c8e24da15bad8b0be2c36edc9dc77e37727f'
     input_filename = '%s/%s' % (self.base_url, sha1_hash)
     output_filename = os.path.join(self.base_path, 'uploaded_lorem_ipsum.txt')
+    self.gsutil.add_expected(0, '', '')  # ls
+    self.gsutil.add_expected(0, '', '', lambda: shutil.copyfile(
+        self.lorem_ipsum, output_filename))  # cp
     code = download_from_google_storage.download_from_google_storage(
         input_filename=self.base_path,
         base_url=self.base_url,

tests/gsutil_test.py

@@ -122,14 +122,9 @@ class GsutilUnitTests(unittest.TestCase):
     version = '4.2'
     gsutil_dir = os.path.join(self.tempdir, 'gsutil_%s' % version, 'gsutil')
     gsutil_bin = os.path.join(gsutil_dir, 'gsutil')
+    gsutil_flag = os.path.join(gsutil_dir, 'install.flag')
     os.makedirs(gsutil_dir)
 
-    self.fake.add_expectation(
-        [sys.executable, gsutil_bin, 'version'], stdout=subprocess.PIPE,
-        stderr=subprocess.STDOUT, _returns=1)
-
-    with open(gsutil_bin, 'w') as f:
-      f.write('Foobar')
     zip_filename = 'gsutil_%s.zip' % version
     url = '%s%s' % (gsutil.GSUTIL_URL, zip_filename)
     _, tempzip = tempfile.mkstemp()
@@ -138,32 +133,26 @@ class GsutilUnitTests(unittest.TestCase):
       zf.writestr('gsutil/gsutil', fake_gsutil)
     with open(tempzip, 'rb') as f:
       self.fake.add_expectation(url, _returns=Buffer(f.read()))
-    self.fake.add_expectation(
-        [sys.executable, gsutil_bin, 'version'], stdout=subprocess.PIPE,
-        stderr=subprocess.STDOUT, _returns=1)
-
-    # This should delete the old bin and rewrite it with 'Fake gsutil'
-    self.assertRaises(
-        gsutil.InvalidGsutilError, gsutil.ensure_gsutil, version, self.tempdir,
-        False)
+
+    # This should write the gsutil_bin with 'Fake gsutil'
+    gsutil.ensure_gsutil(version, self.tempdir, False)
     self.assertTrue(os.path.exists(gsutil_bin))
     with open(gsutil_bin, 'r') as f:
       self.assertEquals(f.read(), fake_gsutil)
+    self.assertTrue(os.path.exists(gsutil_flag))
     self.assertEquals(self.fake.expectations, [])
 
   def test_ensure_gsutil_short(self):
     version = '4.2'
     gsutil_dir = os.path.join(self.tempdir, 'gsutil_%s' % version, 'gsutil')
     gsutil_bin = os.path.join(gsutil_dir, 'gsutil')
+    gsutil_flag = os.path.join(gsutil_dir, 'install.flag')
     os.makedirs(gsutil_dir)
 
-    # Mock out call().
-    self.fake.add_expectation(
-        [sys.executable, gsutil_bin, 'version'],
-        stdout=subprocess.PIPE, stderr=subprocess.STDOUT, _returns=0)
-
     with open(gsutil_bin, 'w') as f:
       f.write('Foobar')
+    with open(gsutil_flag, 'w') as f:
+      f.write('Barbaz')
     self.assertEquals(
         gsutil.ensure_gsutil(version, self.tempdir, False), gsutil_bin)
 

tests/upload_to_google_storage_unittests.py → tests/upload_to_google_storage_unittest.py

@@ -19,8 +19,8 @@ import unittest
 sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
 
 import upload_to_google_storage
-from download_from_google_storage_unittests import GsutilMock
-from download_from_google_storage_unittests import ChangedWorkingDirectory
+from download_from_google_storage_unittest import GsutilMock
+from download_from_google_storage_unittest import ChangedWorkingDirectory
 
 # ../third_party/gsutil/gsutil
 GSUTIL_DEFAULT_PATH = os.path.join(