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

Unit test for mobile draft #8363

Open
wants to merge 113 commits into
base: main
Choose a base branch
from
Open

Unit test for mobile draft #8363

wants to merge 113 commits into from

Conversation

Rajat-Dabade
Copy link
Contributor

Summary

Added unit test for mobile in draft PR: #8280

Ticket Link

https://mattermost.atlassian.net/browse/MM-61735

Checklist

  • Added or updated unit tests (required for all new features)
  • Has UI changes
  • Includes text changes and localization file updates
  • Have tested against the 5 core themes to ensure consistency between them.
  • Have run E2E tests by adding label E2E iOS tests for PR.

Release Note

NONE

@Rajat-Dabade Rajat-Dabade marked this pull request as ready for review November 26, 2024 13:45
@Rajat-Dabade Rajat-Dabade self-assigned this Nov 26, 2024
@Rajat-Dabade Rajat-Dabade added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Nov 26, 2024
Base automatically changed from mobile-drafts to main January 13, 2025 13:10
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

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

Great work!
Nothing blocking, but a couple of things that can be improved.

Comment on lines +18 to +20
jest.mock('@components/formatted_text', () => jest.fn(() => null));
jest.mock('@components/formatted_time', () => jest.fn(() => null));
jest.mock('@components/compass_icon', () => jest.fn(() => null));
Copy link
Contributor

Choose a reason for hiding this comment

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

Any strong reason to mock these three components?

import type {Database} from '@nozbe/watermelondb';
import type DraftModel from '@typings/database/models/servers/draft';

describe('Draft Post', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it makes sense to add more tests here apart of the snapshot one.

Comment on lines +31 to +32
layoutWidth: 100,
location: 'draft',
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any tests (here or in other files) testing for what these props control. 0/5 if it makes sense to add such tests.

Comment on lines +39 to +40
(useTheme as jest.Mock).mockReturnValue(mockTheme);
(getUserTimezone as jest.Mock).mockReturnValue('UTC');
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use jest.mocked to avoid the casting, and keep the typings.

Suggested change
(useTheme as jest.Mock).mockReturnValue(mockTheme);
(getUserTimezone as jest.Mock).mockReturnValue('UTC');
jest.mocked(useTheme).mockReturnValue(mockTheme);
jest.mocked(getUserTimezone).mockReturnValue('UTC');

If you are going to use these too often, you may want to just create something like...

const mockedUseTheme = jest.mocked(useTheme);

describe(..., () => {
   ...
   beforeEach(() => {
        mockedUseTheme.mockReturnValue(mockTheme);
        ...

Comment on lines +44 to +52
const baseProps = {
channel: {type: General.DM_CHANNEL, displayName: 'Direct Message Channel'} as ChannelModel,
draftReceiverUser: undefined,
updateAt: 1633024800000,
rootId: undefined,
testID: 'channel-info',
currentUser: {timezone: 'UTC'} as unknown as UserModel,
isMilitaryTime: true,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

You could have the base props predefined (be it in a function or an object) and just set the "relevant props for the test". For example...

function getBaseProps() {
    return {
            channel: {type: General.DM_CHANNEL, displayName: 'Direct Message Channel'} as ChannelModel,
            draftReceiverUser: undefined,
            updateAt: 1633024800000,
            rootId: undefined,
            testID: 'channel-info',
            currentUser: {timezone: 'UTC'} as unknown as UserModel,
            isMilitaryTime: true,
        };
}

...
   it(..., () => {
        const props = getBaseProps();
        props.channel = {type: General.DM_CHANNEL, displayName: 'Direct Message Channel'} as ChannelModel,
        const wrapper = render(<DraftPostHeader {...props}/>);
        ....

Or if you prefer the object route...

const baseProps = {
    ...
};

...
    it(..., () => {
        const props = {
            ...baseProps,
            channel: {type: General.DM_CHANNEL, displayName: 'Direct Message Channel'} as ChannelModel,
        }
        const wrapper = render(<DraftPostHeader {...props}/>);
        ...

expect(wrapper.toJSON()).toMatchSnapshot();
});

it('calls draftDeleteHandler when pressed', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we are only checking dismissBottomSheet, but nothing else. Shouldn't we check that deleteDraftConfirmation is called with the correct arguments?

const server = await TestHelper.setupServerDatabase();
database = server.database;
});
it('should render the draft options component', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It is hard to see the difference between the two tests.

expect(wrapper.toJSON()).toMatchSnapshot();
});

it('Should call editHandler when pressed', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about this only checking for dismissBottomSheet.

expect(wrapper.toJSON()).toMatchSnapshot();
});

it('should call dismissBottmSheet after sending the draft', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment about this only checking for dismissBottomSheet.

expect(wrapper.toJSON()).toMatchSnapshot();
});

it('should call dismissBottmSheet after sending the draft', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('should call dismissBottmSheet after sending the draft', () => {
it('should call dismissBottomSheet after sending the draft', () => {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants