Conversation
📝 WalkthroughWalkthroughAdds an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #93 +/- ##
==========================================
+ Coverage 47.75% 47.84% +0.09%
==========================================
Files 276 276
Lines 16194 16195 +1
==========================================
+ Hits 7733 7749 +16
+ Misses 7657 7642 -15
Partials 804 804 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Lodek
left a comment
There was a problem hiding this comment.
Hope you came out the other side stronger after coding some Go 🫡
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x/bulletin/keeper/msg_server_test.go (1)
208-212: Test name was fixed but expected error message still has typo.The test case name on line 208 was corrected to "invalid creator address", but the
expErrMsgon line 212 still contains"nvalid creator address"(missing the leading 'i'). This inconsistency suggests the fix was incomplete.Proposed fix
{ name: "create post (error: invalid creator address)", input: &types.MsgCreatePost{}, setup: func() {}, expErr: true, - expErrMsg: "nvalid creator address", + expErrMsg: "invalid creator address", },
🧹 Nitpick comments (1)
x/bulletin/keeper/msg_server_test.go (1)
391-400: Consider more robust event attribute assertions.Accessing event attributes by hardcoded index (
ev.Attributes[0],[1],[2]) is fragile. If the event attribute order changes in the proto definition or emission logic, this test will fail with confusing messages or silently pass incorrect assertions.Consider also verifying:
- The event type (
ev.Type)- Attribute keys alongside values for self-documenting assertions
Suggested improvement
ev := evs[0] + require.Equal(t, "sourcehub.bulletin.EventPostCreated", ev.Type) - require.Equal(t, `"session-id"`, ev.Attributes[0].Value) - require.Equal(t, eventDid, ev.Attributes[1].Value) - require.Equal(t, `"bulletin/ns1"`, ev.Attributes[2].Value) + // Assert attribute keys and values for robustness + require.Equal(t, "artifact", ev.Attributes[0].Key) + require.Equal(t, `"session-id"`, ev.Attributes[0].Value) + require.Equal(t, "creator", ev.Attributes[1].Key) + require.Equal(t, eventDid, ev.Attributes[1].Value) + require.Equal(t, "namespace", ev.Attributes[2].Key) + require.Equal(t, `"bulletin/ns1"`, ev.Attributes[2].Value)Alternatively, create a helper that finds attributes by key:
func findEventAttr(ev sdk.Event, key string) string { for _, attr := range ev.Attributes { if attr.Key == key { return attr.Value } } return "" }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/bulletin/keeper/msg_server_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
x/bulletin/keeper/msg_server_test.go (3)
testutil/sample/sample.go (1)
AccAddress(10-14)x/bulletin/keeper/acp_utils.go (1)
RegisterNamespace(83-93)x/bulletin/types/tx.pb.go (6)
MsgRegisterNamespace(239-242)MsgRegisterNamespace(246-246)MsgRegisterNamespace(247-249)MsgCreatePost(127-133)MsgCreatePost(137-137)MsgCreatePost(138-140)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (1)
x/bulletin/keeper/msg_server_test.go (1)
361-401: Good test coverage for artifact in event emission.The test properly:
- Resets the event manager to isolate the test
- Sets up the required policy and namespace
- Creates a post with the new
Artifactfield- Verifies the artifact appears in the emitted event
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Adds an artifact for event tracking to Create Post in bulletin see https://github.com/sourcenetwork/orbis-rs/issues/22 for more details on why this was needed