Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Corrupt GDP1h files when downloading Issue #531 #532

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from
Draft

Corrupt GDP1h files when downloading Issue #531 #532

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
8c4100a
Empty commit
KevinShuman Nov 21, 2024
8b65a76
Creates verify and fix functions w/ tests
KevinShuman Nov 21, 2024
f6ec330
Reformats verify and fix functions and their tests
KevinShuman Nov 21, 2024
5b7f1c4
Removes verify/fix functions & tests
KevinShuman Nov 26, 2024
5b18813
Includes multi-attempt check for 0 byte files
KevinShuman Nov 26, 2024
5d750d1
Modifies _download_with_progress() to pass tests
KevinShuman Nov 26, 2024
25f9442
Modifies _download_with_progress() to pass mypy
KevinShuman Nov 26, 2024
476757e
Adjusts _download_with_progress to handle buffer
KevinShuman Dec 10, 2024
87d18ae
Adjusts tests to work with changes with _download_with_progress
KevinShuman Dec 10, 2024
9c9a1f2
Ignores type errors for cut_str(), which were not resulting in errors…
KevinShuman Dec 11, 2024
fc70232
Ignores type errors for cut_str(), which were not resulting in errors…
KevinShuman Dec 11, 2024
6c92c02
Adds test for better coverage
KevinShuman Dec 13, 2024
d7dce77
Adds test for better coverage
KevinShuman Dec 13, 2024
44cbcfa
Adds another test to improve coverage
KevinShuman Dec 13, 2024
40e2292
Resolves linting error
KevinShuman Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions clouddrift/adapters/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,9 +205,9 @@

if isinstance(temp_output, str) and os.path.getsize(temp_output) > 0:
# (temp_output = str <-> output = str)
if os.path.exists(output): # type: ignore
os.remove(output) # type: ignore
os.rename(temp_output, output) # type: ignore
if os.path.exists(output): # type: ignore
os.remove(output) # type: ignore
os.rename(temp_output, output) # type: ignore
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mypy doesn't recognize that because temp_output is a str that output is a str. The definition of temp_out comes from determining is output is a str. I.e. if output is a str, define temp_output as a str.

return
elif isinstance(output, BufferedIOBase):
_logger.debug("Download completed successfully to buffer.")
Expand All @@ -216,8 +216,8 @@
_logger.warning(f"Download failed or incomplete: {temp_output}")
except requests.RequestException as e:
_logger.error(f"Request failed: {e}")
except Exception as e:
_logger.error(f"Unexpected error: {e}")

Check warning on line 220 in clouddrift/adapters/utils.py

View check run for this annotation

Codecov / codecov/patch

clouddrift/adapters/utils.py#L219-L220 

Added lines #L219 - L220 were not covered by tests

raise Exception(f"Failed to download {url} after {max_attempts} attempts")

Expand Down
19 changes: 11 additions & 8 deletions tests/adapters/utils_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ class utils_tests(unittest.TestCase):
bar_mock: Mock

def setUp(self) -> None:
'''
"""
Set up the mocks for the tests.
'''
"""
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frankly, not sure why I changed these. Might change them back.


# Mock responses for requests.head and requests.get
self.head_response_mock = Mock()
Expand Down Expand Up @@ -351,9 +351,7 @@ def test_download_with_empty_chunk_raises_error_after_max_attempts(self):

with MultiPatcher(
[
patch(
"clouddrift.adapters.utils.tqdm", Mock()
),
patch("clouddrift.adapters.utils.tqdm", Mock()),
patch(
"clouddrift.adapters.utils.os.path.exists", Mock(return_value=False)
),
Expand Down Expand Up @@ -412,7 +410,9 @@ def test_download_request_exception_context_manager(self):
patch("clouddrift.adapters.utils.os.rename", Mock()),
patch(
"clouddrift.adapters.utils.requests.get",
MagicMock(side_effect=[RequestException("Network error")] * max_attempts),
MagicMock(
side_effect=[RequestException("Network error")] * max_attempts
),
),
patch("clouddrift.adapters.utils._logger.error", Mock()),
]
Expand All @@ -431,7 +431,7 @@ def test_download_request_exception_context_manager(self):

# Verify that _logger.error was called max_attempts times with appropriate messages
expected_error_calls = [
call.error(f"Request failed: Network error") for _ in range(max_attempts)
call.error("Request failed: Network error") for _ in range(max_attempts)
]
utils._logger.error.assert_has_calls(expected_error_calls, any_order=False)
self.assertEqual(utils._logger.error.call_count, max_attempts)
Expand All @@ -440,7 +440,10 @@ def test_download_request_exception_context_manager(self):
utils.os.rename.assert_not_called()

# Verify that the exception message contains the expected text
self.assertIn("Failed to download http://example.com/file after 5 attempts", str(context.exception))
self.assertIn(
"Failed to download http://example.com/file after 5 attempts",
str(context.exception),
)

# Verify that buffer.write was not called
buffer = Mock(spec=BufferedIOBase)
Expand Down
Loading