test: add unit tests for observability modules#53
Merged
nilushancosta merged 1 commit intoopenchoreo:mainfrom Mar 18, 2026
Merged
test: add unit tests for observability modules#53nilushancosta merged 1 commit intoopenchoreo:mainfrom
nilushancosta merged 1 commit intoopenchoreo:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds extensive unit tests across logs, metrics, and tracing modules and adjusts UpdateAlertRule error handling to return 400 BadRequest when the client error contains "not found". No public API signatures were changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
observability-logs-openobserve/internal/openobserve/client_test.go (1)
425-439: Tighten mock handlers to reject unexpected routes/methods.These handlers currently allow some unexpected requests (broad method match and no
defaultbranch), which can let tests pass even when request construction regresses.Example hardening pattern (apply similarly in the three tests)
func TestUpdateAlert_HTTPError(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch { case r.Method == "GET" && r.URL.Path == "/api/v2/default/alerts": resp := map[string]interface{}{ "list": []map[string]string{ {"alert_id": "alert-upd-err", "name": "my-alert"}, }, } json.NewEncoder(w).Encode(resp) - case r.Method == "PUT": + case r.Method == "PUT" && r.URL.Path == "/api/v2/default/alerts/alert-upd-err": w.WriteHeader(http.StatusBadRequest) w.Write([]byte("bad request")) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) } }))Also applies to: 480-494, 599-613
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@observability-logs-openobserve/internal/openobserve/client_test.go` around lines 425 - 439, TestUpdateAlert_HTTPError's httptest handler is too permissive (broad r.Method == "PUT" case and no default) so unexpected requests can sneak through; update the handler in TestUpdateAlert_HTTPError to match both method and exact path for the PUT branch (e.g., r.Method == "PUT" && r.URL.Path == "/api/v2/default/alerts/{id}" or the concrete path your client should call) and add a default branch that calls t.Fatalf or writes http.StatusNotFound for any unmatched method/path to fail the test; apply the same tightening pattern to the other two mock handlers referenced (the tests covering lines 480-494 and 599-613) so all handlers explicitly validate method+path and reject unexpected requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@observability-logs-openobserve/internal/handlers_test.go`:
- Around line 792-833: This test currently asserts a generic 500 for a missing
rule; update TestUpdateAlertRule_NotFound to assert a more specific response
(e.g., expect gen.UpdateAlertRule404JSONResponse or an upstream 400 response
when the upstream returns 400) instead of gen.UpdateAlertRule500JSONResponse, by
adjusting the assertion that checks the return type from
handler.UpdateAlertRule; do the same for the related tests (the other cases
around TestGetAlertRule_NotFound and TestUpdateAlertRule_InvalidError
referenced) so empty lookup results map to a 404 response and upstream 400
responses are preserved as 400s rather than normalized to 500. Ensure you
reference the handler.UpdateAlertRule call and the response type checks when
making the change.
In `@observability-tracing-openobserve/internal/openobserve/queries_test.go`:
- Around line 450-534: The tests currently call debugLogger() which writes noisy
output to stderr and never verifies that a debug log was actually emitted;
change each test (TestGenerateTracesListQuery_DebugLogging,
TestGenerateSpansListQuery_DebugLogging,
TestGenerateSpanDetailQuery_DebugLogging) to create a bytes.Buffer, construct an
slog.TextHandler that writes to that buffer (e.g., slog.NewTextHandler(&buf,
&slog.HandlerOptions{Level: slog.LevelDebug})), build a logger with
slog.New(handler), pass that logger into
generateTracesListQuery/generateSpansListQuery/generateSpanDetailQuery, and
after the call assert that buf.String() contains the expected debug output (for
example the SQL snippet you already check like "FROM mystream" or "trace_id =
'abc123'") so the test verifies a debug record was emitted instead of printing
to stderr.
---
Nitpick comments:
In `@observability-logs-openobserve/internal/openobserve/client_test.go`:
- Around line 425-439: TestUpdateAlert_HTTPError's httptest handler is too
permissive (broad r.Method == "PUT" case and no default) so unexpected requests
can sneak through; update the handler in TestUpdateAlert_HTTPError to match both
method and exact path for the PUT branch (e.g., r.Method == "PUT" && r.URL.Path
== "/api/v2/default/alerts/{id}" or the concrete path your client should call)
and add a default branch that calls t.Fatalf or writes http.StatusNotFound for
any unmatched method/path to fail the test; apply the same tightening pattern to
the other two mock handlers referenced (the tests covering lines 480-494 and
599-613) so all handlers explicitly validate method+path and reject unexpected
requests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1bfec8f-17d6-49c0-baa9-93bf5db5655e
📒 Files selected for processing (9)
observability-logs-openobserve/internal/handlers_test.goobservability-logs-openobserve/internal/observer/client_test.goobservability-logs-openobserve/internal/openobserve/client_test.goobservability-metrics-prometheus/internal/config_test.goobservability-metrics-prometheus/internal/handlers_test.goobservability-metrics-prometheus/internal/observer/client_test.goobservability-tracing-openobserve/internal/handlers_test.goobservability-tracing-openobserve/internal/openobserve/client_test.goobservability-tracing-openobserve/internal/openobserve/queries_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Signed-off-by: Nilushan Costa <nilushan@wso2.com>
nilushancosta
force-pushed
the
observability
branch
from
03380c9 to
5b875fa
Compare
akila-i
approved these changes
Mar 18, 2026
There was a problem hiding this comment.
♻️ Duplicate comments (2)
observability-logs-openobserve/internal/handlers_test.go (1)
1070-1092:⚠️ Potential issue | 🟠 MajorThese tests lock broad 500 mappings for client/upstream errors.
DeleteAlertRulenot-found-like failures andCreateAlertRuleupstream 400 are asserted as 500 here, which bakes generic internal-error semantics into the suite.Also applies to: 1094-1147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@observability-logs-openobserve/internal/handlers_test.go` around lines 1070 - 1092, The tests currently assert that upstream "not found" and client 400 errors surface as generic 500 responses; update the test cases for DeleteAlertRule and CreateAlertRule to simulate appropriate upstream status codes and assert the corresponding specific handler response types (e.g., replace expecting gen.DeleteAlertRule500JSONResponse with the more specific gen.DeleteAlertRule404JSONResponse or the proper 4xx response type for CreateAlertRule). Locate the tests around TestDeleteAlertRule_Error and the CreateAlertRule tests that use NewLogsHandler and openobserve.NewClient, change the httptest server responses to return realistic status codes/body and update the resp type assertions to match the handler's intended 4xx mappings instead of forcing 500.observability-logs-openobserve/internal/openobserve/client_test.go (1)
504-519:⚠️ Potential issue | 🟠 MajorMake config-error test deterministic (avoid localhost transport dependency).
This test can pass for the wrong reason (connection failure) even if config validation regresses. Use an
httptest.Serverand assert no request is made when config generation fails.Proposed deterministic test patch
func TestCreateAlert_ConfigError(t *testing.T) { - client := newTestClient("http://localhost:1") + requestHit := make(chan struct{}, 1) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestHit <- struct{}{} + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":"unexpected"}`)) + })) + defer server.Close() + + client := newTestClient(server.URL) enabled := true name := "test" // Invalid operator causes config generation to fail _, err := client.CreateAlert(context.Background(), LogAlertParams{ Name: &name, Operator: "invalid_op", Window: "5m", Interval: "1m", Enabled: &enabled, }) if err == nil { t.Fatal("expected error for invalid config") } + select { + case <-requestHit: + t.Fatal("expected config validation failure before any HTTP request") + default: + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@observability-logs-openobserve/internal/openobserve/client_test.go` around lines 504 - 519, The test TestCreateAlert_ConfigError currently uses newTestClient("http://localhost:1") making it non-deterministic; replace that with an httptest.Server whose handler fails the test if invoked, pass server.URL into newTestClient, and defer server.Close(); then call client.CreateAlert with the invalid LogAlertParams (Operator: "invalid_op") and assert err != nil — this ensures config generation fails before any network request and the server handler guarantees no request was made. Use the existing test name TestCreateAlert_ConfigError and the CreateAlert/LogAlertParams symbols to locate and update the test.
🧹 Nitpick comments (2)
observability-logs-openobserve/internal/handlers_test.go (1)
830-832: Strengthen the not-found update assertion to include payload fields.This test currently checks only response type. Also assert
Title == BadRequestandMessage == "alert rule not found"so the handler contract change is fully protected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@observability-logs-openobserve/internal/handlers_test.go` around lines 830 - 832, The test only checks the response type; strengthen it by casting resp to gen.UpdateAlertRule400JSONResponse, extracting resp.Payload and asserting resp.Payload.Title == "BadRequest" and resp.Payload.Message == "alert rule not found" (using t.Fatalf or t.Errorf on mismatch) so the handler contract for UpdateAlertRule400JSONResponse is fully verified; locate the assertion near the existing type check for resp and add the payload field comparisons.observability-logs-openobserve/internal/handlers.go (1)
267-271: Avoid coupling not-found mapping to error message text.This branch depends on
strings.Contains(err.Error(), "not found"), which is brittle across wrapper/message changes in the OpenObserve client. Prefer a typed/sentinel error anderrors.Is(...)for stable status mapping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@observability-logs-openobserve/internal/handlers.go` around lines 267 - 271, Replace the brittle string check on err.Error() with a typed error comparison using errors.Is to detect the not-found case: import "errors" and use errors.Is(err, <client>.ErrNotFound) (or the OpenObserve client's sentinel/not-found error value or helper, e.g., openobserve.ErrNotFound or openobserve.IsNotFound(err)) in the branch that currently returns gen.UpdateAlertRule400JSONResponse so the same response is returned when errors.Is(...) is true; retain the existing returned gen.UpdateAlertRule400JSONResponse structure and fall back to the existing error handling for other errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@observability-logs-openobserve/internal/handlers_test.go`:
- Around line 1070-1092: The tests currently assert that upstream "not found"
and client 400 errors surface as generic 500 responses; update the test cases
for DeleteAlertRule and CreateAlertRule to simulate appropriate upstream status
codes and assert the corresponding specific handler response types (e.g.,
replace expecting gen.DeleteAlertRule500JSONResponse with the more specific
gen.DeleteAlertRule404JSONResponse or the proper 4xx response type for
CreateAlertRule). Locate the tests around TestDeleteAlertRule_Error and the
CreateAlertRule tests that use NewLogsHandler and openobserve.NewClient, change
the httptest server responses to return realistic status codes/body and update
the resp type assertions to match the handler's intended 4xx mappings instead of
forcing 500.
In `@observability-logs-openobserve/internal/openobserve/client_test.go`:
- Around line 504-519: The test TestCreateAlert_ConfigError currently uses
newTestClient("http://localhost:1") making it non-deterministic; replace that
with an httptest.Server whose handler fails the test if invoked, pass server.URL
into newTestClient, and defer server.Close(); then call client.CreateAlert with
the invalid LogAlertParams (Operator: "invalid_op") and assert err != nil — this
ensures config generation fails before any network request and the server
handler guarantees no request was made. Use the existing test name
TestCreateAlert_ConfigError and the CreateAlert/LogAlertParams symbols to locate
and update the test.
---
Nitpick comments:
In `@observability-logs-openobserve/internal/handlers_test.go`:
- Around line 830-832: The test only checks the response type; strengthen it by
casting resp to gen.UpdateAlertRule400JSONResponse, extracting resp.Payload and
asserting resp.Payload.Title == "BadRequest" and resp.Payload.Message == "alert
rule not found" (using t.Fatalf or t.Errorf on mismatch) so the handler contract
for UpdateAlertRule400JSONResponse is fully verified; locate the assertion near
the existing type check for resp and add the payload field comparisons.
In `@observability-logs-openobserve/internal/handlers.go`:
- Around line 267-271: Replace the brittle string check on err.Error() with a
typed error comparison using errors.Is to detect the not-found case: import
"errors" and use errors.Is(err, <client>.ErrNotFound) (or the OpenObserve
client's sentinel/not-found error value or helper, e.g., openobserve.ErrNotFound
or openobserve.IsNotFound(err)) in the branch that currently returns
gen.UpdateAlertRule400JSONResponse so the same response is returned when
errors.Is(...) is true; retain the existing returned
gen.UpdateAlertRule400JSONResponse structure and fall back to the existing error
handling for other errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff90f6dc-3ae0-4e2a-b573-5b9ec75814a7
📒 Files selected for processing (10)
observability-logs-openobserve/internal/handlers.goobservability-logs-openobserve/internal/handlers_test.goobservability-logs-openobserve/internal/observer/client_test.goobservability-logs-openobserve/internal/openobserve/client_test.goobservability-metrics-prometheus/internal/config_test.goobservability-metrics-prometheus/internal/handlers_test.goobservability-metrics-prometheus/internal/observer/client_test.goobservability-tracing-openobserve/internal/handlers_test.goobservability-tracing-openobserve/internal/openobserve/client_test.goobservability-tracing-openobserve/internal/openobserve/queries_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- observability-tracing-openobserve/internal/openobserve/queries_test.go
- observability-metrics-prometheus/internal/observer/client_test.go
- observability-tracing-openobserve/internal/openobserve/client_test.go
- observability-metrics-prometheus/internal/handlers_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Add more unit tests for observability modules
openchoreo/openchoreo#2755
Goals
Increase test coverage
Approach
Added Go unit tests for observability-logs-openobserve, observability-metrics-prometheus and observability-tracing-openobserve modules
Summary by CodeRabbit
Bug Fixes
Tests