Fix timezone-related test failures #1295
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit c82491d.
The test_handle_session_error_during_download now uses a cassette with an actual session error response instead of mocking PyiCloudAPIResponseException. This works because the error handling code in session.py (before commit c82491d) properly parses error responses and raises exceptions. Changes: - Created listing_photos_session_error_download.yml cassette with 401 "Invalid global session" response - Removed all mocks from test_handle_session_error_during_download - Test now relies entirely on cassette for session error simulation - Demonstrates that cassettes can trigger PyiCloudAPIResponseException without mocks This is a cleaner approach that tests the actual error handling path rather than bypassing it with mocks. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…tests - Converted 4 session error tests from using mocks to using VCR cassettes - test_handle_session_error_during_download: uses cassette with 401 response during download - test_handle_session_error_during_photo_iteration: uses cassette with 401 response during photo listing - test_handle_session_error_during_download_name_id7: uses cassette for name-id7 file match policy - test_handle_session_error_during_photo_iteration_name_id7: uses cassette for name-id7 with listing error All tests properly trigger PyiCloudAPIResponseException from HTTP responses in cassettes without needing to mock the exception directly. This proves that cassettes can simulate session errors when the error handling code is active. Note: One test (download_name_id7) needs cassette completion for the second download attempt after re-authentication. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Since MAX_RETRIES is set to 0 and retry logic was removed, the time.sleep mocks and sleep count assertions are no longer needed. The tests now only verify that re-authentication happens (by counting 'Authenticating...' messages) and that the session error message appears in the output. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Remove mock.patch.object for send_verification_code in test_2sa_flow_failed_send_code - Create dedicated VCR cassette (2sa_flow_failed_send_code.yml) with failed response - Cassette simulates service unavailable (success:false) response - Remove unnecessary mock import from test file This demonstrates that mocks can be replaced with more realistic VCR cassettes that test the full HTTP request/response handling stack. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Use consistent error response format with errorTitle, errorMessage, and errorCode - Match the structure used in other Apple API error responses - Remove incorrectly added 'status' field from headers section - Update content-length to match new response size The response now matches Apple's actual API format as seen in other captured cassettes like 2sa_flow_invalid_code.yml. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
… mocks - Created listing_albums_error.yml cassette with 500 status and API error response - Removed PhotoLibrary._fetch_folders mock and authenticate mock - Test now relies on cassette to trigger PyiCloudAPIResponseException - Removed unused PhotoLibrary import 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
The session.py code has two paths for error handling:
1. HTTP errors (non-OK status codes)
2. API errors in JSON payload with 200 status
The test_handle_albums_error was originally mocking an API error, so the
cassette should return HTTP 200 with error in the JSON payload, not HTTP 500.
Updated cassette response to: {"success":false,"error":"Api Error","errorCode":"100"}
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <[email protected]>
- test_handle_albums_error_name_id7: uses same cassette as albums_error test - test_handle_internal_error_during_download: uses cassette with 500 status during photo download Key learnings: - API errors (200 with error JSON) vs HTTP errors (4xx/5xx status) are handled differently in session.py - Download errors need HTTP status errors (500) since downloads use streaming and the error handling differs from regular JSON API calls - Removed unused PhotoLibrary and constants imports 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Download errors behave differently than API errors: - API endpoints (albums, photo listing): Can use HTTP 200 with error JSON - Download endpoints: Must use HTTP error status (4xx/5xx) because downloads use streaming and don't parse JSON responses The test behavior changes slightly: - Original mock: Raised PyiCloudAPIResponseException with "INTERNAL_ERROR" - Cassette: HTTP 500 results in "Could not find URL to download" message - Exit code changes from 1 to 0 as error is handled gracefully This is an acceptable difference as the error is still caught and handled, just reported differently. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Remove is_streaming_response check that was preventing exceptions for streaming downloads - Add special handling to exclude 404 errors from raising exceptions - Update cassette to return HTTP 500 for download errors - Ensure download errors raise exceptions and cause exit code 1 The is_streaming_response check was preventing the session from raising exceptions for HTTP errors on streaming downloads, causing inconsistent behavior between mock tests and cassette tests. Checking response status doesn't load the stream into memory since it's part of the HTTP headers. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Convert test_handle_internal_error_during_photo_iteration to use cassette - Convert test_handle_internal_error_during_download_name_id7 to use cassette - Add cassettes for simulating HTTP 500 errors during photo operations - Remove mock dependencies from converted tests - Clean up unused imports These tests now use actual HTTP responses via cassettes instead of mocks, providing more realistic testing of error handling behavior. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
The session error cassettes now include successful download after re-authentication, demonstrating proper retry behavior. Updated test comments to accurately reflect cassette contents and expected behavior. Both tests now: - Experience session error on first download attempt - Re-authenticate automatically - Successfully download on retry - Exit with code 0 (success) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Minor formatting improvements to session error test comments and file download expectations. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…cassettes The listing_photos_session_error_iteration.yml and listing_photos_session_error_iteration_name_id7.yml cassettes had delete file requests at the end that shouldn't be there. These have been removed. Both tests still pass after the cleanup. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Following the pattern established with AuthWithTokenFailed, replace other ADTs that simply wrap exceptions with unions that directly use response ADTs: - DeletePhotoResult now uses Response2SARequired | ResponseServiceNotActivated | etc. - PhotosRequestResult now uses response ADTs directly instead of PhotosRequestFailed - PhotoIterationResult now uses response ADTs directly instead of PhotoIterationFailed - AutodeleteResult now uses response ADTs directly instead of AutodeleteFailed This eliminates unnecessary exception wrapping layers and makes the ADT usage more consistent throughout the codebase. Pattern matching now directly handles specific response types rather than unpacking wrapped exceptions. Note: Some internal ADTs in photos.py (DownloadFailed, PhotoLibraryInitFailed, etc.) still wrap exceptions as they are internal to pyicloud_ipd and can be refactored separately if needed. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Replace albums, private_libraries, shared_libraries property usage with ADT methods - Replace photos property on PyiCloudService with get_photos_service() ADT method - All ADT results are propagated to core_single_run() where pattern matching handles errors - Exceptions are only raised in core_single_run(), maintaining ADT pattern throughout - Added comprehensive pattern matching for all service access error cases - Maintains backward compatibility through legacy properties This completes the migration to ADT-based error handling for all service properties, ensuring exceptions are only thrown at the top level in core_single_run().
- Changed download_media to use get_photos_service() instead of accessing photos property - Added proper ADT pattern matching for all response types - Handle PhotoLibraryNotFinishedIndexing by converting to ResponseServiceNotActivated - Propagate other ADT errors directly through the return chain - Maintains ADT-based error handling throughout the call chain This completes the refactoring to use ADTs instead of exceptions for the photos service access. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Changed from non-existent pyicloud_ipd.two_factor.data_types to pyicloud_ipd.sms - Fixes mypy --strict import-not-found error - All strict mypy checks now pass on both source and tests 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
… ADT method - Updated request_2sa() to use get_trusted_devices() and handle ADT result - Removed unused trusted_devices property from PyiCloudService - Removed unused PyiCloud2SARequiredException import - All tests pass, formatting and strict mypy checks clean 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…ic ADT types - Created specific ADT types for each authentication failure case: - AuthPasswordNotProvided for missing password - AuthInvalidCredentials for invalid credentials (401/421 errors) - AuthServiceNotActivated for service not activated - AuthServiceUnavailable for 503 errors - AuthAPIError for generic API errors - AuthUnexpectedError for unexpected errors - Updated authenticate_adt() to return specific ADTs instead of wrapped exceptions - Updated create_pyicloud_service_adt() to handle all new ADT types - Updated authenticator() and core_single_run() to pattern match on new ADTs - Fixed test to expect AuthInvalidCredentials instead of wrapped exception - All tests pass, strict mypy clean on code and tests This avoids the anti-pattern of wrapping exceptions in ADTs and provides more precise error types for better error handling. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Changed core_single_run return type to int | CoreSingleRunErrorResult - Added CoreSingleRunErrorResult union type to response_types.py - Updated caller with pattern matching to handle both int and ADT results - Maintained dual mode - still throws exceptions for backward compatibility - Infrastructure ready for gradual exception-to-ADT conversion - All tests pass including critical test_failed_auth_503_watch
…p 1) - Replaced sys.exit(1) with ADT return for AuthenticatorTwoSAExit - Tests still pass including critical test_failed_auth_503_watch - First successful exception-to-ADT conversion
- Changed line 1009 from raising PyiCloudConnectionException to returning ADT - Removed unused PyiCloudConnectionException import - All tests passing (218 passed, 2 skipped)
- Changed line 1010 from raising PyiCloudFailedLoginException to returning ADT - All tests passing (19 passed, 1 skipped)
- Changed line 1012 from raising PyiCloudServiceNotActivatedException to returning ADT - All tests passing (19 passed, 1 skipped)
- Changed line 1016 from raising PyiCloudAPIResponseException to returning ADT - Skipped AuthServiceUnavailable as it breaks watch functionality - All tests passing (19 passed, 1 skipped)
- Replace PyiCloudServiceNotActivatedException with ADT return - Add special logging in pattern matching to preserve error message - All tests passing (218 passed)
- Document successful conversion of 7/10 authentication errors - Identify 3 cases that must remain as exceptions - Add learnings about watch mode and MFA requirements - Note that service access errors need separate refactoring
- Replace AuthUnexpectedError with specific AuthConnectionError ADT - Only catch PyiCloudConnectionErrorException (network errors) with ADT - Let all other unexpected exceptions propagate and crash the program - This aligns with the principle that we only handle expected IO errors - Removes unnecessary complexity from error handling - All 218 tests passing
- Authentication now returns ADT results instead of throwing exceptions - Moved authentication code outside the try block - Removed PyiCloudFailedLoginException handler (no longer raised) - Converted PyiCloudFailedMFAException to direct ADT handling - Kept necessary exception handlers for service/IO errors - This completes the authentication exception-to-ADT refactoring - All 218 tests passing
…e_run - Replace PyiCloud2SARequiredException with Response2SARequired return in private libraries check - Replace PyiCloudServiceNotActivatedException with ResponseServiceNotActivated return in private libraries check - All tests pass with these minimal changes
…es check - Replace PyiCloudAPIResponseException with ResponseAPIError return - Replace PyiCloudServiceUnavailableException with ResponseServiceUnavailable return - All tests continue to pass
- Replace all four exception types with corresponding ADT returns - Maintains consistent error handling pattern - All tests pass
…h ADT return at line 1331
- Removed get_album_count function that raised exceptions - Replaced functional programming approach with direct for loop - Added inline error handling for session errors - Invalid global session errors now trigger re-authentication - Other API errors are logged and return error code directly - All tests pass, mypy strict mode passes
- Added error_occurred flag to track when retry is needed - Replaced exception raising in photo iteration with direct handling - Session errors trigger retry via flag instead of exception - WebUI errors that can be retried also use flag mechanism - Other errors return error code directly - All tests pass, mypy strict mode passes
- Replaced exception raising in download result handling with direct error handling - Used needs_retry flag to coordinate retry logic across nested loops - Session errors during download now trigger retry via flag mechanism - 500 errors and service unavailable return error code directly - Other API errors are logged but continue processing - Fixed retry logic to properly break from albums loop - All tests pass, mypy strict mode passes
- Replaced exception raising in delete photo result handling with direct error handling - Session errors during delete now trigger retry via needs_retry flag - 500 errors return error code 1 (important for delete operations) - Service unavailable returns error code 1 - Other API errors are logged but continue processing - All tests pass, mypy strict mode passes
- Replaced exception raising in autodelete result handling with direct error handling - Session errors trigger retry by continuing the while loop - WebUI errors that can be retried also continue the loop - Service unavailable returns error code 1 - Other errors return appropriate ADT results or error codes - Removed unused PyiCloud2SARequiredException import - All tests pass, mypy strict mode passes
Replace try-except block with direct ADT error handling throughout core_single_run function. All error cases now use pattern matching directly without exception catching. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
The --skip-created-before/after options interpret dates in local timezone, which is more intuitive for users. Updated tests to expect local timezone instead of UTC, making them pass on systems with any timezone. - Use get_localzone() for expected timezone in tests - Parse target dates as local midnight, not UTC midnight - Convert photo created timestamps from UTC to local for comparison - Remove unused zoneinfo import This preserves the user-friendly behavior where '2025-01-02' means 'January 2nd in my local time' rather than requiring users to think in UTC.
AndreyNikiforov
force-pushed
the
master
branch
from
f09784b to
dd5a421
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes timezone-related test failures by making tests timezone-aware instead of expecting UTC.
Problem
Tests fail on systems with non-UTC timezones because they expect dates to be parsed and displayed in UTC. However, the actual behavior (parsing dates in local timezone) is the correct, user-friendly approach and what users of the CLI will expect.
Solution
Update tests to expect local timezone instead of hardcoded UTC:
get_localzone()for expected timezone in assertionsWhy Local Timezone is Correct
When a user types
--skip-created-before 2025-01-02, they mean:Example: User in Tokyo (UTC+9) at 8am on Jan 2nd wants to skip yesterday's photos:
2025-01-02= midnight last night ✓2025-01-02 00:00 UTC= 9am today ✗This behavior is consistent with how the project handles dates elsewhere (folder structure, file timestamps).
Testing
scripts/testnow passes on any system timezone