Dramatically reduce usage of mocked plugin tests #707
Conversation
| @@ -147,7 +147,7 @@ func (ah *ActivityHandler) handleAttachments(channelID, userID, text string, msg | |||
|
|
|||
| // handle a message reference (reply) | |||
| if a.ContentType == "messageReference" { | |||
| parentID, newText = ah.handleMessageReference(a, msg.ChatID+msg.ChannelID, newText) | |||
There was a problem hiding this comment.
newText is never used, and thus never modified.
| func (p *Plugin) handlePromptForConnection(userID, channelID string) { | ||
| // For now, don't display the connect message | ||
|
|
||
| // message := "Some users in this conversation rely on Microsoft Teams to receive your messages, but your account isn't connected. " | ||
| // p.SendEphemeralConnectMessage(channelID, userID, message) | ||
| } | ||
|
|
There was a problem hiding this comment.
As we're not showing the message at all, let's simplify this.
| @@ -300,7 +300,7 @@ func (ah *ActivityHandler) handleCreatedActivity(msg *clientmodels.Message, subs | |||
|
|
|||
| var chatShouldSync bool | |||
| var discardReason string | |||
| chatShouldSync, _, _, discardReason, err = ah.plugin.ChatShouldSync(channel) | |||
There was a problem hiding this comment.
We no longer need the containsRemoteUser return value anywhere.
| if !ah.plugin.GetSyncChats() { | ||
| return metrics.DiscardedReasonChatsDisabled | ||
| } | ||
| } else { | ||
| if !ah.plugin.GetSyncLinkedChannels() { | ||
| return metrics.DiscardedReasonLinkedChannelsDisabled | ||
| } |
There was a problem hiding this comment.
We now also discard if sync is enabled for chats or channels. Previously, this code would potentially try to still delete messages despite this functionality being disabled.
| @@ -585,8 +595,10 @@ func (ah *ActivityHandler) handleDeletedActivity(activityIds clientmodels.Activi | |||
| return metrics.DiscardedReasonOther | |||
| } | |||
|
|
|||
| ah.IgnorePluginHooksMap.Store(fmt.Sprintf("delete_post_%s", postInfo.MattermostID), true) | |||
There was a problem hiding this comment.
Switching to the new unit tests exposed a race condition that the plugin elsewhere solves using the IgnorePluginHooksMap -- the idea is that when we call DeletePost below, the server will reflect that event back to us as a message hook, and we'll try to delete the Teams post (which will in turn fail because it's already deleted). I'm uncertain still if this is the right long term solution, but adding it here to mimic what we already do for creating posts and reactions.
| @@ -89,7 +94,7 @@ func (p *Plugin) MessageHasBeenPosted(_ *plugin.Context, post *model.Post) { | |||
| } | |||
|
|
|||
| if isDirectOrGroupMessage { | |||
| chatShouldSync, containsRemoteUser, members, _, err := p.ChatShouldSync(channel) | |||
There was a problem hiding this comment.
The return value of containsRemoteUser is no longer needed.
| @@ -102,7 +107,7 @@ func (p *Plugin) MessageHasBeenPosted(_ *plugin.Context, post *model.Post) { | |||
| dstUsers = append(dstUsers, m.UserId) | |||
| } | |||
|
|
|||
| _, err = p.SendChat(post.UserId, dstUsers, post, containsRemoteUser) | |||
There was a problem hiding this comment.
The parameter containsRemoteUser is no longer used in p.SendChat.
| @@ -234,7 +239,7 @@ func (p *Plugin) MessageHasBeenUpdated(c *plugin.Context, newPost, _ /*oldPost*/ | |||
|
|
|||
| var chatShouldSync bool | |||
| var members []*model.ChannelMember | |||
| chatShouldSync, _, members, _, err = p.ChatShouldSync(channel) | |||
There was a problem hiding this comment.
The return value of containsRemoteUser is no longer needed.
| func (p *Plugin) GetURL() string { | ||
| return getURL(p.API.GetConfig()) | ||
| } |
There was a problem hiding this comment.
Testing these without a mock proved tricky because of how the server intervenes and changes the SiteURL. So I opted to refactor and just test getURL and getRelativeURL directly from passed *model.Config parameters.
| @@ -17,28 +17,28 @@ import ( | |||
| // | |||
| // TODO: This method does too much, but it's reflective of the underlying complexity of the | |||
| // business logic. Thankfully, it's well tested! | |||
| func (p *Plugin) ChatShouldSync(channel *model.Channel) (bool, bool, []*model.ChannelMember, string, error) { | |||
There was a problem hiding this comment.
The return value of containsRemoteUser is no longer needed.
lieut-data
requested review from
JulienTant and
sbishel
and removed request for
JulienTant
|
@enahum, I know this is outside your usual wheel house, but wondering if you can help give a second review? |
| // Simulate the post having originated from Mattermost. Later, we'll let the code | ||
| // do this itself once. | ||
| err = th.p.GetStore().LinkPosts(storemodels.PostInfo{ | ||
| MattermostID: post.Id, | ||
| MSTeamsID: messageID, | ||
| MSTeamsChannel: chatOrChannelID, | ||
| MSTeamsLastUpdateAt: time.Now(), | ||
| }) |
There was a problem hiding this comment.
Just out of curiosity, what is the reason to have to force it here?
Summary
This is a suite of changes to dramatically reduce the use of mocked plugin tests in favour of the new reattached plugin testing methodology. This is mostly just unit tests, but a few changes were completed to the core product to simplify along the way.
This is a precursor to another round of simplification to help us converge on v10 deliverables.
Ticket Link
Fixes: https://mattermost.atlassian.net/browse/MM-55512