test(tracer): add unit tests for get_logs parameter construction#983
Conversation
Introduces a \DummyTracerClient\ with a mocked \_get\ method to isolate and test the \TracerLogsMixin\ without making real HTTP requests. Coverage includes default parameter construction (size=100), \ race_id\ precedence, and \ un_id\ fallback behavior. Closes Tracer-Cloud#890
Greptile SummaryAdds three focused unit tests for Confidence Score: 5/5Safe to merge; test logic is correct and all findings are minor P2 style/coverage suggestions. All three assertions match the actual implementation behaviour. No P0/P1 defects were found. The remaining comments are about future-proofing the dummy fixture and adding a missing size parameter test case — neither blocks correctness today. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant T as Test
participant D as DummyTracerClient
participant M as TracerLogsMixin.get_logs()
participant G as _get (MagicMock)
T->>D: DummyTracerClient()
Note over D: org_id = "test_org_123"<br/>_get = MagicMock
T->>M: get_logs(trace_id?, run_id?, size=100)
M->>M: build params {orgId, size}
alt trace_id provided
M->>M: params[runId] = trace_id
else run_id provided
M->>M: params[runId] = run_id
end
M->>G: _get("/api/opensearch/logs", params)
G-->>M: {success: True, data: []}
M-->>T: returns dict
T->>G: assert_called_once_with(path, expected_params)
Reviews (1): Last reviewed commit: "test(tracer): add unit tests for get_log..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| client._get.assert_called_once_with( | ||
| "/api/opensearch/logs", | ||
| {"orgId": "test_org_123", "size": 100, "runId": "trace_abc"} | ||
| ) | ||
|
|
||
|
|
||
| def test_get_logs_run_id_fallback(): |
There was a problem hiding this comment.
The test name test_get_logs_trace_id_precedence asserts "runId": "trace_abc", which is technically correct given the implementation maps trace_id → params["runId"]. However the test name and docstring say "precedence" but the assertion doesn't actually verify that run_id="run_xyz" was ignored — it only checks that the final dict value equals the trace_id value. Adding an explicit assertNotIn or checking the value isn't "run_xyz" would make the intent clearer.
More importantly, the assertion key "runId" for a value that came from the trace_id argument mirrors a potential naming confusion in the underlying implementation. If this mapping is intentional, a brief comment documenting why trace_id feeds the runId field would help future readers.
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!
| class DummyTracerClient(TracerLogsMixin): | ||
| """Dummy client to isolate and test the TracerLogsMixin.""" | ||
|
|
||
| def __init__(self): | ||
| self.org_id = "test_org_123" | ||
| self._get = MagicMock(return_value={"success": True, "data": []}) |
There was a problem hiding this comment.
super().__init__() skipped silently
DummyTracerClient.__init__ manually sets self.org_id and self._get without calling super().__init__(). TracerClientBase.__init__ initialises self.base_url, self.organization_slug, and an httpx.Client. While get_logs only relies on org_id and _get, any future mixin that touches self.base_url or self.organization_slug will raise an AttributeError silently with this pattern. Consider using unittest.mock.patch or a @pytest.fixture with spec=TracerClientBase to prevent this drift.
|
|
||
| def test_get_logs_default_size(): | ||
| """Cover default size.""" | ||
| client = DummyTracerClient() | ||
| client.get_logs() | ||
|
|
||
| client._get.assert_called_once_with( | ||
| "/api/opensearch/logs", | ||
| {"orgId": "test_org_123", "size": 100} |
There was a problem hiding this comment.
size parameter
The suite tests the default size=100 but never passes an explicit size (e.g. client.get_logs(size=50)). The current three tests would still pass even if the size argument were completely ignored by the implementation. Adding one test for a custom size would close this gap.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| from app.services.tracer_client.tracer_logs import TracerLogsMixin | ||
|
|
||
|
|
||
| class DummyTracerClient(TracerLogsMixin): |
VaibhavUpreti
left a comment
VaibhavUpreti
left a comment
There was a problem hiding this comment.
awesome @Piyushtiwari919 , merging this for velocity, I'll fix the changes on main
|
Thanks @VaibhavUpreti , sounds great. Thanks for stepping in to handle the final linter adjustments. Have a great week! |
3 participants
Introduces a
DummyTracerClientwith a mocked_getmethod to isolate and test theTracerLogsMixinwithout making real HTTP requests. Coverage includes default parameter construction (size=100),trace_idprecedence, andrun_idfallback behavior.Fixes #890
Describe the changes you have made in this PR -
tests/services/tracer_client/test_tracer_logs.pyDummyTracerClientclass to attach the mixin and mock the internal_getnetwork call usingMagicMock.size=100,orgId).trace_idoverridesrun_idwhen both are passed).run_idis used whentrace_idis missing).rufflinting andpytestcoverage requirements.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:
TracerLogsMixinwithout triggering real HTTP requests to an OpenSearch database, which would make the tests flaky and dependent on network status.MagicMockacts as a spy, allowing us to intercept the_getcall and assert exactly what parameters were passed. Creating aDummyTracerClientis the cleanest way to test a standalone mixin.DummyTracerClientinitializes the spy. The three test functions handle the empty state, the collision state (both IDs), and the fallback state.Checklist before requesting a review