chore(policy): consolidate sort integration helpers, add tiebreaker tests, add test cleanup #3403
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR refactors integration tests across seven files in ChangesSort Test Refactoring and Tie-Breaker Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 38 minutes and 51 seconds.Comment |
github-actions
Bot
added
comp:db
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the reliability and maintainability of the policy service's list operations and integration tests. By introducing a deterministic tiebreaker to SQL sorting logic, it ensures stable result ordering. Additionally, it refactors test helper methods to be more robust and includes automatic cleanup, which simplifies test maintenance and improves test suite stability. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. In tables where the timestamps align, / A tiebreaker makes the order fine. / With ID as the final guide, / No ambiguity can hide. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements deterministic sorting tie-breakers by adding id ASC to the ORDER BY clauses in SQL queries for Namespaces, Obligations, Registered Resources, and Key Access Servers, alongside updated integration tests. Feedback highlights that SQL tie-breakers for Attributes and Subject Mappings are missing, which will cause their new tests to fail. Additionally, the reviewer noted potential Go version compatibility issues with Go 1.23 slice functions, possible test flakiness due to high-precision timestamps, a potential panic in KAS key test helpers, and inconsistent timestamp generation within single requests.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements deterministic sorting for several list endpoints by adding a secondary 'id ASC' tiebreaker to SQL queries when 'created_at' timestamps are identical. It includes updates to the database layer for namespaces, obligations, registered resources, and KAS keys, along with corresponding integration tests and a new 'forceCreatedAtTie' utility. Feedback highlights that several entities—specifically attributes, key access servers, subject mappings, and subject condition sets—are missing the necessary database-level tiebreaker logic to support the newly added tests. Additionally, one test helper in the subject mappings suite remains unrefactored, creating an inconsistency with the rest of the integration test suite.
There was a problem hiding this comment.
Code Review
This PR introduces deterministic sorting by adding a secondary id ASC tiebreaker to several policy entity list queries and adds integration tests to verify this behavior. Feedback indicates that several SQL files (Attributes, Key Access Servers, Subject Mappings) were omitted from the update, leading to potential test failures. Additionally, a missing import in the attributes test and a table name typo in the subject mapping tests need to be addressed.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
service/integration/subject_mappings_test.go (1)
2774-2811:⚠️ Potential issue | 🟠 MajorSort-test cleanup leaks
SubjectConditionSetrows created viaNewSubjectConditionSetLine 2784 creates a new subject condition set per mapping, but Lines 2847-2848 only delete the subject mapping. Since deleting a mapping does not delete its condition set, this leaves orphan rows and pollutes later list/sort tests in the same suite.
💡 Proposed fix
func (s *SubjectMappingsSuite) deleteSortTestSubjectMappings(ids []string) { for _, id := range ids { - _, err := s.db.PolicyClient.DeleteSubjectMapping(s.ctx, id) + sm, err := s.db.PolicyClient.GetSubjectMapping(s.ctx, id) + s.Require().NoError(err) + + _, err = s.db.PolicyClient.DeleteSubjectMapping(s.ctx, id) s.Require().NoError(err) + + if scs := sm.GetSubjectConditionSet(); scs != nil && scs.GetId() != "" { + _, err = s.db.PolicyClient.DeleteSubjectConditionSet(s.ctx, scs.GetId()) + s.Require().NoError(err) + } } }Also applies to: 2844-2850
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/integration/subject_mappings_test.go` around lines 2774 - 2811, createSortTestSubjectMappings creates a new SubjectConditionSet for each mapping but test cleanup only deletes the mapping, leaving orphan SubjectConditionSet rows; update createSortTestSubjectMappings and the cleanup to capture the subject-condition-set id returned by CreateSubjectMapping (e.g., created.GetSubjectConditionSetId() or the appropriate field on the response) alongside created.GetId(), return or record both ids, and ensure the teardown calls PolicyClient.DeleteSubjectConditionSet (or the service method that deletes condition sets) for each captured SubjectConditionSet id in addition to deleting the SubjectMapping; reference createSortTestSubjectMappings, SubjectConditionSetCreate, CreateSubjectMapping and the mapping deletion code to implement this.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@service/integration/namespaces_test.go`:
- Around line 1416-1422: The helper deleteSortTestNamespaces only calls
s.db.PolicyClient.DeactivateNamespace which leaves rows visible to queries that
use ACTIVE_STATE_ENUM_ANY; update deleteSortTestNamespaces to also permanently
remove or purge the test rows after deactivation by invoking the corresponding
delete method on the PolicyClient (e.g., call s.db.PolicyClient.DeleteNamespace
or the service's purge/delete API for a namespace) for each id and assert no
error, so tests that list with ACTIVE_STATE_ENUM_ANY no longer see leftover
namespaces; ensure you check the returned error from both DeactivateNamespace
and the permanent delete call.
---
Outside diff comments:
In `@service/integration/subject_mappings_test.go`:
- Around line 2774-2811: createSortTestSubjectMappings creates a new
SubjectConditionSet for each mapping but test cleanup only deletes the mapping,
leaving orphan SubjectConditionSet rows; update createSortTestSubjectMappings
and the cleanup to capture the subject-condition-set id returned by
CreateSubjectMapping (e.g., created.GetSubjectConditionSetId() or the
appropriate field on the response) alongside created.GetId(), return or record
both ids, and ensure the teardown calls PolicyClient.DeleteSubjectConditionSet
(or the service method that deletes condition sets) for each captured
SubjectConditionSet id in addition to deleting the SubjectMapping; reference
createSortTestSubjectMappings, SubjectConditionSetCreate, CreateSubjectMapping
and the mapping deletion code to implement this.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aa93d939-9982-4bf3-bead-75a9ee122c27
📒 Files selected for processing (16)
service/integration/attributes_test.goservice/integration/kas_registry_key_test.goservice/integration/kas_registry_test.goservice/integration/namespaces_test.goservice/integration/obligations_test.goservice/integration/registered_resources_test.goservice/integration/subject_mappings_test.goservice/integration/utils.goservice/policy/db/key_access_server_registry.sql.goservice/policy/db/namespaces.sql.goservice/policy/db/obligations.sql.goservice/policy/db/queries/key_access_server_registry.sqlservice/policy/db/queries/namespaces.sqlservice/policy/db/queries/obligations.sqlservice/policy/db/queries/registered_resources.sqlservice/policy/db/registered_resources.sql.go
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
add deletion wrappers/helpers for tests missing it, and then added that func as a defer call in each test. namespaces and attributes use Deactivate __ (soft delete), the others use Delete __ (hard delete). the helpers/wrappers simply loop over the IDs and call those deletion functions to clear up the test DB
across all 7 list RPCs getting sort functionality. how it works: 1. create 3 rows without sleep gaps (same created_at) 2. sort the returned row IDs manually (slices.sorted) 3. call the API with created_at ASC 4. timestamps tie (intended), leaving the tiebreaker the sorting factor 5. compare returned IDs against the manual sorted order from earlier (id ASC), these should be the same
All 8 tiebreaker integration tests assume created_at timestamps collide when resources are created in a tight loop, but PostgreSQL's microsecond precision means they don't. The tiebreaker (id ASC) never fires, causing tests to assert random UUID order equals creation order — which fails ~83% of the time. Solution: add forceCreatedAtTie helper that forces all 3 IDs to the same created_at via raw SQL UPDATE. This guarantees the tiebreaker determines sort order and actually tests the feature. Also fixed KAS key test helper issues from code review: - Guard prefixes[0] access against empty slice - Capture time.Now().UnixNano() once per iteration instead of twice All tiebreaker tests now call forceCreatedAtTie before listing: - attributes_test.go: attribute_definitions - namespaces_test.go: attribute_namespaces - obligations_test.go: obligation_definitions - registered_resources_test.go: registered_resources - kas_registry_test.go: key_access_servers - kas_registry_key_test.go: key_access_server_keys - subject_mappings_test.go: subject_mappings and subject_condition_set
inconsistency pointed out by gemini. createSortTestSM and createSortTestSCS now accept []string prefixes
integration testing cleanup was "soft deleting" (setting active to false) which proved to be useless. added a new util func to hard delete the test data in the DB (which is what is performed on the other endpoints, just with pre-existing functions, the api doesn't let you hard delete for namespaces or attributes)
f7302cf to
0fdb7bc
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
dsm20
changed the title
dsm20
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
service/integration/namespaces_test.go (1)
476-510:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd deferred cleanup to these namespace sort tests.
These three cases create namespaces but never call
defer s.deleteSortTestNamespaces(ids). Because they list withACTIVE_STATE_ENUM_ANY, the leftovers remain query-visible for the rest of the suite.Suggested fix
func (s *NamespacesSuite) Test_ListNamespaces_SortByUnspecifiedDirection_DefaultsToDESC() { ids := s.createSortTestNamespaces([]string{"unspecified-dir-ns-0", "unspecified-dir-ns-1", "unspecified-dir-ns-2"}) + defer s.deleteSortTestNamespaces(ids) listRsp, err := s.db.PolicyClient.ListNamespaces(s.ctx, &namespaces.ListNamespacesRequest{ State: common.ActiveStateEnum_ACTIVE_STATE_ENUM_ANY, @@ func (s *NamespacesSuite) Test_ListNamespaces_SortByBothUnspecified_DefaultsToCreatedAtDESC() { ids := s.createSortTestNamespaces([]string{"both-unspecified-ns-0", "both-unspecified-ns-1", "both-unspecified-ns-2"}) + defer s.deleteSortTestNamespaces(ids) listRsp, err := s.db.PolicyClient.ListNamespaces(s.ctx, &namespaces.ListNamespacesRequest{ State: common.ActiveStateEnum_ACTIVE_STATE_ENUM_ANY, @@ func (s *NamespacesSuite) Test_ListNamespaces_SortOmitted() { ids := s.createSortTestNamespaces([]string{"sort-omitted-ns-0", "sort-omitted-ns-1", "sort-omitted-ns-2"}) + defer s.deleteSortTestNamespaces(ids) listRsp, err := s.db.PolicyClient.ListNamespaces(s.ctx, &namespaces.ListNamespacesRequest{ State: common.ActiveStateEnum_ACTIVE_STATE_ENUM_ANY,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/integration/namespaces_test.go` around lines 476 - 510, Each of the three tests (Test_ListNamespaces_SortByUnspecifiedDirection_DefaultsToDESC, Test_ListNamespaces_SortByBothUnspecified_DefaultsToCreatedAtDESC, and Test_ListNamespaces_SortOmitted) creates test namespaces via createSortTestNamespaces but never cleans them up; add a deferred cleanup by calling defer s.deleteSortTestNamespaces(ids) immediately after each ids := s.createSortTestNamespaces(...) line so the created namespaces are removed after the test.service/integration/attributes_test.go (1)
691-727:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore deferred cleanup in these three sort tests.
Unlike the earlier sort cases, these tests never call
defer s.deleteSortTestAttributes(ids). The created rows stay around for the rest of the suite and can bleed into later list/pagination assertions.Suggested fix
func (s *AttributesSuite) Test_ListAttributes_SortByUnspecifiedDirection_DefaultsToDESC() { nsID := s.createSortTestNamespace("sort-unspecified-dir") ids := s.createSortTestAttributes(nsID, []string{"unspecified-dir-attr-0", "unspecified-dir-attr-1", "unspecified-dir-attr-2"}) + defer s.deleteSortTestAttributes(ids) listRsp, err := s.db.PolicyClient.ListAttributes(s.ctx, &attributes.ListAttributesRequest{ Namespace: nsID, @@ func (s *AttributesSuite) Test_ListAttributes_SortByBothUnspecified_DefaultsToCreatedAtDESC() { nsID := s.createSortTestNamespace("sort-both-unspecified") ids := s.createSortTestAttributes(nsID, []string{"both-unspecified-attr-0", "both-unspecified-attr-1", "both-unspecified-attr-2"}) + defer s.deleteSortTestAttributes(ids) listRsp, err := s.db.PolicyClient.ListAttributes(s.ctx, &attributes.ListAttributesRequest{ Namespace: nsID, @@ func (s *AttributesSuite) Test_ListAttributes_SortOmitted() { nsID := s.createSortTestNamespace("sort-omitted") ids := s.createSortTestAttributes(nsID, []string{"omitted-sort-attr-0", "omitted-sort-attr-1", "omitted-sort-attr-2"}) + defer s.deleteSortTestAttributes(ids) listRsp, err := s.db.PolicyClient.ListAttributes(s.ctx, &attributes.ListAttributesRequest{ Namespace: nsID,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/integration/attributes_test.go` around lines 691 - 727, These three tests (Test_ListAttributes_SortByUnspecifiedDirection_DefaultsToDESC, Test_ListAttributes_SortByBothUnspecified_DefaultsToCreatedAtDESC, and Test_ListAttributes_SortOmitted) create test attributes but omit deferred cleanup, causing data to leak into later tests; fix by adding a defer s.deleteSortTestAttributes(ids) immediately after each call to s.createSortTestAttributes(...) (and if other tests use createSortTestNamespace add defer s.deleteSortTestNamespace(nsID) where missing) so the created rows are removed after the test completes.service/integration/subject_mappings_test.go (2)
1627-1666:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd missing cleanup defers in three SubjectConditionSets sort tests.
Line 1628, Line 1643, and Line 1658 create test subject condition sets without cleanup. These rows persist and can skew list-ordering and pagination assertions over time.
Patch
func (s *SubjectMappingsSuite) Test_ListSubjectConditionSets_SortByUnspecifiedDirection_DefaultsToDESC() { ids := s.createSortTestSubjectConditionSets([]string{"unspecified-dir-scs-0", "unspecified-dir-scs-1", "unspecified-dir-scs-2"}) + defer s.deleteSortTestSubjectConditionSets(ids) listRsp, err := s.db.PolicyClient.ListSubjectConditionSets(s.ctx, &subjectmapping.ListSubjectConditionSetsRequest{ @@ func (s *SubjectMappingsSuite) Test_ListSubjectConditionSets_SortByBothUnspecified_DefaultsToCreatedAtDESC() { ids := s.createSortTestSubjectConditionSets([]string{"both-unspecified-scs-0", "both-unspecified-scs-1", "both-unspecified-scs-2"}) + defer s.deleteSortTestSubjectConditionSets(ids) listRsp, err := s.db.PolicyClient.ListSubjectConditionSets(s.ctx, &subjectmapping.ListSubjectConditionSetsRequest{ @@ func (s *SubjectMappingsSuite) Test_ListSubjectConditionSets_SortOmitted() { ids := s.createSortTestSubjectConditionSets([]string{"sort-omitted-scs-0", "sort-omitted-scs-1", "sort-omitted-scs-2"}) + defer s.deleteSortTestSubjectConditionSets(ids) listRsp, err := s.db.PolicyClient.ListSubjectConditionSets(s.ctx, &subjectmapping.ListSubjectConditionSetsRequest{})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/integration/subject_mappings_test.go` around lines 1627 - 1666, The tests Test_ListSubjectConditionSets_SortByUnspecifiedDirection_DefaultsToDESC, Test_ListSubjectConditionSets_SortByBothUnspecified_DefaultsToCreatedAtDESC, and Test_ListSubjectConditionSets_SortOmitted create subject condition sets via createSortTestSubjectConditionSets but do not clean them up; add defers to remove the created entries to avoid polluting subsequent tests. After calling ids := s.createSortTestSubjectConditionSets(...), immediately iterate the returned ids and defer a cleanup call (e.g., for _, id := range ids { defer s.deleteSubjectConditionSet(id) } or the suite's appropriate deletion helper) so each created SubjectConditionSet is deleted when the test finishes. Ensure the defer uses the per-id variable correctly (capture loop variable) and reference the existing createSortTestSubjectConditionSets and the suite's delete/cleanup helper when implementing.
755-794:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd missing cleanup defers in three SubjectMappings sort tests.
Line 756, Line 771, and Line 786 create test subject mappings but never clean them up. This leaks rows and can destabilize later integration tests.
Patch
func (s *SubjectMappingsSuite) Test_ListSubjectMappings_SortByUnspecifiedDirection_DefaultsToDESC() { ids := s.createSortTestSubjectMappings([]string{"unspecified-dir-sm-0", "unspecified-dir-sm-1", "unspecified-dir-sm-2"}) + defer s.deleteSortTestSubjectMappings(ids) listRsp, err := s.db.PolicyClient.ListSubjectMappings(s.ctx, &subjectmapping.ListSubjectMappingsRequest{ @@ func (s *SubjectMappingsSuite) Test_ListSubjectMappings_SortByBothUnspecified_DefaultsToCreatedAtDESC() { ids := s.createSortTestSubjectMappings([]string{"both-unspecified-sm-0", "both-unspecified-sm-1", "both-unspecified-sm-2"}) + defer s.deleteSortTestSubjectMappings(ids) listRsp, err := s.db.PolicyClient.ListSubjectMappings(s.ctx, &subjectmapping.ListSubjectMappingsRequest{ @@ func (s *SubjectMappingsSuite) Test_ListSubjectMappings_SortOmitted() { ids := s.createSortTestSubjectMappings([]string{"sort-omitted-sm-0", "sort-omitted-sm-1", "sort-omitted-sm-2"}) + defer s.deleteSortTestSubjectMappings(ids) listRsp, err := s.db.PolicyClient.ListSubjectMappings(s.ctx, &subjectmapping.ListSubjectMappingsRequest{})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/integration/subject_mappings_test.go` around lines 755 - 794, The three tests (Test_ListSubjectMappings_SortByUnspecifiedDirection_DefaultsToDESC, Test_ListSubjectMappings_SortByBothUnspecified_DefaultsToCreatedAtDESC, Test_ListSubjectMappings_SortOmitted) call createSortTestSubjectMappings(...) to insert rows but never remove them; add a defer immediately after each ids := ... line to clean up the created IDs (e.g. defer s.deleteSubjectMappings(ids) or the existing test teardown helper your suite uses) so the created subject mappings are removed after the test finishes, ensuring no leaked rows affect other integration tests.service/integration/kas_registry_test.go (1)
1066-1079:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
defercleanup in three sort tests leaves orphaned DB rows
Test_ListKeyAccessServers_SortByUnspecifiedDirection_DefaultsToDESC(line 1067),Test_ListKeyAccessServers_SortByBothUnspecified_DefaultsToCreatedAtDESC(line 1082), andTest_ListKeyAccessServers_SortOmitted(line 1097) each callcreateSortTestKeyAccessServersbut never register adefer s.deleteSortTestKeyAccessServers(ids). Every other refactored sort test in this file defers cleanup. The PR objective explicitly states cleanup should be added to all sort tests. Accumulated rows from prior test runs can shift thecreated_at DESCwindow and cause flaky assertions.🐛 Proposed fix — add the missing defers
func (s *KasRegistrySuite) Test_ListKeyAccessServers_SortByUnspecifiedDirection_DefaultsToDESC() { ids := s.createSortTestKeyAccessServers([]string{"unspecified-dir-kas-0", "unspecified-dir-kas-1", "unspecified-dir-kas-2"}) + defer s.deleteSortTestKeyAccessServers(ids)func (s *KasRegistrySuite) Test_ListKeyAccessServers_SortByBothUnspecified_DefaultsToCreatedAtDESC() { ids := s.createSortTestKeyAccessServers([]string{"both-unspecified-kas-0", "both-unspecified-kas-1", "both-unspecified-kas-2"}) + defer s.deleteSortTestKeyAccessServers(ids)func (s *KasRegistrySuite) Test_ListKeyAccessServers_SortOmitted() { ids := s.createSortTestKeyAccessServers([]string{"sort-omitted-kas-0", "sort-omitted-kas-1", "sort-omitted-kas-2"}) + defer s.deleteSortTestKeyAccessServers(ids)Also applies to: 1081-1094, 1096-1105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/integration/kas_registry_test.go` around lines 1066 - 1079, In the three tests Test_ListKeyAccessServers_SortByUnspecifiedDirection_DefaultsToDESC, Test_ListKeyAccessServers_SortByBothUnspecified_DefaultsToCreatedAtDESC, and Test_ListKeyAccessServers_SortOmitted add a deferred cleanup after calling createSortTestKeyAccessServers by immediately calling defer s.deleteSortTestKeyAccessServers(ids) so the test-created rows are removed; locate the call to createSortTestKeyAccessServers(...) in each test and insert the defer statement referencing the returned ids to match the other refactored sort tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@service/integration/attributes_test.go`:
- Around line 1911-1914: The helper deleteSortTestAttributes currently only
removes rows from attribute_definitions (via forceDeleteRows) and thus leaks the
per-test namespace created by createSortTestNamespace plus any auto-seeded
actions; change deleteSortTestAttributes to accept the namespace id (nsID) and
delete the namespace row and its seeded actions as well (e.g. call
forceDeleteRows for "namespaces" and the table storing auto-seeded actions in
addition to "attribute_definitions"), keep the original deletion of
attribute_definitions using forceDeleteRows, and update all callers to pass the
nsID and call it with defer (defer s.deleteSortTestAttributes(nsID, ids)) so the
per-test namespace and actions are cleaned up immediately after each sort test.
---
Outside diff comments:
In `@service/integration/attributes_test.go`:
- Around line 691-727: These three tests
(Test_ListAttributes_SortByUnspecifiedDirection_DefaultsToDESC,
Test_ListAttributes_SortByBothUnspecified_DefaultsToCreatedAtDESC, and
Test_ListAttributes_SortOmitted) create test attributes but omit deferred
cleanup, causing data to leak into later tests; fix by adding a defer
s.deleteSortTestAttributes(ids) immediately after each call to
s.createSortTestAttributes(...) (and if other tests use createSortTestNamespace
add defer s.deleteSortTestNamespace(nsID) where missing) so the created rows are
removed after the test completes.
In `@service/integration/kas_registry_test.go`:
- Around line 1066-1079: In the three tests
Test_ListKeyAccessServers_SortByUnspecifiedDirection_DefaultsToDESC,
Test_ListKeyAccessServers_SortByBothUnspecified_DefaultsToCreatedAtDESC, and
Test_ListKeyAccessServers_SortOmitted add a deferred cleanup after calling
createSortTestKeyAccessServers by immediately calling defer
s.deleteSortTestKeyAccessServers(ids) so the test-created rows are removed;
locate the call to createSortTestKeyAccessServers(...) in each test and insert
the defer statement referencing the returned ids to match the other refactored
sort tests.
In `@service/integration/namespaces_test.go`:
- Around line 476-510: Each of the three tests
(Test_ListNamespaces_SortByUnspecifiedDirection_DefaultsToDESC,
Test_ListNamespaces_SortByBothUnspecified_DefaultsToCreatedAtDESC, and
Test_ListNamespaces_SortOmitted) creates test namespaces via
createSortTestNamespaces but never cleans them up; add a deferred cleanup by
calling defer s.deleteSortTestNamespaces(ids) immediately after each ids :=
s.createSortTestNamespaces(...) line so the created namespaces are removed after
the test.
In `@service/integration/subject_mappings_test.go`:
- Around line 1627-1666: The tests
Test_ListSubjectConditionSets_SortByUnspecifiedDirection_DefaultsToDESC,
Test_ListSubjectConditionSets_SortByBothUnspecified_DefaultsToCreatedAtDESC, and
Test_ListSubjectConditionSets_SortOmitted create subject condition sets via
createSortTestSubjectConditionSets but do not clean them up; add defers to
remove the created entries to avoid polluting subsequent tests. After calling
ids := s.createSortTestSubjectConditionSets(...), immediately iterate the
returned ids and defer a cleanup call (e.g., for _, id := range ids { defer
s.deleteSubjectConditionSet(id) } or the suite's appropriate deletion helper) so
each created SubjectConditionSet is deleted when the test finishes. Ensure the
defer uses the per-id variable correctly (capture loop variable) and reference
the existing createSortTestSubjectConditionSets and the suite's delete/cleanup
helper when implementing.
- Around line 755-794: The three tests
(Test_ListSubjectMappings_SortByUnspecifiedDirection_DefaultsToDESC,
Test_ListSubjectMappings_SortByBothUnspecified_DefaultsToCreatedAtDESC,
Test_ListSubjectMappings_SortOmitted) call createSortTestSubjectMappings(...) to
insert rows but never remove them; add a defer immediately after each ids := ...
line to clean up the created IDs (e.g. defer s.deleteSubjectMappings(ids) or the
existing test teardown helper your suite uses) so the created subject mappings
are removed after the test finishes, ensuring no leaked rows affect other
integration tests.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 326e172d-b541-4789-9358-b12e83695a00
📒 Files selected for processing (8)
service/integration/attributes_test.goservice/integration/kas_registry_key_test.goservice/integration/kas_registry_test.goservice/integration/namespaces_test.goservice/integration/obligations_test.goservice/integration/registered_resources_test.goservice/integration/subject_mappings_test.goservice/integration/utils.go
#3402 added more integration tests that needed cleanup calls
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
elizabethhealy
left a comment
elizabethhealy
left a comment
There was a problem hiding this comment.
lgtm, left an optional nit
2 participants
Proposed Changes
Consolidates integration test helpers across all policy sort test suites, adds proper test cleanup, and adds tiebreaker-specific integration tests that verify the
id ASCfallback produces deterministic ordering.Changes
Integration Test Helpers -- service/integration/utils.go
forceDeleteRows: hard-deletes rows by ID via raw SQL, bypassing the API's soft-delete/deactivate limitation for test cleanupforceCreatedAtTie: setscreated_atto a fixed timestamp for given IDs, guaranteeing that theORDER BYtiebreaker (id ASC) determines sort orderIntegration Test Consolidation -- service/integration/*_test.go
createSortTest*helpers from(label string)to(prefixes []string)across all RPCs, giving each test explicit control over resource namesdefercleanup calls to all sort integration tests viadeleteSortTest*helpersTest_List*_SortTieBreaker_CreatedAtWithIDFallbackfor all 8 RPCs: Namespaces, Attributes, KeyAccessServers, KasKeys, Obligations, RegisteredResources, SubjectMappings, SubjectConditionSetsTiebreaker Test Strategy
Each tiebreaker test:
created_attimestamps viaforceCreatedAtTiecreated_at ASCand asserts rows match the UUID-sorted orderThis proves the
id ASCtiebreaker added in #3402 produces deterministic results when the primary sort column ties.Summary by CodeRabbit
Release Notes