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

Raghunandhan8818 · 2024-03-09T10:32:17Z

Fixes #435

Added @testing-library/react , Version 12.1.2

Migrated team_sidebar component to Typescript
Added team_sidebar.test.tsx

I have added an older version of @testing-library/react , since this was only compatible with react 16.

I guess we could upgrade @testing-library/react when we plan to migrate to react 18 . wdyt ?

Signed-off-by: Raghunandhan AJ <[email protected]>

codecov · 2024-03-09T16:36:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 34.02%. Comparing base (387d84d) to head (bb5571c).
Report is 18 commits behind head on master.

❗ Current head bb5571c differs from pull request most recent head e4c1763. Consider uploading reports for the commit e4c1763 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #465      +/-   ##
==========================================
+ Coverage   33.43%   34.02%   +0.58%     
==========================================
  Files          22       22              
  Lines        4008     4021      +13     
==========================================
+ Hits         1340     1368      +28     
+ Misses       2535     2515      -20     
- Partials      133      138       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Raghunandhan AJ <[email protected]>

mickmister · 2024-03-11T22:27:57Z

I guess we could upgrade @testing-library/react when we plan to migrate to react 18 . wdyt ?

Yes sound good 👍 Right now the webapp is using React v17. We want to generally stay in line with the same version of the webapp. Most plugins are currently referencing React 16. They are still compatible because the version used at runtime will be provided by the webapp through an exposed window.React webpack external.

mattermost-plugin-gitlab/webapp/webpack.config.js

Lines 42 to 43 in 53b10f3

    
           externals: { 
        
               react: 'React',

So the version specified in this repo is just for compile-time checks, and is not actually used at runtime. React 17 was said to not have breaking changes, though React 18 will likely have breaking changes that require us to upgrade the React version in the plugins.

mickmister

Awesome work @Raghunandhan8818 👍 Thanks especially for the additions of the @testing-library/react library

The PR LGTM. I have just a few comments for discussion. Please let me know what you think. Thanks @Raghunandhan8818

webapp/src/components/team_sidebar/index.js

webapp/package.json

webapp/src/components/team_sidebar/index.js

mickmister · 2024-03-11T22:45:05Z

webapp/src/components/team_sidebar/team_sidebar.test.tsx

+}));
+
+describe('TeamSidebar', () => {
+    test('renders nothing when show is false', () => {


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

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

Signed-off-by: Raghunandhan AJ <[email protected]>

Raghunandhan8818 · 2024-03-12T17:58:43Z

Thanks @mickmister for the commendable suggestions !

Please take a look at the updated PR.

mickmister

Thanks @Raghunandhan8818, just one more comment for discussion on the PR

mickmister · 2024-03-12T22:45:39Z

webapp/src/components/team_sidebar/team_sidebar.test.tsx

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


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

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 ?

@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.

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

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 😅

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

@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?

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 ?

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.

…pendencies Signed-off-by: Raghunandhan AJ <[email protected]>

mickmister

LGTM 👍 Just one comment for discussion

Thanks @Raghunandhan8818!

mickmister · 2024-03-15T16:24:56Z

webapp/src/components/team_sidebar/team_sidebar.test.tsx

+            connected: true,
+            username: '',
+            clientId: '',
+            lhsData: {
+                reviews: [],
+                yourAssignedPrs: [],
+                yourAssignedIssues: [],
+                todos: [],
+            },
+            gitlabURL: '',
+            organization: '',
+            rhsPluginAction: () => true,


Not sure why clientId is here exposed to the client. That's a bit concerning 😅 At least there's not a clientSecret which would be detrimental

If we are setting connected: true, then I think we should also set username and gitlabURL, since those will always be present when connected is true. No worries if it doesn't change the snapshot I suppose

@mickmister , I have set values to the gitlabURL and username and updated the snapshots.

Kindly take a look at it.

Signed-off-by: Raghunandhan AJ <[email protected]>

mickmister

Thanks for addressing the above feedback @Raghunandhan8818. I have some optional non-blocking feedback. Let me know what you think and we can merge this 👍

mickmister · 2024-03-26T16:45:01Z

webapp/src/components/team_sidebar/team_sidebar.test.tsx

            clientId: '',
            lhsData: {
                reviews: [],
                yourAssignedPrs: [],
                yourAssignedIssues: [],
                todos: [],
            },
-            gitlabURL: '',
+            gitlabURL: 'https://gitlab.com/gitlab-org/gitlab.git',


This would actually be the org URL that the user is connected to, and not a repo URL

Suggested change

gitlabURL: 'https://gitlab.com/gitlab-org/gitlab.git',

gitlabURL: 'https://gitlab.com/gitlab-org',

webapp/src/components/team_sidebar/__snapshots__/team_sidebar.test.tsx.snap

mickmister · 2024-03-26T16:45:54Z

webapp/src/components/team_sidebar/team_sidebar.tsx

+    );
+};
+
+export default TeamSidebar;


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

Signed-off-by: Raghunandhan AJ <[email protected]>

Raghunandhan8818 · 2024-03-28T15:21:56Z

@mickmister , I have modified the gitlabURL and updated the snapshots as you said. Kindly take a look at it.

mickmister · 2024-03-28T19:02:52Z

@Raghunandhan8818 Looks like there are some linting issues being reported by CI:

https://github.com/mattermost/mattermost-plugin-gitlab/actions/runs/8469587481/job/23207702228?pr=465

npm ERR! npm WARN ERESOLVE overriding peer dependency
npm ERR! npm WARN While resolving: [email protected]
npm ERR! npm WARN Found: [email protected]
npm ERR! npm WARN node_modules/react
npm ERR! npm WARN   dev react@"^15.4.2" from the root project
npm ERR! npm WARN 
npm ERR! npm WARN Could not resolve dependency:
npm ERR! npm WARN peer react@"18.2.0" from [email protected]
npm ERR! npm WARN node_modules/react-native
npm ERR! npm WARN   peer react-native@">=0.59" from @react-native-community/[email protected]
npm ERR! npm WARN   node_modules/@react-native-community/netinfo
npm ERR! npm WARN   1 more (@react-native/virtualized-lists)
npm ERR! npm WARN 
npm ERR! npm WARN Conflicting peer dependency: [email protected]
npm ERR! npm WARN node_modules/react
npm ERR! npm WARN   peer react@"18.2.0" from [email protected]
npm ERR! npm WARN   node_modules/react-native
npm ERR! npm WARN     peer react-native@">=0.59" from @react-native-community/[email protected]
npm ERR! npm WARN     node_modules/@react-native-community/netinfo
npm ERR! npm WARN     1 more (@react-native/virtualized-lists)
npm ERR! npm WARN ERESOLVE overriding peer dependency

This is only happening for the lint job for some reason, and it's failing during installing dependencies. Maybe we need to downgrade the version of @reduxjs/toolkit?

I ran the job again and am getting a different error:

https://github.com/mattermost/mattermost-plugin-gitlab/actions/runs/8469587481/job/23208034368

> [email protected] check-types
> tsc
Error: node_modules/@reduxjs/toolkit/node_modules/reselect/dist/reselect.d.ts(8,122): error TS1005: '?' expected.
Error: node_modules/@reduxjs/toolkit/node_modules/reselect/dist/reselect.d.ts(11,1): error TS1005: '?' expected.
make: *** [Makefile:62: check-style] Error 2
Error: Process completed with exit code 2.

I'm thinking we need to tell typescript to ignore any errors in node modules. I see we're using skipLibCheck in tsconfig.json, but the @reduxjs/toolkit library is still being processed.

…g in the tsconfig.json Signed-off-by: Raghunandhan AJ <[email protected]>

Raghunandhan8818 · 2024-05-07T15:39:33Z

Heyy @mickmister , as you mentioned i have downgraded @reduxjs/toolkit and i excluded entire node_modules explictly in the tsconfig.json . What do you think and let me know if I have to do anything.

mickmister · 2024-05-14T13:39:13Z

Hi @Raghunandhan8818, thanks for addressing those concerns. It looks like there are some merge conflicts in package-lock.json due to recent changes. Are you able to resolve these? Thank you @Raghunandhan8818

raghavaggarwal2308 · 2024-12-11T10:52:47Z

@Raghunandhan8818 Are you open to resolving the conflicts on this PR?

raghavaggarwal2308 · 2024-12-13T09:55:02Z

@Raghunandhan8818 There are still conflicts present in the PR. Can you please check?

mattermost-build · 2024-12-24T01:04:08Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

Raghunandhan8818 added 2 commits March 3, 2024 23:48

migrate team sidebar component to typescript

3a28f7a

Signed-off-by: Raghunandhan AJ <[email protected]>

added tests to the team_sidebar components

7b3775b

Signed-off-by: Raghunandhan AJ <[email protected]>

Raghunandhan8818 requested review from mickmister and spirosoik as code owners March 9, 2024 10:32

Raghunandhan8818 mentioned this pull request Mar 9, 2024

Convert team_sidebar component to typescript #435

Open

fixed lint issues

9d0099e

Signed-off-by: Raghunandhan AJ <[email protected]>

mickmister reviewed Mar 11, 2024

View reviewed changes

moved rtl to devdependencies, added snapshot tests to team_sidebar

d7dacd5

Signed-off-by: Raghunandhan AJ <[email protected]>

Raghunandhan8818 changed the title ~~Migrate team_sidebar component to typescript and added tests~~ Add @testing-library/react and component tests Mar 12, 2024

Raghunandhan8818 requested a review from mickmister March 12, 2024 17:56

mickmister reviewed Mar 12, 2024

View reviewed changes

Raghunandhan8818 requested a review from mickmister March 13, 2024 17:11

mickmister added the 3: QA Review Requires review by a QA tester label Mar 13, 2024

mickmister approved these changes Mar 13, 2024

View reviewed changes

mickmister requested a review from AayushChaudhary0001 March 13, 2024 20:37

removed mocking in team_sidebar.test, added @reduxjs/toolkit in devde…

8c95b3b

…pendencies Signed-off-by: Raghunandhan AJ <[email protected]>

Raghunandhan8818 requested review from mickmister and hmhealey March 14, 2024 18:04

mickmister approved these changes Mar 15, 2024

View reviewed changes

gitlabURL and username added in the team_sidebar.test

1ae80bd

Signed-off-by: Raghunandhan AJ <[email protected]>

Raghunandhan8818 requested a review from mickmister March 23, 2024 15:57

mickmister reviewed Mar 26, 2024

View reviewed changes

changed gitlabURL in team_sidebar.test and updated the snapshots

bb5571c

Signed-off-by: Raghunandhan AJ <[email protected]>

Raghunandhan8818 requested a review from mickmister March 28, 2024 15:22

mickmister approved these changes Mar 28, 2024

View reviewed changes

downgrade @redux/toolkit and excluded 'node_modules'from type-checkin…

e4c1763

…g in the tsconfig.json Signed-off-by: Raghunandhan AJ <[email protected]>

Raghunandhan8818 requested a review from mickmister May 14, 2024 08:07

mickmister removed their request for review July 10, 2024 05:24

Fix. conflicts in package.json

f051d2e

Raghunandhan8818 requested a review from wiggin77 as a code owner December 12, 2024 17:11

mattermost-build added the Lifecycle/1:stale label Dec 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Raghunandhan8818 commented Mar 9, 2024 •

edited

Loading

codecov bot commented Mar 9, 2024 •

edited

Loading

mickmister commented Mar 11, 2024

mickmister left a comment

mickmister Mar 11, 2024

Raghunandhan8818 Mar 12, 2024

Raghunandhan8818 commented Mar 12, 2024

mickmister left a comment

mickmister Mar 12, 2024

Raghunandhan8818 Mar 13, 2024

mickmister Mar 13, 2024 •

edited

Loading

mickmister Mar 13, 2024

hmhealey Mar 14, 2024

hmhealey Mar 14, 2024

mickmister Mar 14, 2024

Raghunandhan8818 Mar 14, 2024

mickmister Mar 15, 2024

mickmister left a comment

mickmister Mar 15, 2024

Raghunandhan8818 Mar 23, 2024

mickmister left a comment

mickmister Mar 26, 2024

mickmister Mar 26, 2024

Raghunandhan8818 commented Mar 28, 2024

mickmister commented Mar 28, 2024

Raghunandhan8818 commented May 7, 2024

mickmister commented May 14, 2024

raghavaggarwal2308 commented Dec 11, 2024

raghavaggarwal2308 commented Dec 13, 2024

mattermost-build commented Dec 24, 2024

	gitlabURL: 'https://gitlab.com/gitlab-org/gitlab.git',
	gitlabURL: 'https://gitlab.com/gitlab-org',

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

Are you sure you want to change the base?

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

Conversation

Raghunandhan8818 commented Mar 9, 2024 • edited Loading

codecov bot commented Mar 9, 2024 • edited Loading

Codecov Report

mickmister commented Mar 11, 2024

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Raghunandhan8818 commented Mar 12, 2024

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Raghunandhan8818 commented Mar 28, 2024

mickmister commented Mar 28, 2024

Raghunandhan8818 commented May 7, 2024

mickmister commented May 14, 2024

raghavaggarwal2308 commented Dec 11, 2024

raghavaggarwal2308 commented Dec 13, 2024

mattermost-build commented Dec 24, 2024

Raghunandhan8818 commented Mar 9, 2024 •

edited

Loading

codecov bot commented Mar 9, 2024 •

edited

Loading

mickmister Mar 13, 2024 •

edited

Loading