chore: add validation tests#111
Merged
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
6 tasks
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (2)
tests/feature/telemetry_test.go (1)
31-33: Prefer eventual polling over fixed 11s sleep.
A static wait slows the suite and can still flake. Use an eventual check against one telemetry backend signal before assertions.♻️ Example approach
- // Wait for telemetry data to be exported - time.Sleep(11 * time.Second) + // Wait until telemetry data is observable (less flaky than fixed sleep) + s.Eventually(func() bool { + resp, err := facades.Http().Get("http://localhost:9090/api/v1/query?query=grpc_controller_total") + if err != nil { + return false + } + body, err := resp.Body() + if err != nil { + return false + } + return len(body) > 0 + }, 20*time.Second, 1*time.Second)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/feature/telemetry_test.go` around lines 31 - 33, Replace the fixed time.Sleep(11 * time.Second) in tests/feature/telemetry_test.go with an eventual-polling loop that repeatedly checks the telemetry backend for the expected signal until a deadline (e.g. using time.After for timeout and time.Tick for polling); locate the Sleep call and wrap the existing assertions to run only after the polling helper (e.g. waitForTelemetrySignal or poll until checkTelemetryExport returns true) succeeds, returning a test failure on timeout so the test stops waiting and avoids flakiness and long fixed sleeps.tests/feature/validation_test.go (1)
401-416: Prefer asserting decoded JSON instead of raw response strings.These exact string comparisons are brittle to serializer formatting/key-order changes and make failures harder to read. Unmarshal the body and compare the error object shape instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/feature/validation_test.go` around lines 401 - 416, The test is brittle because it compares raw JSON strings from resp.Content(); instead decode the response into a Go value and assert the structure instead: call resp.Content() (or read resp.Body), json.Unmarshal into a map[string]interface{} (or a defined error struct), use s.Require().NoError(err) on unmarshal, and then assert the presence and values of keys like "message", "code", "date", and "name" (used in the ValidateRequest subtest and the second POST response) rather than comparing the exact serialized string; update the s.Equal assertions to compare the decoded object fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/http/controllers/validation_controller.go`:
- Around line 164-181: The handler currently uses the user-controlled variable
"rule" directly when calling ctx.Request().Validate and building
validation.Messages, which allows execution of arbitrary validators; fix it by
implementing an explicit allowlist (e.g., a slice or map named allowedRules or a
helper isAllowedRule(rule string) bool) containing only permitted rule
identifiers, check rule against that allowlist before proceeding, return a 400
error if not allowed, and only then assemble options (validation.Messages) and
call ctx.Request().Validate with the safe, mapped rule value (keep using "f" as
the field key and the existing variables validator, options, and
validation.Messages).
In `@tests/feature/validation_test.go`:
- Around line 256-259: The test cases for the escape filters (tests named
"escapeJs", "escapeJS", "escapeHtml", "escapeHTML") only assert non-empty via
passRule: "required", so a no-op escape would still pass; update each case to
assert the actual escaped output (add a passAssert or use a stronger passRule)
that verifies the value has been transformed (e.g., contains expected escape
sequences like < > for HTML or backslash/escaped quotes for JS) so the
tests validate the escaping behavior rather than just non-emptiness.
- Around line 225-234: The assertions in the passAssert closures (e.g., for test
cases named
"int","toInt","uint","toUint","int64","toInt64","float","toFloat","bool","toBool","str2time","strToTime")
use cast.ToX helpers which mask whether the filter actually converted the value;
update those assertions to check concrete types/values without casting (or
change the test to bind the filtered result into a typed struct field and assert
that field's static type/value), e.g., replace require.Equal(t, 12,
cast.ToInt(actual)) with an assertion that actual is an int (and equals 12) or
use a typed struct in the test harness to verify the conversion occurred. Ensure
you update all passAssert closures referenced (including the other locations
noted) to remove cast.* calls and assert the concrete type/value directly.
---
Nitpick comments:
In `@tests/feature/telemetry_test.go`:
- Around line 31-33: Replace the fixed time.Sleep(11 * time.Second) in
tests/feature/telemetry_test.go with an eventual-polling loop that repeatedly
checks the telemetry backend for the expected signal until a deadline (e.g.
using time.After for timeout and time.Tick for polling); locate the Sleep call
and wrap the existing assertions to run only after the polling helper (e.g.
waitForTelemetrySignal or poll until checkTelemetryExport returns true)
succeeds, returning a test failure on timeout so the test stops waiting and
avoids flakiness and long fixed sleeps.
In `@tests/feature/validation_test.go`:
- Around line 401-416: The test is brittle because it compares raw JSON strings
from resp.Content(); instead decode the response into a Go value and assert the
structure instead: call resp.Content() (or read resp.Body), json.Unmarshal into
a map[string]interface{} (or a defined error struct), use
s.Require().NoError(err) on unmarshal, and then assert the presence and values
of keys like "message", "code", "date", and "name" (used in the ValidateRequest
subtest and the second POST response) rather than comparing the exact serialized
string; update the s.Equal assertions to compare the decoded object fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fc6f94eb-7b2d-4f31-b865-3ea0be5bdf23
📒 Files selected for processing (9)
app/filters/append_suffix.goapp/http/controllers/validation_controller.gobootstrap/app.gobootstrap/filters.goconfig/telemetry.gogo.modroutes/api.gotests/feature/telemetry_test.gotests/feature/validation_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
1 participant
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.
📑 Description
@coderabbitai summary
✅ Checks