test: add direct unit tests for app/services/honeycomb/client.py #974
Conversation
|
cc : @VaibhavUpreti @rrajan94 PTAL! |
Greptile SummaryThis PR adds 14 unit tests for Confidence Score: 5/5Safe to merge; all findings are P2 coverage gaps with no production-code changes. The PR only adds test code and all identified issues are missing test cases (P2), not correctness bugs. No production code is modified, so there is no merge risk. tests/services/test_honeycomb_client.py — polling loop, run_query failure propagation, and query_traces empty-filter branch need additional test cases. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[run_query] --> B[create_query]
B -->|success=False| Z1[return error]
B -->|success=True| C[create_query_result]
C -->|success=False| Z2[return error ⚠️ no test]
C -->|success=True| D{complete?}
D -->|Yes — break| E[extract results]
D -->|No| F[time.sleep + get_query_result]
F -->|success=False| Z3[return error]
F -->|success=True| D
D -->|loop exhausted for/else| Z4[timeout error ⚠️ no test]
E --> G[return success]
style Z2 fill:#ffe0b2
style Z4 fill:#ffe0b2
Reviews (1): Last reviewed commit: "fix: resolve ruff lint and format issues..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "result": {"id": "r1", "complete": True, "data": {"results": [{"data": {"val": 10}}]}}, | ||
| } | ||
| ) | ||
|
|
||
| # We also need to mock time.sleep to speed up tests if it ever hits it | ||
| with patch("time.sleep"): | ||
| result = client.run_query({"calculations": []}) | ||
|
|
||
| assert result["success"] is True | ||
| assert result["query_id"] == "q1" | ||
| assert result["query_result_id"] == "r1" | ||
| assert result["results"] == [{"val": 10}] | ||
|
|
||
|
|
||
| def test_query_traces_success(client: HoneycombClient) -> None: | ||
| client.run_query = MagicMock(return_value={"success": True, "results": []}) | ||
|
|
There was a problem hiding this comment.
query_traces empty-filter branch not tested
When query_traces is called with no service_name and no trace_id, the client returns {"success": False, "error": "Honeycomb trace queries require a service_name or trace_id."}. This guard branch has no test. Adding a simple case like client.query_traces() asserting result["success"] is False would close the gap.
Context Used: Testing conventions and patterns (source)
|
@muddlebee PTAL! |
|
@gitsofaryan pls fix the greptile review points? |
|
@muddlebee addressed! |
|
@gitsofaryan CI still failing |
|
@muddlebee fixed! |
|
@gitsofaryan great work. thank you 🎸 |
2 participants
Fixes #883
Describe the changes you have made in this PR -
Added direct unit tests for the Honeycomb service client in
app/services/honeycomb/client.py. These tests cover key behaviors including authentication validation, query creation, and query result retrieval (polling). The implementation usesunittest.mockto ensure no network access is required, and covers both success paths and error handling for HTTP failures and generic exceptions.Demo/Screenshot for feature changes and bug fixes -
Code Understanding and AI Usage
Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?
If you used AI assistance:
Explain your implementation approach:
The tests use
pytestandunittest.mockto isolate theHoneycombClientfrom the external API and thehttpxlibrary. I focused on covering all branch points in the exception handling logic to ensure that HTTP errors (4xx/5xx) and network-level failures are handled consistently and return the expected error structure. High-level methods likerun_queryandquery_tracesare also tested to ensure correct internal logic and payload formatting.Checklist before requesting a review