test(spanner): fix backup restore TimeoutError and premature instance cleanup#17150
test(spanner): fix backup restore TimeoutError and premature instance cleanup#17150sakthivelmanii wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the test suite to include TimeoutError in retry logic and extends the instance cleanup threshold from one to four hours. Feedback was provided regarding the exception handling in conftest.py, where catching the broad GoogleAPICallError might mask critical issues like permission or authentication failures; using more specific exceptions like NotFound and DeadlineExceeded was recommended.
|
|
||
| retry_cleanup(to_scrub.delete)() | ||
| except exceptions.NotFound: | ||
| except exceptions.GoogleAPICallError: |
There was a problem hiding this comment.
Catching exceptions.GoogleAPICallError is overly broad for a function named scrub_instance_ignore_not_found. This base class includes critical errors such as PermissionDenied or Unauthenticated, which, if ignored, could lead to silent failures and resource leaks in the test environment. If the goal is to handle timeouts during cleanup in addition to missing instances, it is safer to catch specific exceptions to avoid masking configuration or permission issues. As per repository rules, avoid broad exception blocks and consider logging the exception (e.g., using logger.warning) instead of returning silently to aid in debugging.
| except exceptions.GoogleAPICallError: | |
| except (exceptions.NotFound, exceptions.DeadlineExceeded): |
References
- Avoid broad except blocks that can mask underlying issues. Catching specific exceptions and logging them helps prevent masking critical failures like permission or authentication issues.
1 participant
No description provided.