Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CLNP-5043] Add unit tests for improved MessageSearch module #1228

Open
wants to merge 1 commit into
base: feat/state-mgmt-migration-1
Choose a base branch
from

Conversation

AhyoungRyu
Copy link
Contributor

@AhyoungRyu AhyoungRyu commented Oct 8, 2024

https://sendbird.atlassian.net/browse/CLNP-5043
Two things are handled based on what I mentioned in https://sendbird.atlassian.net/wiki/spaces/UIKitreact/pages/2511765635/UIKit+React+new+State+Management+Method+Proposal#4.1-Unit-Test

  • Added unit tests for useMessageSearch hook and new MessageSearchProvider
  • Added integration tests for MessageSearchUI component

So the MessageSearch module test coverage has been changed
from
File --------------------------------------------------| % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
Screenshot 2024-10-08 at 2 36 55 PM
to
after
note that it used to be like 0%, but now the test coverage of the newly added files is almost 100%; green 🟩.

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If unsure, ask the members.
This is a reminder of what we look for before merging your code.

  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • Public components / utils / props are appropriately exported
  • I have added necessary documentation (if appropriate)

result.current.actions.setQueryInvalid();
});

expect(result.current.state.isInvalid).toBe(true);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I was reading the code and the test, it was a bit confusing for me what isInvalid means because both a channel and a query could go invalid. Do you mind changing the name to isQueryInvalid for clarity?

result.current.actions.setCurrentChannel({ url: 'test-channel' } as any);
await waitFor(() => {
expect(result.current.state.currentChannel).toEqual({ url: 'test-channel' });
expect(result.current.state.initialized).toBe(true);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes more sense to me to make sure the other states such as loading and isInvalid unchanged. It would help finding some regression before the thing could go into trouble imho.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants