From 151f295d8221a1b3ee6f333560ed6e460947e51e Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Thu, 4 Aug 2022 22:07:19 +0530 Subject: [PATCH] MM-45021: Fix flaky testGetFlaggedPostsForUser (#20769) The method a.postRemoveFromChannelMessage was being called from a goroutine. Therefore, when SystemAdminClient.GetFlaggedPostsForUser was being called later in the test with a mock post store, it would naturally fail because the store would now be a different store but the goroutine was supposed to be finished. A hacky solution would be to add a sleep before starting the mocked API call. But a deeper question is why was the method run in a goroutine in the first place. Removing a user from a channel is not a very common operation and even if we look at the method, if the user is trying to remove themselves, that message happens synchronously, but if they are removing another user, that runs in a goroutine. This seems like a very weird behavior. Therefore, to be consistent I have just removed the goroutine and made everything synchronous. The next step would be to stop logging an error and just return the error upwards instead. Because that's what happens in the other condition. But that would be exceeding the scope too much. Maybe in a separate PR. https://mattermost.atlassian.net/browse/MM-45021 ```release-note NONE ``` --- app/channel.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/app/channel.go b/app/channel.go index 6287cbc165..860b44f125 100644 --- a/app/channel.go +++ b/app/channel.go @@ -2506,11 +2506,9 @@ func (a *App) RemoveUserFromChannel(c request.CTX, userIDToRemove string, remove return err } } else { - a.Srv().Go(func() { - if err := a.postRemoveFromChannelMessage(c, removerUserId, user, channel); err != nil { - mlog.Error("Failed to post user removal message", mlog.Err(err)) - } - }) + if err := a.postRemoveFromChannelMessage(c, removerUserId, user, channel); err != nil { + c.Logger().Error("Failed to post user removal message", mlog.Err(err)) + } } return nil