test(coralogix): add direct unit tests for CoralogixClient#1282
test(coralogix): add direct unit tests for CoralogixClient#1282muddlebee merged 2 commits intoTracer-Cloud:mainfrom
Conversation
Greptile SummaryThis PR adds 10 direct unit tests for Confidence Score: 4/5Safe to merge; only minor style and coverage gaps found, no functional bugs in production code. All findings are P2 (style/coverage). Mock targets are correct, all assertions match the actual client implementation, and the patch paths are accurate. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[test suite] --> B[query_logs tests]
A --> C[validate_access test]
A --> D[probe_access tests]
A --> E[build_coralogix_logs_query test]
B --> B1[success: parses NDJSON rows]
B --> B2[HTTP error: returns failure envelope]
B --> B3[empty response: 0 logs]
B --> B4[invalid JSON lines: silently skipped]
B --> B5[network exception: returns failure envelope]
C --> C1[passes through total + warnings]
D --> D1[missing: unconfigured client]
D --> D2[failed: validate_access fails]
D --> D3[passed: scope + row count in detail]
E --> E1[appends limit clause with application_name filter]
Reviews (1): Last reviewed commit: "test(coralogix): add direct unit tests f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| from unittest.mock import MagicMock, patch | ||
|
|
||
| import httpx | ||
| import pytest |
There was a problem hiding this comment.
The test imports CoralogixIntegrationConfig from app.integrations.config_models, but the client itself imports it from app.integrations.models (which re-exports it). Both resolve to the same class at runtime, but if the re-export path is ever changed or the two diverge, the test fixture will silently construct a different object than what the client is instantiated with in production. Using the same import path as the source module makes this coupling explicit.
| with patch("app.services.coralogix.client.httpx.post", return_value=mock_response) as mock_post: | ||
| result = client.query_logs("source logs") | ||
|
|
||
| mock_post.assert_called_once() | ||
| assert result["success"] is True |
There was a problem hiding this comment.
mock_post assertion placed outside the patch context
mock_post.assert_called_once() is called after the with patch(...) as mock_post: block exits — the patch has already been removed at that point. While the MagicMock object retains its call history and the assertion still passes, this pattern is unconventional and can mislead readers into thinking the assert runs while the patch is live. Moving it inside the with block matches the style used in the reference test_datadog_client.py.
| with patch("app.services.coralogix.client.httpx.post", return_value=mock_response) as mock_post: | |
| result = client.query_logs("source logs") | |
| mock_post.assert_called_once() | |
| assert result["success"] is True | |
| with patch("app.services.coralogix.client.httpx.post", return_value=mock_response) as mock_post: | |
| result = client.query_logs("source logs") | |
| mock_post.assert_called_once() |
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!
| # ------------------------- | ||
| # build_coralogix_logs_query (pure helper) | ||
| # ------------------------- | ||
|
|
||
|
|
||
| def test_build_coralogix_logs_query_appends_limit_clause() -> None: | ||
| result = build_coralogix_logs_query(application_name="opensre", limit=5) | ||
|
|
||
| assert result.startswith("source logs") | ||
| assert "filter $l.applicationname == 'opensre'" in result | ||
| assert result.endswith("limit 5") |
There was a problem hiding this comment.
build_coralogix_logs_query test doesn't cover raw_query passthrough or subsystem_name filter
The single helper test only exercises application_name + limit. Two important branches go untested: the raw_query non-empty early-return (which skips all filter building and delegates to _ensure_limit_clause), and the subsystem_name filter clause. A regression in either branch would not be caught by this suite.
|
🚀 Houston, we have a merge. @AniketR10 your PR is in orbit. Thanks for launching this one!
👋 Join us on Discord - OpenSRE : hang out, contribute, or hunt for features and issues. Everyone's welcome. |
2 participants
Fixes #877
Describe the changes you have made in this PR -
Adds direct unit tests for CoralogixClient at tests/services/test_coralogix_client.py. The client was only tested indirectly through the tool layer, so its return envelopes weren't pinned.
10 tests covering query_logs (success, HTTP error, empty, invalid JSON, network exception), validate_access, probe_access (missing/failed/passed), and the build_coralogix_logs_query helper.
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:
..."}, empty/garbage responses don't crash, and probe_access returns the right ProbeResult status for each branch. Style follows tests/services/test_datadog_client.py.Checklist before requesting a review