ADBDirectoryTransfer: fix test breakage.
Fixes a few reasons why ADBDirectoryTransfer was breaking:
1. Differing send_file/get_file behavior on LocalHost vs SSHHost.
LocalHost was copying directory contents rather than the directory
itself. This CL changes LocalHost to match rsync behavior, which
should help avoid this case in the future by making local tests
behave the same as bot tests.
2. ADBDirectoryTransfer targeting.
The source/dest targeting in this test was set up to work based on
the old LocalHost behavior. This CL modifies the test to make sure
it's the whole directory that is sent, and adds an additional subdir
to make sure that also works as expected.
3. Preserving permissions in ADBHost get_file.
The permission transferring code had a few bugs for directories.
This CL fixes the pathing and variable names so that permissions will
be copied correctly for all the transferred files/directories.
BUG: b/28150688
TEST: test_droid.py brillo_ADBFileTransfer.10iters \
brillo_ADBDirectoryTransfer
TEST: unittest_suite.py fails the same tests as before.
Change-Id: I941444dbf34ca488e831ceac06a4a4e0b4abf828
Reviewed-on: https://chromium-review.googlesource.com/344413
Commit-Ready: David Pursell <dpursell@chromium.org>
Tested-by: David Pursell <dpursell@chromium.org>
Reviewed-by: David Pursell <dpursell@chromium.org>
Reviewed-by: Kevin Cheng <kevcheng@chromium.org>
diff --git a/client/bin/local_host.py b/client/bin/local_host.py
index 7c5e8f6..e8ba3df 100644
--- a/client/bin/local_host.py
+++ b/client/bin/local_host.py
@@ -100,6 +100,10 @@
preserve_symlinks=False):
"""Copy files from source to dest, will be the base for {get,send}_file.
+ If source is a directory and ends with a trailing slash, only the
+ contents of the source directory will be copied to dest, otherwise
+ source itself will be copied under dest.
+
@param source: The file/directory on localhost to copy.
@param dest: The destination path on localhost to copy to.
@param delete_dest: A flag set to choose whether or not to delete
@@ -109,10 +113,16 @@
@param preserve_symlinks: Try to preserve symlinks instead of
transforming them into files/dirs on copy.
"""
+ # If dest is a directory and source doesn't end with /, copy source
+ # underneath dest; otherwise, replace dest.
+ if os.path.isdir(dest) and not source.endswith(os.sep):
+ dest = os.path.join(dest, os.path.basename(source))
+
if delete_dest and os.path.exists(dest):
# Check if it's a file or a dir and use proper remove method.
if os.path.isdir(dest):
shutil.rmtree(dest)
+ os.mkdir(dest)
else:
os.remove(dest)
@@ -136,6 +146,11 @@
preserve_symlinks=False):
"""Copy files from source to dest.
+ If source is a directory and ends with a trailing slash, only the
+ contents of the source directory will be copied to dest, otherwise
+ source itself will be copied under dest. This is to match the
+ behavior of AbstractSSHHost.get_file().
+
@param source: The file/directory on localhost to copy.
@param dest: The destination path on localhost to copy to.
@param delete_dest: A flag set to choose whether or not to delete
@@ -154,6 +169,11 @@
preserve_symlinks=False):
"""Copy files from source to dest.
+ If source is a directory and ends with a trailing slash, only the
+ contents of the source directory will be copied to dest, otherwise
+ source itself will be copied under dest. This is to match the
+ behavior of AbstractSSHHost.send_file().
+
@param source: The file/directory on the drone to send to the device.
@param dest: The destination path on the device to copy to.
@param delete_dest: A flag set to choose whether or not to delete
diff --git a/client/bin/local_host_unittest.py b/client/bin/local_host_unittest.py
index 6206c3d..1199b50 100755
--- a/client/bin/local_host_unittest.py
+++ b/client/bin/local_host_unittest.py
@@ -134,5 +134,117 @@
host.symlink_closure([sname]))
+ def test_get_directory(self):
+ """Tests get_file() copying a directory."""
+ host = local_host.LocalHost()
+
+ source_dir = os.path.join(self.tmpdir.name, 'dir')
+ os.mkdir(source_dir)
+ open(os.path.join(source_dir, 'file'), 'w').close()
+
+ dest_dir = os.path.join(self.tmpdir.name, 'dest')
+
+ host.get_file(source_dir, dest_dir)
+
+ self.assertTrue(os.path.isdir(dest_dir))
+ self.assertTrue(os.path.isfile(os.path.join(dest_dir, 'file')))
+
+
+ def test_get_directory_into_existing_directory(self):
+ """Tests get_file() copying a directory into an existing dir."""
+ host = local_host.LocalHost()
+
+ source_dir = os.path.join(self.tmpdir.name, 'dir')
+ os.mkdir(source_dir)
+ open(os.path.join(source_dir, 'file'), 'w').close()
+
+ dest_dir = os.path.join(self.tmpdir.name, 'dest')
+ os.mkdir(dest_dir)
+
+ host.get_file(source_dir, dest_dir)
+
+ self.assertTrue(os.path.isdir(os.path.join(dest_dir, 'dir')))
+ self.assertTrue(os.path.isfile(os.path.join(dest_dir, 'dir', 'file')))
+
+
+ def test_get_directory_into_existing_directory_delete(self):
+ """Tests get_file() replacing an existing dir."""
+ host = local_host.LocalHost()
+
+ source_dir = os.path.join(self.tmpdir.name, 'dir')
+ os.mkdir(source_dir)
+ open(os.path.join(source_dir, 'file'), 'w').close()
+
+ dest_dir = os.path.join(self.tmpdir.name, 'dest')
+ os.mkdir(dest_dir)
+ os.mkdir(os.path.join(dest_dir, 'dir'))
+ open(os.path.join(dest_dir, 'dir', 'file2'), 'w').close()
+
+ host.get_file(source_dir, dest_dir, delete_dest=True)
+
+ self.assertTrue(os.path.isdir(os.path.join(dest_dir, 'dir')))
+ self.assertTrue(os.path.isfile(os.path.join(dest_dir, 'dir', 'file')))
+ self.assertFalse(os.path.isfile(os.path.join(dest_dir, 'dir', 'file2')))
+
+
+ def test_get_directory_contents(self):
+ """Tests get_file() copying dir contents to a new location.
+
+ In this case it should behave the same as without a slash since
+ we are creating a nonexistent directory.
+ """
+ host = local_host.LocalHost()
+
+ source_dir = os.path.join(self.tmpdir.name, 'dir')
+ os.mkdir(source_dir)
+ open(os.path.join(source_dir, 'file'), 'w').close()
+
+ dest_dir = os.path.join(self.tmpdir.name, 'dest')
+
+ # End the source with '/' to copy the contents only.
+ host.get_file(os.path.join(source_dir, ''), dest_dir)
+
+ self.assertTrue(os.path.isdir(dest_dir))
+ self.assertTrue(os.path.isfile(os.path.join(dest_dir, 'file')))
+
+
+ def test_get_directory_contents_into_existing_directory(self):
+ """Tests get_file() copying dir contents into an existing dir."""
+ host = local_host.LocalHost()
+
+ source_dir = os.path.join(self.tmpdir.name, 'dir')
+ os.mkdir(source_dir)
+ open(os.path.join(source_dir, 'file'), 'w').close()
+
+ dest_dir = os.path.join(self.tmpdir.name, 'dest')
+ os.mkdir(dest_dir)
+
+ # End the source with '/' to copy the contents only.
+ host.get_file(os.path.join(source_dir, ''), dest_dir)
+
+ self.assertTrue(os.path.isdir(dest_dir))
+ self.assertTrue(os.path.isfile(os.path.join(dest_dir, 'file')))
+
+
+ def test_get_directory_contents_into_existing_directory_delete(self):
+ """Tests get_file() replacing contents of an existing dir."""
+ host = local_host.LocalHost()
+
+ source_dir = os.path.join(self.tmpdir.name, 'dir')
+ os.mkdir(source_dir)
+ open(os.path.join(source_dir, 'file'), 'w').close()
+
+ dest_dir = os.path.join(self.tmpdir.name, 'dest')
+ os.mkdir(dest_dir)
+ open(os.path.join(dest_dir, 'file2'), 'w').close()
+
+ # End the source with '/' to copy the contents only.
+ host.get_file(os.path.join(source_dir, ''), dest_dir, delete_dest=True)
+
+ self.assertTrue(os.path.isdir(dest_dir))
+ self.assertTrue(os.path.isfile(os.path.join(dest_dir, 'file')))
+ self.assertFalse(os.path.isfile(os.path.join(dest_dir, 'file2')))
+
+
if __name__ == "__main__":
unittest.main()
diff --git a/server/hosts/adb_host.py b/server/hosts/adb_host.py
index 5ebc8ac..bc0026f 100644
--- a/server/hosts/adb_host.py
+++ b/server/hosts/adb_host.py
@@ -115,6 +115,10 @@
# Default timeout in minutes for fastboot commands.
DEFAULT_FASTBOOT_RETRY_TIMEOUT_MIN = 10
+# Default permissions for files/dirs copied from the device.
+_DEFAULT_FILE_PERMS = 0o600
+_DEFAULT_DIR_PERMS = 0o700
+
class AndroidInstallError(error.InstallError):
"""Generic error for Android installation related exceptions."""
@@ -867,12 +871,16 @@
@param preserve_symlinks: Try to preserve symlinks instead of
transforming them into files/dirs on copy.
"""
- # Stage the files on the test station.
- tmp_dir = self.teststation.get_tmp_dir()
- dest_path = os.path.join(tmp_dir, os.path.basename(source))
+ # Stage the files on the test station under teststation_temp_dir.
+ teststation_temp_dir = self.teststation.get_tmp_dir()
+ teststation_dest = os.path.join(teststation_temp_dir,
+ os.path.basename(source))
- if delete_dest:
- self.teststation.run('rm -rf %s' % dest)
+ # If dest is a directory, source will be put under it.
+ if os.path.isdir(dest):
+ receive_path = os.path.join(dest, os.path.basename(source))
+ else:
+ receive_path = dest
source_info = {}
if preserve_symlinks or preserve_perm:
@@ -886,40 +894,38 @@
if preserve_symlinks and source_info['symlink']:
os.symlink(source_info['symlink'], dest)
else:
- self.adb_run('pull %s %s' % (source, dest_path))
+ self.adb_run('pull %s %s' % (source, teststation_temp_dir))
# Copy over the file from the test station and clean up.
- self.teststation.get_file(dest_path, dest)
+ self.teststation.get_file(teststation_dest, dest,
+ delete_dest=delete_dest)
try:
- self.teststation.run('rm -rf %s' % tmp_dir)
+ self.teststation.run('rm -rf %s' % teststation_temp_dir)
except (error.AutoservRunError, error.AutoservSSHTimeout) as e:
- logging.warn('failed to remove dir %s: %s', tmp_dir, e)
+ logging.warn('failed to remove dir %s: %s',
+ teststation_temp_dir, e)
- for root, dirs, files in os.walk(dest):
- rel_root = os.path.relpath(root, dest)
+ # Set the permissions of the received file/dirs.
+ if os.path.isdir(receive_path):
+ for root, _dirs, files in os.walk(receive_path):
+ def process(rel_path, default_perm):
+ info = self._get_file_info(os.path.join(source, rel_path))
- def process(item, default_perm):
- info = self._get_file_info(os.path.join(source, rel_root, item))
+ if info['perms'] != 0:
+ target = os.path.join(receive_path, rel_path)
+ if preserve_perm:
+ os.chmod(target, info['perms'])
+ else:
+ os.chmod(target, default_perm)
- if info['perms'] != 0:
- target = os.path.join(root, item)
- if preserve_perm:
- os.chmod(target, info['perms'])
- else:
- os.chmod(target, default_perm)
-
- for f in files:
- process(f, 0o600)
-
- for d in dirs:
- process(f, 0o700)
-
- if preserve_perm:
- os.chmod(dest, source_info['perms'])
- elif os.path.isdir(dest):
- os.chmod(dest, 0o700)
+ rel_root = os.path.relpath(root, receive_path)
+ process(rel_root, _DEFAULT_DIR_PERMS)
+ for f in files:
+ process(os.path.join(rel_root, f), _DEFAULT_FILE_PERMS)
+ elif preserve_perm:
+ os.chmod(receive_path, source_info['perms'])
else:
- os.chmod(dest, 0o600)
+ os.chmod(receive_path, _DEFAULT_FILE_PERMS)
def get_release_version(self):
diff --git a/server/site_tests/brillo_ADBDirectoryTransfer/brillo_ADBDirectoryTransfer.py b/server/site_tests/brillo_ADBDirectoryTransfer/brillo_ADBDirectoryTransfer.py
index e5023e4..286ccd9 100644
--- a/server/site_tests/brillo_ADBDirectoryTransfer/brillo_ADBDirectoryTransfer.py
+++ b/server/site_tests/brillo_ADBDirectoryTransfer/brillo_ADBDirectoryTransfer.py
@@ -14,6 +14,7 @@
_DATA_STR_A = 'Alluminum, linoleum, magnesium, petrolium.'
_DATA_STR_B = ('A basket of biscuits, a basket of mixed biscuits,'
'and a biscuit mixer.')
+_DATA_STR_C = 'foo\nbar\nbaz'
class brillo_ADBDirectoryTransfer(test.test):
@@ -22,33 +23,43 @@
def setup(self):
+ # Create a test directory tree to send and receive:
+ # test_dir/
+ # file_a
+ # file_b
+ # test_subdir/
+ # file_c
self.temp_dir = tempfile.mkdtemp()
- self.temp_dir_return = tempfile.mkdtemp()
- self.file_a = os.path.join(self.temp_dir, 'file_a')
- self.file_b = os.path.join(self.temp_dir, 'file_b')
+ self.test_dir = os.path.join(self.temp_dir, 'test_dir')
+ os.mkdir(self.test_dir)
+ os.mkdir(os.path.join(self.test_dir, 'subdir'))
- with open(self.file_a, 'w') as f:
+ with open(os.path.join(self.test_dir, 'file_a'), 'w') as f:
f.write(_DATA_STR_A)
- with open(self.file_b, 'w') as f:
+ with open(os.path.join(self.test_dir, 'file_b'), 'w') as f:
f.write(_DATA_STR_B)
+ with open(os.path.join(self.test_dir, 'subdir', 'file_c'), 'w') as f:
+ f.write(_DATA_STR_C)
+
def run_once(self, host=None):
"""Body of the test."""
- device_temp_dir = os.path.join(host.get_tmp_dir(), 'adb_test_dir')
- return_temp_dir = os.path.join(self.temp_dir_return, 'adb_test_return')
- return_file_a = os.path.join(return_temp_dir, 'file_a')
- return_file_b = os.path.join(return_temp_dir, 'file_b')
+ device_temp_dir = host.get_tmp_dir()
+ device_test_dir = os.path.join(device_temp_dir, 'test_dir')
+ return_dir = os.path.join(self.temp_dir, 'return_dir')
- host.send_file(self.temp_dir, device_temp_dir, delete_dest=True)
- host.get_file(device_temp_dir, return_temp_dir, delete_dest=True)
+ # Copy test_dir to the device then back as return_dir.
+ host.send_file(self.test_dir, device_test_dir, delete_dest=True)
+ host.get_file(device_test_dir, return_dir, delete_dest=True)
- if not filecmp.cmp(self.file_a, return_file_a, shallow=False) or \
- not filecmp.cmp(self.file_b, return_file_b, shallow=False):
- raise error.TestFail('One of the files changed in transit')
+ for path in ('file_a', 'file_b', os.path.join('subdir', 'file_c')):
+ original = os.path.join(self.test_dir, path)
+ returned = os.path.join(return_dir, path)
+ if not filecmp.cmp(original, returned, shallow=False):
+ raise error.TestFail(path + ' changed in transit')
def cleanup(self):
shutil.rmtree(self.temp_dir)
- shutil.rmtree(self.temp_dir_return)