Conversation
* Implemented MentionEach command to mention all users with a specific role. * Updated command names and constants to include MentionEach. * Added utility functions for handling user and role mentions.
…d refactor getUserWithRole to use models.SessionInterface
Summary by CodeRabbit
WalkthroughThis update introduces the "MentionEach" command to the Discord bot, enabling users to mention all members of a specific role with flexible options. The implementation includes new command registration, request validation, and asynchronous processing via a message queue. The handler retrieves role members, formats mentions, and supports three modes: standard (all mentions in one message), dev (batch individual mentions), and dev-title (list users without pinging). The codebase is expanded with utility functions for member retrieval and formatting, comprehensive unit tests, and typo corrections in method names. All changes are modular, with dependency injection and robust error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DiscordAPI
participant Service
participant Queue
participant Handler
participant Utils
User->>DiscordAPI: Issues /mention-each command
DiscordAPI->>Service: Sends interaction request
Service->>Service: Validate input & feature flag
Service->>Queue: Enqueue DataPacket with metadata
Service->>DiscordAPI: Responds "Processing..."
Queue->>Handler: Delivers DataPacket
Handler->>Utils: Fetch members with role
Utils-->>Handler: Returns member list
Handler->>Utils: Format mentions/message
Handler->>DiscordAPI: Sends mention(s) to channel
Assessment against linked issues
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (1)
commands/main/resgister_test.go (1)
98-104:⚠️ Potential issueRemove duplicated test case.
This test case is identical to the one on lines 91-97, which also tests that RegisterCommands panics when ApplicationCommandCreate returns an error.
- t.Run("should panic when openSession.ApplicationCommandCreate() returns an error", func(t *testing.T) { - mockSess := &mockSession{openError: nil, commandError: assert.AnError} - - assert.Panics(t, func() { - RegisterCommands(mockSess) - }, "RegisterCommands should panic when ApplicationCommandCreate returns an error") - })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
commands/handlers/main.go(1 hunks)commands/handlers/main_test.go(1 hunks)commands/handlers/mentionEachHandler.go(1 hunks)commands/handlers/mentionEachHandler_test.go(1 hunks)commands/main.go(1 hunks)commands/main/register.go(1 hunks)commands/main/resgister_test.go(2 hunks)commands/register/register.go(1 hunks)commands/register/register_test.go(2 hunks)config/config.go(1 hunks)dtos/commands.go(1 hunks)go.mod(1 hunks)models/discord.go(1 hunks)models/discord_test.go(2 hunks)service/main.go(1 hunks)service/mentionEachService.go(1 hunks)tests/mocks/discordSessionMock.go(1 hunks)utils/constants.go(1 hunks)utils/membersUtils.go(1 hunks)utils/membersUtils_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
commands/main/resgister_test.go (1)
Learnt from: Devashish08
PR: Real-Dev-Squad/discord-service#80
File: utils/members_utils_test.go:28-37
Timestamp: 2025-04-11T16:51:24.677Z
Learning: The `ChannelMessageSend` mock implementation in `utils/members_utils_test.go` is necessary because it's part of the `DiscordSessionInterface` interface in the Discord service. In Go, all methods of an interface must be implemented for a mock to properly satisfy the interface, even if some methods aren't directly used in the current tests.
commands/register/register_test.go (1)
Learnt from: Devashish08
PR: Real-Dev-Squad/discord-service#80
File: utils/members_utils_test.go:28-37
Timestamp: 2025-04-11T16:51:24.677Z
Learning: The `ChannelMessageSend` mock implementation in `utils/members_utils_test.go` is necessary because it's part of the `DiscordSessionInterface` interface in the Discord service. In Go, all methods of an interface must be implemented for a mock to properly satisfy the interface, even if some methods aren't directly used in the current tests.
tests/mocks/discordSessionMock.go (1)
Learnt from: Devashish08
PR: Real-Dev-Squad/discord-service#80
File: utils/members_utils_test.go:28-37
Timestamp: 2025-04-11T16:51:24.677Z
Learning: The `ChannelMessageSend` mock implementation in `utils/members_utils_test.go` is necessary because it's part of the `DiscordSessionInterface` interface in the Discord service. In Go, all methods of an interface must be implemented for a mock to properly satisfy the interface, even if some methods aren't directly used in the current tests.
🧬 Code Graph Analysis (9)
commands/handlers/main_test.go (3)
dtos/queue.go (1)
DataPacket(9-13)utils/constants.go (1)
CommandNames(11-16)commands/handlers/main.go (1)
MainHandler(20-37)
service/main.go (2)
utils/constants.go (1)
CommandNames(11-16)commands/handlers/main.go (1)
CS(18-18)
commands/handlers/main.go (2)
utils/constants.go (1)
CommandNames(11-16)service/main.go (1)
CS(14-14)
commands/main.go (1)
utils/constants.go (1)
CommandNames(11-16)
utils/membersUtils_test.go (3)
tests/mocks/discordSessionMock.go (1)
DiscordSession(12-14)utils/constants.go (1)
DISCORD_GUILD_MEMBER_API_LIMIT(8-8)utils/membersUtils.go (4)
GetUsersWithRole(12-55)FormatUserMentions(57-67)FormatMentionResponse(69-77)FormatUserListResponse(79-92)
utils/constants.go (1)
dtos/commands.go (1)
CommandNameTypes(3-8)
utils/membersUtils.go (2)
models/discord.go (1)
SessionInterface(33-40)utils/constants.go (1)
DISCORD_GUILD_MEMBER_API_LIMIT(8-8)
commands/handlers/mentionEachHandler.go (3)
models/discord.go (2)
SessionInterface(33-40)SessionWrapper(5-7)utils/membersUtils.go (4)
GetUsersWithRole(12-55)FormatUserListResponse(79-92)FormatMentionResponse(69-77)FormatUserMentions(57-67)commands/handlers/main.go (2)
CommandHandler(14-16)CreateSession(44-57)
service/mentionEachService.go (5)
service/main.go (1)
CommandService(10-12)dtos/discord.go (1)
Data(5-8)dtos/queue.go (1)
DataPacket(9-13)utils/constants.go (1)
CommandNames(11-16)queue/main.go (1)
SendMessage(90-114)
🔇 Additional comments (33)
config/config.go (1)
7-7: LGTM: Import reordering is a minor style changeThe reordering of the "os" import to the last position is a simple style adjustment and doesn't affect functionality.
dtos/commands.go (1)
7-7: LGTM: New command type added correctlyThe addition of the
MentionEachfield to theCommandNameTypesstruct properly supports the new "mention-each" command functionality.utils/constants.go (2)
5-9: LGTM: Well-structured constants blockGrouping related constants into a single block improves code organization. The new
DISCORD_GUILD_MEMBER_API_LIMITconstant with a value of 1000 appropriately handles Discord API pagination limits.
12-15: LGTM: Command name constant added properlyThe addition of the "mention-each" command name is correctly integrated into the
CommandNamesvariable, maintaining consistency with the structure used for other commands.go.mod (1)
19-19: LGTM: New indirect dependencyThe addition of
github.com/stretchr/objx v0.5.2as an indirect dependency is likely introduced by test mock frameworks and is appropriate for the testing changes in this PR.commands/register/register.go (1)
35-35: Method name correction implemented correctly.The change from
GetUerId()toGetUserId()fixes a typo in the method call name, ensuring consistency with the renamed method in the session interface.commands/main/register.go (1)
33-33: Method name correction implemented correctly.The change from
GetUerId()toGetUserId()aligns with the renamed method in the session interface, ensuring consistency across the codebase.service/main.go (1)
25-26: Command routing for "mention-each" added correctly.The new case statement for handling the "mention-each" command follows the existing code pattern and properly routes to the MentionEachService handler function.
commands/handlers/main_test.go (1)
25-36: Test case for "mention-each" command handler is well-structured.The test follows the established pattern and verifies that MainHandler correctly returns a non-nil handler when processing the "mention-each" command. The test includes appropriate assertions and error checking.
commands/handlers/main.go (1)
31-32: Integration of MentionEach command looks good.The new case statement appropriately integrates the
mentionEachHandlerinto the command dispatch flow, following the established pattern used for other commands.models/discord_test.go (4)
10-11: Good improvement: Local constant instead of import dependency.Using a local constant reduces unnecessary dependencies and improves the test file's maintainability.
41-42: Good fix: Method name typo correction.Correcting
GetUerId()toGetUserId()improves naming consistency throughout the codebase.
44-48: Appropriate test coverage for GuildMembers method.This test properly verifies that the method is implemented and panics when invoked on the dummy session wrapper, following the pattern established for other methods.
50-54: Appropriate test coverage for ChannelMessageSend method.This test correctly verifies that the method is implemented and panics when invoked on the dummy session wrapper, consistent with other interface method tests.
commands/register/register_test.go (3)
40-43: Good addition: Mock fields for new methods.These fields support the mock implementations of the Discord session interface methods required for the MentionEach command.
62-65: Good fix: Method name typo correction.Correcting
GetUerId()toGetUserId()maintains consistency with the fixes in other files.
67-74: Appropriate mock implementations for interface completeness.These implementations satisfy the Go interface requirement that all methods must be implemented, even if not directly used in the current tests.
commands/main/resgister_test.go (3)
40-43: Good addition: Mock fields for new methods.These fields properly support the mock implementations of the Discord session interface methods required for the MentionEach command.
62-65: Good fix: Method name typo correction.Correcting
GetUerId()toGetUserId()maintains consistency with the fixes in other files.
67-75: Appropriate mock implementations for interface completeness.These implementations satisfy the Go interface requirement that all methods must be implemented, even if not directly used in the current tests.
commands/main.go (1)
29-68: Well-structured command definitionThe
MentionEachcommand implementation is clean, well-documented with comments for each option, and follows the established pattern of other commands in the codebase. The options are logically organized with the required role parameter first, followed by optional parameters.models/discord.go (3)
21-23: Typo correction improves code clarityCorrecting the method name from
GetUerIdtoGetUserIdimproves readability and maintains consistent naming conventions.
25-31: Appropriate wrapper methods for Discord APIThe new wrapper methods for
GuildMembersandChannelMessageSendprovide a clean interface for the mention-each feature to interact with Discord. This follows the established pattern of wrapping Discord session methods.
37-39: Interface definition is consistent with implementationThe interface updates correctly reflect the method signatures added to the SessionWrapper struct, ensuring proper implementation compliance.
utils/membersUtils.go (3)
12-55: Robust implementation of member retrieval with paginationThe
GetUsersWithRolefunction effectively handles Discord API pagination to fetch all guild members with a specific role. Good practices observed:
- Proper error handling and wrapping
- Comprehensive logging at debug and info levels
- Defensive coding with nil checks for member.User
- Efficient pagination by tracking the last member ID
57-67: Clean implementation of mention formatting with error handlingThe
FormatUserMentionsfunction correctly converts member objects to Discord mention strings while handling nil User cases appropriately.
79-92: User-friendly response formatting with clear messagingThe
FormatUserListResponsefunction uses a clean switch statement to handle different user count scenarios, providing appropriate messaging for each case.utils/membersUtils_test.go (4)
14-145: Comprehensive tests for user role retrievalThe tests for
GetUsersWithRolecover all important scenarios:
- Single and multiple users with matching roles
- Error handling from the Discord API
- Empty result sets
- Handling invalid member data
- Pagination across multiple API calls
The test cases are well-structured with clear test names and thorough assertions.
147-178: Thorough tests for mention formattingThe tests for
FormatUserMentionsproperly test the formatting logic, including handling:
- Normal cases with valid users
- Empty member lists
- Nil member lists
- Members with nil User fields
All edge cases are well-covered.
180-196: Good coverage of message formattingThe tests for
FormatMentionResponsecorrectly verify both scenarios:
- Formatting with a message prefix
- Formatting with only mentions (empty message)
197-236: Complete tests for user list response formattingThe tests for
FormatUserListResponsethoroughly cover all cases:
- Zero users with the role
- One user with the role
- Multiple users with the role
- Nil mentions list
- Empty role ID
Each case has clear assertions to verify the correct output format.
service/mentionEachService.go (1)
83-91: Potentially long interaction response may exceed Discord’s 2 000-character limitAlthough the response here only contains the role mention, later edits or localisation might accidentally push it over the limit.
Consider guarding withlen(responseContent) <= 2000or splitting the message.commands/handlers/mentionEachHandler_test.go (1)
258-272: Test path may never hithandleStandardMode
fetchMembersWithRoleFuncreturns zero members (l. 262-264).
In production code the handler short-circuits tosendNoMembersMessageFuncwhen the slice is empty.
Yet this case stubshandleStandardModeFuncto returnmockErr(l. 265-267) and asserts that the error bubbles up.Unless the handler purposefully calls
handleStandardModewith an empty slice, the test is asserting an impossible path and will start failing once the real logic is corrected.Please verify the intended behaviour and either:
- return a non-empty slice here, or
- assert on
sendNoMembersMessageFuncinstead.
| func FormatMentionResponse(mentions []string, message string) string { | ||
| mentionStrings := strings.Join(mentions, " ") | ||
| if message == "" { | ||
| return mentionStrings | ||
| } | ||
|
|
||
| return fmt.Sprintf("%s %s", message, mentionStrings) | ||
|
|
||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Message formatting logic is clean and handles edge cases
The FormatMentionResponse function correctly handles both cases with and without a custom message.
Consider removing the unnecessary blank line on line 76 for cleaner code.
return fmt.Sprintf("%s %s", message, mentionStrings)
-
}📝 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.
| func FormatMentionResponse(mentions []string, message string) string { | |
| mentionStrings := strings.Join(mentions, " ") | |
| if message == "" { | |
| return mentionStrings | |
| } | |
| return fmt.Sprintf("%s %s", message, mentionStrings) | |
| } | |
| func FormatMentionResponse(mentions []string, message string) string { | |
| mentionStrings := strings.Join(mentions, " ") | |
| if message == "" { | |
| return mentionStrings | |
| } | |
| return fmt.Sprintf("%s %s", message, mentionStrings) | |
| } |
| func (m *DiscordSession) ApplicationCommandCreate(appID, guildID string, cmd *discordgo.ApplicationCommand) (*discordgo.ApplicationCommand, error) { | ||
| args := m.Called(appID, guildID, cmd) | ||
| var retCmd *discordgo.ApplicationCommand | ||
| if args.Get(0) != nil { | ||
| var ok bool | ||
| retCmd, ok = args.Get(0).(*discordgo.ApplicationCommand) | ||
| if !ok { | ||
| panic(fmt.Sprintf("mock return value 0 for ApplicationCommandCreate is not *discordgo.ApplicationCommand: %T", args.Get(0))) | ||
| } | ||
| } | ||
| return retCmd, args.Error(1) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid panic by failing the test instead
panicing inside a mock unexpectedly aborts the entire test run and makes it harder to locate the failure.
Consider returning a descriptive error so that the caller’s require/assert captures it cleanly.
| type DiscordSession struct { | ||
| mock.Mock | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add a compile-time assertion to guarantee the mock stays in sync with the interface
Right now we only hope that DiscordSession fulfils models.SessionInterface.
A build-time check will fail fast whenever the interface is extended and the mock is not updated – this prevents subtle test breakages later on.
type DiscordSession struct {
mock.Mock
}
+
+// Ensure the mock implements all methods from models.SessionInterface
+var _ models.SessionInterface = (*DiscordSession)(nil)You will have to import the models package at the top of the file.
| messageOption := findOption(s.discordMessage.Data.Options, "message") | ||
| if messageOption != nil && messageOption.Value != nil { | ||
| message, _ = messageOption.Value.(string) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Nil-safe string cast for the optional message field
messageOption.Value.(string) will silently yield an empty string and a false flag when the value is not a string.
If a non-string sneaks through, we silently ignore the problem. Prefer validating the type and logging a warning, similar to how ff_enabled is handled.
| roleID, ok := roleOption.Value.(string) | ||
| if !ok { | ||
| errorMsg := "Invalid role format(not a String)" | ||
| logrus.Errorf("%s: Expected string, got %T", errorMsg, roleOption.Value) | ||
| sendErrorResponse(response, errorMsg) | ||
| return | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Shadowed ok variable hides earlier declaration
The ok declared here shadows the one defined on line 27, which can be confusing during debugging.
- roleID, ok := roleOption.Value.(string)
+ roleID, ok := roleOption.Value.(string)If you need a fresh boolean, give it a more expressive name (e.g. isString). Otherwise, reuse the existing variable.
Committable suggestion skipped: line range outside the PR's diff.
| userMessage := metaData["message"] | ||
| if len(userMessage) > MaxUserMessageLength { | ||
| truncatedSuffix := "..." | ||
| logrus.Warnf("User provided message length (%d) exceeds limit (%d). Truncating.", len(userMessage), MaxUserMessageLength) | ||
| userMessage = userMessage[:MaxUserMessageLength] + truncatedSuffix | ||
| } | ||
| params.Message = userMessage |
There was a problem hiding this comment.
Truncation logic can overflow the max length
userMessage[:MaxUserMessageLength] + "..." returns 1003 characters, not 1000.
Trim the suffix before appending to stay within Discord’s limit.
- userMessage = userMessage[:MaxUserMessageLength] + truncatedSuffix
+ userMessage = userMessage[:MaxUserMessageLength-len(truncatedSuffix)] + truncatedSuffix📝 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.
| userMessage := metaData["message"] | |
| if len(userMessage) > MaxUserMessageLength { | |
| truncatedSuffix := "..." | |
| logrus.Warnf("User provided message length (%d) exceeds limit (%d). Truncating.", len(userMessage), MaxUserMessageLength) | |
| userMessage = userMessage[:MaxUserMessageLength] + truncatedSuffix | |
| } | |
| params.Message = userMessage | |
| userMessage := metaData["message"] | |
| if len(userMessage) > MaxUserMessageLength { | |
| truncatedSuffix := "..." | |
| logrus.Warnf("User provided message length (%d) exceeds limit (%d). Truncating.", len(userMessage), MaxUserMessageLength) | |
| userMessage = userMessage[:MaxUserMessageLength-len(truncatedSuffix)] + truncatedSuffix | |
| } | |
| params.Message = userMessage |
| func TestExtractCommandParamsLogic(t *testing.T) { | ||
|
|
||
| t.Run("Valid parameters all present", func(t *testing.T) { | ||
| metaData := map[string]string{ | ||
| "role_id": "role1", "channel_id": "chan1", "guild_id": "guild1", | ||
| "message": "Hello", "dev": "true", "dev_title": "false", | ||
| } | ||
| params, err := extractCommandParamsFunc(metaData) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, "role1", params.RoleID) | ||
| assert.Equal(t, "chan1", params.ChannelID) | ||
| assert.Equal(t, "guild1", params.GuildID) | ||
| assert.Equal(t, "Hello", params.Message) | ||
| assert.True(t, params.Dev) | ||
| assert.False(t, params.DevTitle) | ||
| }) | ||
|
|
||
| t.Run("Valid parameters optional missing", func(t *testing.T) { | ||
| metaData := map[string]string{ | ||
| "role_id": "role1", "channel_id": "chan1", "guild_id": "guild1", | ||
| } | ||
| params, err := extractCommandParamsFunc(metaData) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, "role1", params.RoleID) | ||
| assert.Equal(t, "", params.Message) | ||
| assert.False(t, params.Dev) | ||
| assert.False(t, params.DevTitle) | ||
| }) | ||
|
|
||
| t.Run("Valid parameters with long message (truncates)", func(t *testing.T) { | ||
| longMessage := strings.Repeat("a", MaxUserMessageLength+50) | ||
| truncatedSuffix := "..." | ||
| expectedMessage := strings.Repeat("a", MaxUserMessageLength) + truncatedSuffix | ||
| metaData := map[string]string{ | ||
| "role_id": "role1", "channel_id": "chan1", "guild_id": "guild1", "message": longMessage, | ||
| } | ||
| params, err := extractCommandParamsFunc(metaData) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, expectedMessage, params.Message, "Message should be truncated") | ||
| assert.Len(t, params.Message, MaxUserMessageLength+len(truncatedSuffix)) | ||
| }) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
High duplication – table-driven tests would improve maintainability
TestExtractCommandParamsLogic manually repeats the same arrange-act-assert pattern for many permutations. A table-driven style can shrink ~120 LOC to ~30-40, making it easier to add new cases:
tests := []struct{
name string
meta map[string]string
want CommandParams
wantErr bool
}{ ... }
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
got, err := extractCommandParamsFunc(tc.meta)
if tc.wantErr { assert.Error(t, err); return }
assert.Equal(t, tc.want, got)
})
}Helps readability and reduces the risk of inconsistent setups.
| t.Run("Success with batching", func(t *testing.T) { | ||
| mockSession := new(mocks.DiscordSession) | ||
| for i := 0; i < 6; i++ { | ||
| expectedMsg := fmt.Sprintf("%s %s", params.Message, mentions[i]) | ||
| mockSession.On("ChannelMessageSend", params.ChannelID, expectedMsg).Return(&discordgo.Message{}, nil).Once() | ||
| } | ||
|
|
||
| err := handleDevModeFunc(mockSession, mentions, params) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Potentially slow tests due to real time.Sleep in batching logic
handleDevModeFunc is invoked directly; if the production implementation sleeps between batches (e.g., time.Sleep(batchDelay)), these unit tests will incur that delay and slow down the test suite.
Consider injecting a sleepFn (defaulting to time.Sleep) into the handler and stub it with a no-op in tests:
-func handleDevMode(...) error {
- ...
- time.Sleep(batchDelay)
+func handleDevMode(..., sleep func(time.Duration)) error {
+ ...
+ sleep(batchDelay)
}Then pass func(_ time.Duration){} from tests.
This keeps production behaviour unchanged while making tests fast and deterministic.
Committable suggestion skipped: line range outside the PR's diff.
| t.Run("Success Path with session Close Error", func(t *testing.T) { | ||
| extractCommandParamsFunc = func(map[string]string) (CommandParams, error) { | ||
| return CommandParams{RoleID: "testRole", ChannelID: "testChannel", GuildID: "testGuild", Message: "Hello", Dev: false, DevTitle: false}, nil | ||
| } | ||
|
|
||
| mockSessionInstance := new(mocks.DiscordSession) | ||
| closeErr := errors.New("failed t0 close session") | ||
| mockSessionInstance.On("Close").Return(closeErr).Once() | ||
|
|
||
| CreateSession = func() (*discordgo.Session, error) { | ||
| return &discordgo.Session{}, nil | ||
| } | ||
|
|
||
| fetchMembersWithRoleFunc = func(session models.SessionInterface, guildID, roleID, channelID string) ([]discordgo.Member, error) { | ||
| return membersList, nil | ||
| } | ||
|
|
||
| handleStandardModeFunc = func(session models.SessionInterface, mentions []string, params CommandParams) error { | ||
| return nil | ||
| } | ||
|
|
||
| handleDevModeFunc = func(session models.SessionInterface, mentions []string, params CommandParams) error { | ||
| return nil | ||
| } | ||
| handleDevTitleModeFunc = func(models.SessionInterface, []string, CommandParams) error { | ||
| t.Fatal("handleDevTitleMode called unexpectedly") | ||
| return nil | ||
| } | ||
| sendNoMembersMessageFunc = func(models.SessionInterface, string) error { | ||
| t.Fatal("sendNoMembersMessage called unexpectedly") | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Mock for Close() is never exercised – session leak assertion ineffective
You instantiate mockSessionInstance (l. 312-315) but CreateSession (l. 316-318) returns &discordgo.Session{}, not the mock.
Therefore:
session.Close()inside the handler is not routed to your mock, so theOn("Close")expectation is never met.- The test passes even if
Close()is never called.
Fix by returning the mock:
-mockSessionInstance := new(mocks.DiscordSession)
-...
-CreateSession = func() (*discordgo.Session, error) {
- return &discordgo.Session{}, nil
-}
+mockSessionInstance := new(mocks.DiscordSession)
+CreateSession = func() (*discordgo.Session, error) {
+ return (*discordgo.Session)(mockSessionInstance) // ⚠️ adjust CreateSession signature to return the interface
+}(or better: change CreateSession to return models.SessionInterface, eliminating the cast).
Without this, the test gives a false sense of coverage regarding resource cleanup.
Committable suggestion skipped: line range outside the PR's diff.
| func TestMentionEachHandler(t *testing.T) { | ||
| originalCreateSession := CreateSession | ||
| originalExtractParams := extractCommandParamsFunc | ||
| originalFetchMembers := fetchMembersWithRoleFunc | ||
| originalSendNoMembers := sendNoMembersMessageFunc | ||
| originalHandleDevTitle := handleDevTitleModeFunc | ||
| originalHandleDev := handleDevModeFunc | ||
| originalHandleStandard := handleStandardModeFunc | ||
|
|
||
| defer func() { | ||
| CreateSession = originalCreateSession | ||
| extractCommandParamsFunc = originalExtractParams | ||
| fetchMembersWithRoleFunc = originalFetchMembers | ||
| sendNoMembersMessageFunc = originalSendNoMembers | ||
| handleDevTitleModeFunc = originalHandleDevTitle | ||
| handleDevModeFunc = originalHandleDev | ||
| handleStandardModeFunc = originalHandleStandard | ||
| }() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Global monkey-patching makes the whole package non-parallel & brittle
All sub-tests change exported function variables (CreateSession, fetchMembersWithRoleFunc, …) that are shared across the package.
Because they are mutated in-place, these tests:
- Cannot safely run with
t.Parallel()(inside this or other test files). - May leak the last patched implementation to other packages’ tests executed after this one.
Prefer a safer pattern:
-func TestMentionEachHandler(t *testing.T) {
- originalCreateSession := CreateSession
- ...
- defer func(){ ...restore... }()
+func TestMentionEachHandler(t *testing.T) {
+ t.Cleanup(func() {
+ // restore every patched fn so other tests see the real impl
+ CreateSession = originalCreateSession
+ extractCommandParamsFunc = originalExtractParams
+ ...
+ })…and wrap each t.Run in its own t.Cleanup or use local wrappers that return closures, so patches are isolated per case.
This keeps the tests deterministic and unlocks t.Parallel() when desired.
Committable suggestion skipped: line range outside the PR's diff.
1 participant
Date: 29/04/2025
Developer Name: @Devashish08
Issue Ticket Number
closes #70
Description
Notes for Reviewers:
commands/handlers/mentionEachHandler_test.go,commands/handlers/main_test.go).This PR introduces unit tests for the
/mention-eachcommand's background handler layer (commands/handlers/), covering the logic implemented in PR #95Related PRs:
Builds Upon: Branch
mention-each-handlerDocumentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Test Coverage
Additional Notes