test: add direct unit tests for tempo mixin (#882)#1059
test: add direct unit tests for tempo mixin (#882)#1059muddlebee merged 6 commits intoTracer-Cloud:mainfrom
Conversation
Greptile SummaryThis PR adds 6 unit tests for Confidence Score: 5/5Safe to merge — all findings are P2 style suggestions with no correctness issues. The tests are well-structured, cover all the key branches (not-configured, general exception, HTTP exception, success path, network failure, attribute edge cases), and the assertions correctly reflect the source implementation. The only gaps are an unused import and one missing enriched-trace field assertion, both non-blocking. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant T as TestTempoMixin
participant F as FakeGrafanaClient
participant M as TempoMixin
participant MR as _make_request (mocked)
participant RG as requests.get (mocked)
T->>F: query_tempo(service_name)
F->>M: is_configured check
alt not configured
M-->>T: {success: False, error: "not configured..."}
else configured
M->>MR: _make_request(url, params)
MR-->>M: {traces: [...]}
loop for each trace
M->>RG: requests.get(trace_url, headers, timeout)
RG-->>M: response (200 + JSON batches)
M->>M: _extract_span_attributes(span)
end
M-->>T: {success: True, traces: [...], total_traces, service_name}
end
Reviews (1): Last reviewed commit: "test: add direct unit tests for tempo mi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
sirohikartik
force-pushed
the
test/882-tempo-clean
branch
from
b852b83 to
c0ba7e8
Compare
| @@ -0,0 +1,170 @@ | |||
| """Unit tests for the Grafana Tempo mixin.""" | |||
There was a problem hiding this comment.
Missing from __future__ import annotations — required by the project's code style for all Python files.
| ] | ||
| } | ||
|
|
||
| attributes = client._extract_span_attributes(mock_span) |
There was a problem hiding this comment.
This asserts that a span missing a key field gets stored under the empty string "". That's documenting a silent collision bug in the production code — if two keyless attrs exist, the second silently overwrites the first. The test should instead assert that the entry is skipped (not present in the result), and the corresponding fix is to add if not key: continue in _extract_span_attributes.
| assert len(enriched_trace["spans"]) == 1 | ||
| span = enriched_trace["spans"][0] | ||
| assert span["name"] == "DB Query" | ||
| assert span["attributes"]["db.system"] == "postgresql" |
There was a problem hiding this comment.
No test covers the non-200 branch of _get_trace_details — when requests.get returns e.g. a 403/404, the function falls through to return {"spans": []} silently. Add a test that mocks mock_response.status_code = 404 and asserts the same empty-spans result, so that path is exercised.
|
Hey @muddlebee I've made the required changes. Please review and let me know if any further changes are required.
Thanks! |
|
@sirohikartik nice work. |
|
🎉 MERGED! @sirohikartik just shipped something. The diff gods are pleased. 🙌
👋 Join us on Discord - OpenSRE : hang out, contribute, or hunt for features and issues. Everyone's welcome. |
3 participants
Fixes #882
Describe the changes you have made in this PR -
Added direct unit tests for
TempoMixin(app/services/grafana/tempo.py).FakeGrafanaClientto isolate tests from the base Grafana client.Demo/Screenshot for feature changes and bug fixes -
===================================== test session starts =====================================
platform darwin -- Python 3.14.4, pytest-9.0.3, pluggy-1.6.0 -- /Users/kartiksirohi/OSS/opensre/env/bin/python3.14
cachedir: .pytest_cache
rootdir: /Users/kartiksirohi/OSS/opensre
configfile: pytest.ini
plugins: langsmith-0.7.36, anyio-4.13.0
collected 6 items
tests/services/test_grafana_tempo.py::TestTempoMixin::test_query_tempo_not_configured PASSED [ 16%]
tests/services/test_grafana_tempo.py::TestTempoMixin::test_query_tempo_general_exception PASSED [ 33%]
tests/services/test_grafana_tempo.py::TestTempoMixin::test_query_tempo_http_exception_with_response PASSED [ 50%]
tests/services/test_grafana_tempo.py::TestTempoMixin::test_query_tempo_successful_trace_parsing PASSED [ 66%]
tests/services/test_grafana_tempo.py::TestTempoMixin::test_get_trace_details_network_failure PASSED [ 83%]
tests/services/test_grafana_tempo.py::TestTempoMixin::test_extract_span_attributes_edge_cases PASSED [100%]
==================================== slowest 20 durations =====================================
(18 durations < 0.005s hidden. Use -vv to show these durations.)
====================================== 6 passed in 0.88s ======================================
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:
I needed to test the TempoMixin independently without making actual network calls to Grafana.
I chose to implement a FakeGrafanaClient class that inherits from TempoMixin.
I used unittest.mock.patch to intercept requests.get inside the _get_trace_details method. This allowed me to safely simulate a complex, deeply nested JSON response (batches -> scopeSpans -> spans) to ensure the attribute parsing works correctly.
I structured the tests to cover the "happy path" (successful parsing) as well as the failure envelopes (missing configuration, network timeouts, and 403 HTTP errors) as requested in the issue criteria.
Checklist before requesting a review