test: add failure path coverage for IMDS helpers#1154
test: add failure path coverage for IMDS helpers#1154muddlebee merged 1 commit intoTracer-Cloud:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds isolated unit tests to ensure the EC2 Instance Metadata Service (IMDS) helper functions fail quietly (return None) when urlopen() raises common network-related exceptions, aligning with the “no network calls in tests” requirement.
Changes:
- Import
_imds_tokenand_imds_getintotests/app/remote/test_server.py. - Add mocked tests asserting
_imds_token()returnsNonewhenurlopen()raisesURLError,TimeoutError, orOSError. - Add mocked tests asserting
_imds_get()returnsNonewhenurlopen()raisesURLError,TimeoutError, orOSError.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile SummaryThis PR adds six unit tests covering the failure paths of Confidence Score: 5/5Safe to merge — test-only change with no impact on production code. All findings are P2 style issues (import placement and missing type hints on inner helpers). The tests themselves are logically correct and the monkeypatching strategy is sound. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[_imds_token / _imds_get called] --> B[urllib.request.urlopen]
B -->|Success| C[return decoded string or None]
B -->|urllib.error.URLError| D[return None]
B -->|TimeoutError| E[return None]
B -->|OSError| F[return None]
subgraph Tests Added
T1[test_imds_token_returns_none_on_url_error]
T2[test_imds_token_returns_none_on_timeout]
T3[test_imds_token_returns_none_on_os_error]
T4[test_imds_get_returns_none_on_url_error]
T5[test_imds_get_returns_none_on_timeout]
T6[test_imds_get_returns_none_on_os_error]
end
T1 -.covers.-> D
T2 -.covers.-> E
T3 -.covers.-> F
T4 -.covers.-> D
T5 -.covers.-> E
T6 -.covers.-> F
Reviews (1): Last reviewed commit: "test: add failure path coverage for IMDS..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def test_imds_token_returns_none_on_url_error(monkeypatch: pytest.MonkeyPatch) -> None: | ||
| import urllib.error | ||
|
|
||
| def _raise_url_error(*args, **kwargs): | ||
| raise urllib.error.URLError("connection refused") | ||
|
|
||
| monkeypatch.setattr("urllib.request.urlopen", _raise_url_error) | ||
|
|
||
| assert _imds_token() is None |
There was a problem hiding this comment.
urllib.error is imported inside the test body on lines 221 and 249 rather than at the module level. The codebase style guide calls for imports to live at the top of the file. Moving them up keeps the test body clean and avoids the repeated local import.
Add import urllib.error to the existing import block at the top of the file, then remove the two inline import urllib.error statements from the test bodies.
Context Used: Code style and conventions (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| def _raise_url_error(*args, **kwargs): | ||
| raise urllib.error.URLError("connection refused") |
There was a problem hiding this comment.
The stub helpers (_raise_url_error, _raise_timeout, _raise_os_error) have no type annotations on their parameters or return type. The project style guide requires type hints on all function parameters and return values (-> None here). The same applies to the equivalent helpers in all six new tests.
| def _raise_url_error(*args, **kwargs): | |
| raise urllib.error.URLError("connection refused") | |
| def _raise_url_error(*args: object, **kwargs: object) -> None: | |
| raise urllib.error.URLError("connection refused") |
Context Used: Code style and conventions (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
@TejasS1233 pls address review comments and merge conflicts |
TejasS1233
force-pushed
the
test/imds-failure-paths
branch
from
066ef85 to
0d07186
Compare
|
Addressed review comments: moved urllib.error import to module level and added type annotations to inner helpers. Rebased onto latest main to resolve conflicts. |
TejasS1233
force-pushed
the
test/imds-failure-paths
branch
from
0d07186 to
1a0dad0
Compare
|
Force-pushed to clean up the commit history -- the branch was behind upstream and pulled in unrelated changes. The PR now contains only the IMDS failure path tests. |
|
LGTM. will wait for CI checks.. |
|
🎻 "The diff was clean, the tests did pass, the reviewer wept." That poem was about @TejasS1233's PR. 🥹
👋 Join us on Discord - OpenSRE : hang out, contribute, or hunt for features and issues. Everyone's welcome. |
Summary
Closes #1126