MM-59652 Add unit tests to actions/websocket/channel #8417
Conversation
| @@ -328,7 +328,7 @@ export async function handleUserAddedToChannelEvent(serverUrl: string, msg: any) | |||
|
|
|||
| loadCallForChannel(serverUrl, channelId); | |||
| } else { | |||
| const addedUser = getUserById(database, userId); | |||
There was a problem hiding this comment.
@larkox I noticed this was checking against the existence of the returned promise instead of the user. Could this potentially be the cause of some issues we've seen with user added to channel events?
There was a problem hiding this comment.
Great catch indeed. About what it might solve, I don't think it is any "terrible" issue. It is just the logic to get a missing user that has been added to the channel. And that is probably "autofixed" by the at mention in the "added to channel" post.
There was a problem hiding this comment.
The tests are technically correct, but I am not a big fan of them. I added a few comments, but I haven't reviewed all the tests (I trust they will be technically correct in the same way).
Let me know if you want to keep the tests like this and I will finish my review taking that into consideration, or if you want to improve the tests.
| (getChannelById as jest.Mock).mockResolvedValue(null); | ||
| (fetchMyChannel as jest.Mock).mockResolvedValue({channels: [{}], memberships: [{}]}); | ||
| (prepareMyChannelsForTeam as jest.Mock).mockResolvedValue([[]]); | ||
| (addChannelToDefaultCategory as jest.Mock).mockResolvedValue({models: []}); |
There was a problem hiding this comment.
I am not a great fan of such heavy mocking for two main reasons:
- The data is not real
- Changes beyond this file may not make the test break (integration testing vs pure unit testing)
It is a more philosophical discussion, so I am 0/5 on whether this require any change.
There was a problem hiding this comment.
The data is not real
The data isn't real in any test. Even if it's more of an integration test somewhere there needs to be mock/fake data. We could make the data more realistic for these tests but it's not required by the implementation of the function so I don't see the point.
Changes beyond this file may not make the test break (integration testing vs pure unit testing)
These should be unit tests imo and only focused on testing the functionality in the related functions for the most part. The unit tests of other files should cover their functionality. Then we have E2E tests to cover integration. I feel that having every test be an integration test would be overkill and make the tests too fragile but I'm open to having my mind changed.
Thoughts @enahum ?
There was a problem hiding this comment.
Continued the conversation here https://hub.mattermost.com/private-core/pl/dqauz7iohbdquratstmojgtyer
|
|
||
| await handleChannelCreatedEvent(serverUrl, msg); | ||
|
|
||
| expect(getChannelById).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
We are checking here for a implementation detail. We should be checking the effect of the function instead (whether the channel gets added to the database, for example).
|
@larkox I seemed to have kicked a hornet's nest in regards to pure vs integration style unit tests. While that's getting figured out I'd rather not hold up this PR as it helps us move towards our coverage goals. I made some updates based on your feedback - can you take another look? Unless there's major blockers I'd like to get this merged and then we can revisit after the discussion completes |
jwilander
added
4: Reviews Complete
Summary
Add unit tests to actions/websocket/channel increasing coverage to 94.33%.
Ticket Link
https://mattermost.atlassian.net/browse/MM-59652
Checklist
E2E iOS tests for PR.Device Information
This PR was tested on: n/a
Screenshots
n/a
Release Note