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.

Sign up for GitHub

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

Jump to bottom

Add @testing-library/react and component tests #465

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Add @testing-library/react and component tests #465

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3a28f7a
migrate team sidebar component to typescript
Raghunandhan8818 Mar 3, 2024
7b3775b
added tests to the team_sidebar components
Raghunandhan8818 Mar 9, 2024
9d0099e
fixed lint issues
Raghunandhan8818 Mar 9, 2024
d7dacd5
moved rtl to devdependencies, added snapshot tests to team_sidebar
Raghunandhan8818 Mar 12, 2024
8c95b3b
removed mocking in team_sidebar.test, added @reduxjs/toolkit in devde…
Raghunandhan8818 Mar 14, 2024
1ae80bd
gitlabURL and username added in the team_sidebar.test
Raghunandhan8818 Mar 23, 2024
bb5571c
changed gitlabURL in team_sidebar.test and updated the snapshots
Raghunandhan8818 Mar 28, 2024
e4c1763
downgrade @redux/toolkit and excluded 'node_modules'from type-checkin…
Raghunandhan8818 May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36,800 changes: 21,834 additions & 14,966 deletions webapp/package-lock.json
View file
Open in desktop

Large diffs are not rendered by default.

11 changes: 6 additions & 5 deletions webapp/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
},
"dependencies": {
"@primer/octicons-react": "10.1.0",
"@testing-library/react": "12.1.2",
mickmister marked this conversation as resolved.
Show resolved Hide resolved
"core-js": "3.6.5",
"csstype": "3.0.3",
"jest": "26.4.2",
Expand All @@ -32,11 +33,6 @@
"reselect": "4.1.6"
},
"devDependencies": {
"babel-jest": "29.5.0",
"enzyme": "3.11.0",
"enzyme-adapter-react-16": "1.15.7",
"enzyme-to-json": "3.6.2",
"identity-obj-proxy": "3.0.0",
"@babel/cli": "7.11.6",
"@babel/core": "7.11.6",
"@babel/eslint-parser": "7.22.15",
Expand All @@ -57,16 +53,21 @@
"@types/react-transition-group": "4.4.0",
"@typescript-eslint/eslint-plugin": "4.1.1",
"@typescript-eslint/parser": "4.1.1",
"babel-jest": "29.5.0",
"babel-loader": "8.1.0",
"babel-plugin-typescript-to-proptypes": "1.4.1",
"css-loader": "4.3.0",
"enzyme": "3.11.0",
"enzyme-adapter-react-16": "1.15.7",
"enzyme-to-json": "3.6.2",
"eslint": "7.9.0",
"eslint-import-resolver-typescript": "3.6.1",
"eslint-import-resolver-webpack": "0.12.2",
"eslint-plugin-import": "2.22.0",
"eslint-plugin-react": "7.20.6",
"eslint-plugin-react-hooks": "4.1.2",
"file-loader": "6.1.0",
"identity-obj-proxy": "3.0.0",
"style-loader": "1.2.1",
"typescript": "4.6.4",
"webpack": "4.44.2",
Expand Down
2 changes: 1 addition & 1 deletion webapp/src/components/team_sidebar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {connect} from 'react-redux';

import TeamSidebar from './team_sidebar.jsx';
import TeamSidebar from './team_sidebar.tsx';
mickmister marked this conversation as resolved.
Show resolved Hide resolved

function mapStateToProps(state) {
const members = state.entities.teams.myMembers || {};
mickmister marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
24 changes: 0 additions & 24 deletions webapp/src/components/team_sidebar/team_sidebar.jsx
View file
Open in desktop

This file was deleted.

59 changes: 59 additions & 0 deletions webapp/src/components/team_sidebar/team_sidebar.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import React from 'react';

import {describe, expect, test, jest} from '@jest/globals';
import {render} from '@testing-library/react';

import TeamSidebar from './team_sidebar';

const mockTheme = {
sidebarBg: '#ffffff',
sidebarText: '#333333',
sidebarUnreadText: '#ff0000',
sidebarTextHoverBg: '#eeeeee',
sidebarTextActiveBorder: '#007bff',
sidebarTextActiveColor: '#007bff',
sidebarHeaderBg: '#f8f9fa',
sidebarHeaderTextColor: '#495057',
onlineIndicator: '#28a745',
awayIndicator: '#ffc107',
dndIndicator: '#dc3545',
mentionBg: '#ffeb3b',
mentionBj: '#ffeb3b',
mentionColor: '#333333',
centerChannelBg: '#f8f9fa',
centerChannelColor: '#333333',
newMessageSeparator: '#007bff',
linkColor: '#007bff',
buttonBg: '#007bff',
buttonColor: '#ffffff',
errorTextColor: '#dc3545',
mentionHighlightBg: '#ffeb3b',
mentionHighlightLink: '#007bff',
codeTheme: 'solarized-dark',
};

jest.mock('../sidebar_buttons', () => ({
__esModule: true,
default: () => <div data-testid='mocked-sidebar-buttons'/>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand why this is necessary. What's the downside to having the component render the actual SidebarButtons?

Copy link
Author

Choose a reason for hiding this comment

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

In my perspective, the responsibility of the team_sidebar component is to decide whether to render the buttons based on the show property. By mocking the sidebar_buttons component, I feel this introduces a separation of concerns that can prove to be essential.

Downside of the actual rendering of sidebar_buttons component being, we need to cover the all cases for the child component although we are testing the parent component.

I feel the actual rendering of sidebar_buttons can be done in a test of its own. wdyt ?

Copy link
Contributor

@mickmister mickmister Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@Raghunandhan8818 I agree with this. Though I personally think we should still render the tree as normal, to get some coverage on the child component and be able to notice differences in the default rendering of the child component, at our current state of "no component tests" in this particular project. IMO the benefits outweigh the drawbacks here. We don't really have a pattern of mocking components in our projects, so this is just a bit unfamiliar to me. It is common to mock things like redux actions in component tests though. Mocking a component seems like "modifying the DOM during the test" to me, and adds an additional step to understand the snapshot on its own.

Although team_sidebar has only one job, it makes the snapshot more useful if we provide it more context in this case. I'm mostly saying this because we don't have a test for sidebar_buttons at the moment, and I want to try to make up for that with this one snapshot test from the parent component. Any refactor of the sidebar_buttons component will show us rendering differences from its current state with this test.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hmhealey I'm curious what you think about this discussion on mocking child components in a component test

Copy link
Member

Choose a reason for hiding this comment

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

If possible, I'd recommend avoiding that. One of the major benefits I've found with using Testing Library is that you can write the tests in an end-to-end style without having to actually run the E2E tests. It also makes it so that the tests don't break as soon as someone refactors the component and decides to move some code into a child component. I think those are why Testing Library always renders all the children of the component instead of giving you the option of shallowly rendering the component like Enzyme does.

That said, if there's some Context or something that we're missing to deeply render the component, I could understand starting with mocking for now. We've had to do a bunch of work to set up context in web app unit tests which has resulted in us adding renderWithContext which covers most cases but still doesn't work for everything.

Relatedly, I'd also recommend not doing snapshot testing for something like this. I feel like most of the time when snapshot tests fail in the web app, people will just update them without looking to see why they failed. Even if we still want to mock the child component, I'd suggest doing it in a way that you can use expect(...).toBeInTheDocument() to confirm that the child is rendered instead of using a snapshot. That also means that people looking at the component in the future will understand more easily what to look for in the test results.

Sorry if this is all a bit of a knowledge dump. I feel like I need to write a testing manifesto to put in the developer docs 😅

Copy link
Member

Choose a reason for hiding this comment

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

Tl;dr: I'd recommend not mocking child components if possible, but we may need more test infrastructure in this repo to do that easily, so I'll defer to @mickmister on that

Copy link
Contributor

Choose a reason for hiding this comment

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

@Raghunandhan8818 So if there were more components rendered by team_sidebar, we would have to mock each one of those imports. To me, that's a sign that this testing approach can be brittle. I'm thinking we should render the tree as normal. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Sure @mickmister @hmhealey , lets do it that way. I have removed mocking from the team_sidebar.test and configured a mock store using @reduxjs/toolkit. I guess this is a good starting point. And further test cases can be added in team_sidebar.test.tsx to cover the sidebar_buttons entirely. But I feel, that can be taken as a seperate PR. wdyt ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like most of the time when snapshot tests fail in the web app, people will just update them without looking to see why they failed

Personally I highly value the snapshot tests, and take a deep look at them every time I review a PR that has them (as well as my own snapshot changes). The only downside imo is you have to update them when something about the DOM changes. Though the benefits of "seeing" that change in the PR is worth it imo.

}));

describe('TeamSidebar', () => {
test('renders nothing when show is false', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's always worth adding a snapshot test if possible when introducing tests for a component. Are we able to use toMatchSnapshot from jest here with the @testing-library/react library?
https://jestjs.io/docs/snapshot-testing#snapshot-testing-with-jest

Copy link
Author

Choose a reason for hiding this comment

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

@mickmister , I have rewritten the tests to make use of jest snapshots.

const {queryByTestId} = render(
<TeamSidebar
show={false}
theme={mockTheme}
/>);
expect(queryByTestId('mocked-sidebar-buttons')).toBeNull();
});

test('renders SidebarButtons with correct props when show is true', () => {
const {getByTestId} = render(
<TeamSidebar
show={true}
theme={mockTheme}
/>);
const sidebarButtonsElement = getByTestId('mocked-sidebar-buttons');
expect(sidebarButtonsElement).not.toBeNull();
});
});
25 changes: 25 additions & 0 deletions webapp/src/components/team_sidebar/team_sidebar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import React, {FC} from 'react';

import {Theme} from 'mattermost-redux/types/preferences';

import SidebarButtons from '../sidebar_buttons';

interface TeamSidebarProps {
show: boolean;
theme: Theme;
}

const TeamSidebar: FC<TeamSidebarProps> = ({show, theme}: TeamSidebarProps) => {
if (!show) {
return null;
}

return (
<SidebarButtons
theme={theme}
isTeamSidebar={true}
/>
);
};

export default TeamSidebar;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: There should be a newline at the end of this file

Loading