test: add internal server coverage#3034
Conversation
📝 WalkthroughWalkthroughAdds a new test file for the internal/servers package, introducing fake tenant/bundle storage and permission invoker implementations plus request builders, and unit tests covering health checks, container wiring, HTTP naming, logging, and permission/tenancy/bundle server behaviors. ChangesServer behavior tests
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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.
🧹 Nitpick comments (2)
internal/servers/server_behavior_test.go (2)
499-501: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
reflect.DeepEqualinstead of pointer comparison for bundle assertion.
read.GetBundle() != bundlerelies on the fake store returning the exact same*v1.DataBundlepointer. This works today but is fragile — if the server or store ever clones the proto, the test fails for the wrong reason. The rest of the file already usesreflect.DeepEqualfor value comparisons (lines 365, 374, 443, 485).♻️ Suggested fix
- if read.GetBundle() != bundle { + if !reflect.DeepEqual(read.GetBundle(), bundle) { t.Fatalf("expected stored bundle, got %v", read.GetBundle())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/servers/server_behavior_test.go` around lines 499 - 501, The bundle assertion in the test should compare values instead of pointer identity, since `read.GetBundle()` may return an equivalent but different `*v1.DataBundle`. Update the assertion in the server behavior test to use `reflect.DeepEqual` for the `bundle` check, matching the existing value-based comparisons already used elsewhere in `server_behavior_test.go` and keeping the test tied to `GetBundle()` contents rather than object identity.
148-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
LookupEntityStreamis implemented in the fake but never exercised by any test.The fake implements
LookupEntityStream(lines 148-151) to satisfy thePermissioninterface, butTestPermissionServerPassesThroughInvokercovers every other RPC (Check, Expand, LookupEntity, LookupSubject, SubjectPermission) except the streamingLookupEntityStream. Consider adding a test that verifies the server forwards the request to the invoker and streams the response.Also applies to: 335-387
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/servers/server_behavior_test.go` around lines 148 - 151, Add test coverage for the streaming Permission RPC by exercising fakePermissionInvoker.LookupEntityStream from TestPermissionServerPassesThroughInvoker or a new focused test. Verify PermissionServer.LookupEntityStream forwards the request to the invoker, uses the fake to capture the request, and streams the response back to the client, similar to the existing coverage for Check, Expand, LookupEntity, LookupSubject, and SubjectPermission.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/servers/server_behavior_test.go`:
- Around line 499-501: The bundle assertion in the test should compare values
instead of pointer identity, since `read.GetBundle()` may return an equivalent
but different `*v1.DataBundle`. Update the assertion in the server behavior test
to use `reflect.DeepEqual` for the `bundle` check, matching the existing
value-based comparisons already used elsewhere in `server_behavior_test.go` and
keeping the test tied to `GetBundle()` contents rather than object identity.
- Around line 148-151: Add test coverage for the streaming Permission RPC by
exercising fakePermissionInvoker.LookupEntityStream from
TestPermissionServerPassesThroughInvoker or a new focused test. Verify
PermissionServer.LookupEntityStream forwards the request to the invoker, uses
the fake to capture the request, and streams the response back to the client,
similar to the existing coverage for Check, Expand, LookupEntity, LookupSubject,
and SubjectPermission.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 745f0ba1-f2bd-4ef4-8904-f756bbe1f984
📒 Files selected for processing (1)
internal/servers/server_behavior_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3034 +/- ##
==========================================
+ Coverage 74.67% 76.24% +1.58%
==========================================
Files 83 83
Lines 9212 9212
==========================================
+ Hits 6878 7023 +145
+ Misses 1798 1638 -160
- Partials 536 551 +15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit