Add complex object unification tests #133
Conversation
Co-authored-by: VivekVinushanth <[email protected]>
Co-authored-by: VivekVinushanth <[email protected]>
|
|
WalkthroughThe PR adds a direct dependency on Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (3)
test/Unification_Scenarios.MD (1)
59-68: Documentation looks good.The new scenarios 10–19 are well documented and align with the integration tests. As a minor style note, "merge together" (lines 61, 62) could be shortened to just "merge" for conciseness.
test/integration/complex_unification_test.go (2)
138-151: Check errors from profile operations to catch failures early.Ignoring errors from
CreateProfileandGetProfilecan hide infrastructure issues or service bugs. Consider usingrequire.NoErrorfor these critical operations.- prof1, _ := profileSvc.CreateProfile(p1, SuperTenantOrg) + prof1, err := profileSvc.CreateProfile(p1, SuperTenantOrg) + require.NoError(t, err, "Failed to create prof1") time.Sleep(500 * time.Millisecond) - prof2, _ := profileSvc.CreateProfile(p2, SuperTenantOrg) + prof2, err := profileSvc.CreateProfile(p2, SuperTenantOrg) + require.NoError(t, err, "Failed to create prof2")This pattern should be applied consistently across all test scenarios.
146-146: Consider polling-based waits for more reliable tests.Fixed
time.Sleepdurations (e.g., 3 seconds) can cause flakiness under varying CI load. A polling approach that checks for the expected state with a timeout would be more robust. This is optional given the documented approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
go.mod(1 hunks)test/Unification_Scenarios.MD(1 hunks)test/integration/complex_unification_test.go(1 hunks)
🧰 Additional context used
🪛 LanguageTool
test/Unification_Scenarios.MD
[style] ~61-~61: ‘merge together’ might be wordy. Consider a shorter alternative.
Context: ...rofiles with different application data merge together. | App data from both profiles gets com...
(EN_WORDINESS_PREMIUM_MERGE_TOGETHER)
[style] ~62-~62: ‘merge together’ might be wordy. Consider a shorter alternative.
Context: ...ng email. | After update, both profiles merge together. | | 14 | Multiple Attributes Match...
(EN_WORDINESS_PREMIUM_MERGE_TOGETHER)
🔇 Additional comments (2)
test/integration/complex_unification_test.go (1)
564-572: Cleanup logic is well-structured.Using
t.Cleanupensures resources are released even if tests fail. The cleanup order (rules → profiles → schema) is appropriate.go.mod (1)
10-10: Thegithub.com/pkg/errorspackage is directly imported ininternal/system/utils/error_utils.goand properly marked as a direct dependency in go.mod. The promotion from indirect to direct is justified and correct.Likely an incorrect or invalid review comment.
| _ = unificationSvc.AddUnificationRule(userIdRule, SuperTenantOrg) | ||
|
|
||
| emailRuleId := uuid.New().String() | ||
| emailRule := model.UnificationRule{ | ||
| RuleId: emailRuleId, | ||
| TenantId: SuperTenantOrg, | ||
| RuleName: EmailBased, | ||
| PropertyName: "identity_attributes.email", | ||
| Priority: 1, | ||
| IsActive: true, | ||
| CreatedAt: time.Now().Unix(), | ||
| UpdatedAt: time.Now().Unix(), | ||
| } | ||
| _ = unificationSvc.AddUnificationRule(emailRule, SuperTenantOrg) | ||
|
|
||
| phoneRuleId := uuid.New().String() | ||
| phoneRule := model.UnificationRule{ | ||
| RuleId: phoneRuleId, | ||
| TenantId: SuperTenantOrg, | ||
| RuleName: PhoneBased, | ||
| PropertyName: "identity_attributes.phone_number", | ||
| Priority: 2, | ||
| IsActive: true, | ||
| CreatedAt: time.Now().Unix(), | ||
| UpdatedAt: time.Now().Unix(), | ||
| } | ||
| _ = unificationSvc.AddUnificationRule(phoneRule, SuperTenantOrg) | ||
|
|
There was a problem hiding this comment.
Handle errors from unification rule setup.
Ignoring errors from AddUnificationRule can mask test setup failures, causing all subsequent tests to fail mysteriously. If rule creation fails, the tests will produce false negatives.
- _ = unificationSvc.AddUnificationRule(userIdRule, SuperTenantOrg)
+ err = unificationSvc.AddUnificationRule(userIdRule, SuperTenantOrg)
+ require.NoError(t, err, "Failed to add user_id rule")
emailRuleId := uuid.New().String()
emailRule := model.UnificationRule{
RuleId: emailRuleId,
TenantId: SuperTenantOrg,
RuleName: EmailBased,
PropertyName: "identity_attributes.email",
Priority: 1,
IsActive: true,
CreatedAt: time.Now().Unix(),
UpdatedAt: time.Now().Unix(),
}
- _ = unificationSvc.AddUnificationRule(emailRule, SuperTenantOrg)
+ err = unificationSvc.AddUnificationRule(emailRule, SuperTenantOrg)
+ require.NoError(t, err, "Failed to add email rule")
phoneRuleId := uuid.New().String()
phoneRule := model.UnificationRule{
RuleId: phoneRuleId,
TenantId: SuperTenantOrg,
RuleName: PhoneBased,
PropertyName: "identity_attributes.phone_number",
Priority: 2,
IsActive: true,
CreatedAt: time.Now().Unix(),
UpdatedAt: time.Now().Unix(),
}
- _ = unificationSvc.AddUnificationRule(phoneRule, SuperTenantOrg)
+ err = unificationSvc.AddUnificationRule(phoneRule, SuperTenantOrg)
+ require.NoError(t, err, "Failed to add phone rule")🤖 Prompt for AI Agents
In test/integration/complex_unification_test.go around lines 100 to 127, the
calls to unificationSvc.AddUnificationRule ignore returned errors during test
setup; change each call to capture the error and fail the test immediately if
non-nil (e.g., check the error and call t.Fatalf or use require.NoError with the
testing.T) so rule creation failures cause the test to stop with a clear message
instead of producing downstream false negatives.
| master1, _ := profileSvc.GetProfile(master1Id) | ||
| require.NotNil(t, master1, "Master1 should exist") | ||
| require.GreaterOrEqual(t, len(master1.MergedFrom), 4, "Master1 should have at least 3 children") |
There was a problem hiding this comment.
Comment and assertion are inconsistent.
The comment says "at least 3 children" but the assertion checks >= 4. Clarify the expected count and align the comment with the assertion.
master1, _ := profileSvc.GetProfile(master1Id)
require.NotNil(t, master1, "Master1 should exist")
- require.GreaterOrEqual(t, len(master1.MergedFrom), 4, "Master1 should have at least 3 children")
+ require.GreaterOrEqual(t, len(master1.MergedFrom), 3, "Master1 should have at least 3 children")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| master1, _ := profileSvc.GetProfile(master1Id) | |
| require.NotNil(t, master1, "Master1 should exist") | |
| require.GreaterOrEqual(t, len(master1.MergedFrom), 4, "Master1 should have at least 3 children") | |
| master1, _ := profileSvc.GetProfile(master1Id) | |
| require.NotNil(t, master1, "Master1 should exist") | |
| require.GreaterOrEqual(t, len(master1.MergedFrom), 3, "Master1 should have at least 3 children") |
🤖 Prompt for AI Agents
In test/integration/complex_unification_test.go around lines 494 to 496, the
test comment and assertion disagree: the comment says "at least 3 children"
while the code asserts len(master1.MergedFrom) >= 4; make them consistent by
deciding the intended minimum (either 3 or 4) and updating either the numeric
comparison or the comment to match—if the expected minimum is 3 change the
assertion to require.GreaterOrEqual(t, len(master1.MergedFrom), 3, "...at least
3 children"), otherwise update the comment text to "at least 4 children".
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.