From 83fd81f8a4eef6c8f176a7257662b18cac3e89be Mon Sep 17 00:00:00 2001 From: Ryan Tseng Date: Mon, 23 Oct 2017 11:13:48 -0700 Subject: [PATCH] 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 Reviewed-by: Robbie Iannucci Reviewed-by: Aaron Gable --- gsutil.py | 20 +++++++------- ... download_from_google_storage_unittest.py} | 25 +++++++++-------- tests/gsutil_test.py | 27 ++++++------------- ...y => upload_to_google_storage_unittest.py} | 4 +-- 4 files changed, 34 insertions(+), 42 deletions(-) rename tests/{download_from_google_storage_unittests.py => download_from_google_storage_unittest.py} (95%) rename tests/{upload_to_google_storage_unittests.py => upload_to_google_storage_unittest.py} (98%) diff --git a/gsutil.py b/gsutil.py index eb339bc41..e8e9ca3cd 100755 --- a/gsutil.py +++ b/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 diff --git a/tests/download_from_google_storage_unittests.py b/tests/download_from_google_storage_unittest.py similarity index 95% rename from tests/download_from_google_storage_unittests.py rename to tests/download_from_google_storage_unittest.py index d2ed8a8e1..1c285f491 100755 --- a/tests/download_from_google_storage_unittests.py +++ b/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, diff --git a/tests/gsutil_test.py b/tests/gsutil_test.py index d34eebfa5..d0fb8b029 100755 --- a/tests/gsutil_test.py +++ b/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) diff --git a/tests/upload_to_google_storage_unittests.py b/tests/upload_to_google_storage_unittest.py similarity index 98% rename from tests/upload_to_google_storage_unittests.py rename to tests/upload_to_google_storage_unittest.py index 3f37823e4..ed8eca1eb 100755 --- a/tests/upload_to_google_storage_unittests.py +++ b/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(