Fix S3Client.copy return value consistency #2488
Conversation
|
Just tagging a couple other recent |
test/contrib/s3_test.py
Outdated
| @@ -527,3 +511,11 @@ def _run_multipart_test(self, part_size, file_size, **kwargs): | |||
| key_size = s3_client.get_key(s3_path).size | |||
| self.assertEqual(file_size, key_size) | |||
| tmp_file.close() | |||
|
|
|||
| def _run_copy_response_test(self, response): | |||
There was a problem hiding this comment.
I'd find the way to check that the size is strictly equal to the sum instead of using GreaterEqual. Same thing to the number of items.
| @@ -454,15 +453,17 @@ def test_copy_dir(self): | |||
| self.assertTrue(s3_client.exists(file_path)) | |||
|
|
|||
| s3_dest = 's3://mybucket/copydir_new/' | |||
| s3_client.copy(s3_dir, s3_dest, threads=10, part_size=copy_part_size) | |||
| response = s3_client.copy(s3_dir, s3_dest, threads=10, part_size=copy_part_size) | |||
There was a problem hiding this comment.
I'd have a test for empty directory and empty file cases.
There was a problem hiding this comment.
There already exists a test_copy_empty_file, but I'll add a test for empty directory
|
@gardenunez Thanks for the review and suggestions!
I have no opposition to this refactor, but it is outside of the scope of this PR. Feel free to contribute it separately though :) |
test/contrib/s3_test.py
Outdated
| @@ -425,6 +424,26 @@ def test_copy_empty_file(self): | |||
| """ | |||
| self._run_copy_test(self.test_put_multipart_empty_file) | |||
|
|
|||
| @mock_s3 | |||
| @skipOnTravis('https://travis-ci.org/spotify/luigi/jobs/145895385') | |||
There was a problem hiding this comment.
Sorry. Copy/paste. Will remove to try to green build without it.
|
|
||
| return 1, key_size | ||
|
|
||
| def _run_copy_response_test(self, response, expected_num=None, expected_size=None): |
| @@ -235,7 +235,7 @@ def test_put_multipart_less_than_split_size(self): | |||
| # 5MB is minimum part size | |||
| part_size = 8388608 | |||
| file_size = 5000 | |||
| self._run_multipart_test(part_size, file_size) | |||
| return self._run_multipart_test(part_size, file_size) | |||
There was a problem hiding this comment.
Why do you return the last thing you do in a test case??? I thought you only used self.assertBlah()
There was a problem hiding this comment.
The test_put_multipart... tests are used below as args to self._run_copy_test(...) and I need the output of self._run_multipart_test to verify if the correct num and size of files were copied
* upstream-master: Fix S3Client.copy return value consistency (spotify#2488) s3client check for deprecated host keyword and raise error with the details (spotify#2493) Fix exception when toml lib is not installed (spotify#2506) Add Okko to companies that use luigi (spotify#2512) Added optional choice for hdfs clients (spotify#2487) Version 2.7.8 revert tornado upgrade Version 2.7.7 added a new event 'progress' (spotify#2498) Add Uppsala University / pharmb.io as user (spotify#2496)
Description
Adds support for num/size copy response for single-file copies.
Motivation and Context
S3Client.copy doesn't return a tuple if not copying a directory. This patch adds support for num/size tuple copy response for non-directories.
Have you tested this? If so, how?
Adds assertions for minimum values to existing copy tests.
All s3_test.py tests pass locally.