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

MNT Use React Testing Library #1497

Merged
merged 1 commit into from
May 10, 2023
Merged

MNT Use React Testing Library #1497

merged 1 commit into from
May 10, 2023

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Apr 11, 2023 •
edited
Loading

Issue #1508

@emteknetnz emteknetnz mentioned this pull request Apr 11, 2023
Closed
3 tasks
@emteknetnz emteknetnz force-pushed the pulls/2.0/testing-library-react branch 29 times, most recently from 6d57979 to 21a23fa Compare April 18, 2023 04:22
@emteknetnz emteknetnz force-pushed the pulls/2.0/testing-library-react branch 5 times, most recently from 18d5e03 to c05a763 Compare May 2, 2023 00:31
@emteknetnz emteknetnz mentioned this pull request May 2, 2023
Closed
@emteknetnz emteknetnz force-pushed the pulls/2.0/testing-library-react branch 2 times, most recently from a4bec51 to 4e89f22 Compare May 3, 2023 02:46
emteknetnz
emteknetnz commented May 3, 2023
View reviewed changes
client/src/components/TreeDropdownField/tests/TreeDropdownField-test.js

describe('lazyLoad()', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Even after spending several hours on this, I was simply unable to get lazyLoad() to trigger

@emteknetnz emteknetnz force-pushed the pulls/2.0/testing-library-react branch from 4e89f22 to 4ad23cf Compare May 3, 2023 03:04
emteknetnz
emteknetnz commented May 3, 2023
View reviewed changes
client/src/components/TreeDropdownField/tests/TreeDropdownField-test.js

describe('handleKeyDown()', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

These were all testing whether a class method was called, rather than testing if a callback passed as a callback was called

@emteknetnz emteknetnz force-pushed the pulls/2.0/testing-library-react branch from 4ad23cf to 18fe346 Compare May 3, 2023 03:21
emteknetnz
emteknetnz commented May 3, 2023
View reviewed changes
client/src/components/TreeDropdownField/tests/TreeDropdownField-test.js

describe('getDropdownOptions()', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

These were all testing the output of a class method that was passed as a prop to a react-select component

@emteknetnz emteknetnz force-pushed the pulls/2.0/testing-library-react branch from 18fe346 to 9beffce Compare May 3, 2023 03:24
emteknetnz
emteknetnz commented May 3, 2023
View reviewed changes
client/src/components/TreeDropdownField/tests/TreeDropdownField-test.js

describe('getPath()', () => {
it('should return a breadcrumb path separated by /', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This test is now covered by TreeDropdownField getBreadcrumb() should traverse the path given and return the relevant nodes in the tree

@emteknetnz emteknetnz force-pushed the pulls/2.0/testing-library-react branch from 9beffce to ac44f1e Compare May 3, 2023 03:56
emteknetnz
emteknetnz commented May 3, 2023
View reviewed changes
client/src/components/PopoverOptionSet/tests/PopoverOptionSet-test.js
expect(wrapper.find(Button).length).toEqual(2);
});

it('render all buttons matching the search term', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

These two tests would rely on logic passed in from the onSearch callback i.e. these tests wouldn't be testing anything within the actual code

@emteknetnz emteknetnz force-pushed the pulls/2.0/testing-library-react branch from ac44f1e to ec16b84 Compare May 3, 2023 04:18
emteknetnz
emteknetnz commented May 3, 2023
View reviewed changes
client/src/containers/InsertLinkModal/tests/InsertLinkModal-test.js
@@ -1,96 +0,0 @@
/* global jest, describe, it, expect, beforeEach */
Copy link
Member Author

Choose a reason for hiding this comment

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

This component doesn't render at all when using RTL

@emteknetnz emteknetnz force-pushed the pulls/2.0/testing-library-react branch from ec16b84 to b06da91 Compare May 3, 2023 05:04
emteknetnz
emteknetnz commented May 3, 2023
View reviewed changes
client/src/components/Breadcrumb/tests/breadcrumb-test.js
@@ -1,76 +0,0 @@
/* global jest, describe, beforeEach, it, expect */
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the casing of the filename to 'Breadcrumb-test.js'

@emteknetnz emteknetnz force-pushed the pulls/2.0/testing-library-react branch from b06da91 to 2af0267 Compare May 3, 2023 05:26
@emteknetnz emteknetnz marked this pull request as ready for review May 3, 2023 06:14
sabina-talipova
sabina-talipova suggested changes May 9, 2023
View reviewed changes
Copy link
Contributor

@sabina-talipova sabina-talipova left a comment

Choose a reason for hiding this comment

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

Just small comment from https://testing-library.com/docs/react-testing-library/api/#container-1

If you find yourself using container to query for rendered elements then you should reconsider! The other queries are designed to be more resilient to changes that will be made to the component you're testing. Avoid using container to query for elements!

client/src/components/Badge/tests/Badge-test.js Outdated Show resolved Hide resolved
client/src/components/Breadcrumb/tests/Breadcrumb-test.js Show resolved Hide resolved
client/src/components/CompositeField/tests/CompositeField-test.js Outdated Show resolved Hide resolved
client/src/components/FieldHolder/tests/FieldHolder-test.js Outdated Show resolved Hide resolved
client/src/components/FieldHolder/tests/FieldHolder-test.js Outdated Show resolved Hide resolved
client/src/components/Form/tests/Form-test.js Show resolved Hide resolved
client/src/components/Form/tests/Form-test.js Outdated Show resolved Hide resolved
client/src/components/TextField/tests/TextField-test.js Show resolved Hide resolved
client/src/components/TextField/tests/TextField-test.js Outdated Show resolved Hide resolved
@emteknetnz
Copy link
Member Author

If you find yourself using container to query for rendered elements then you should reconsider!

We're way too far through this progress to move away from container based querying at this point since that's what the majority of the refactored tests use

@emteknetnz emteknetnz force-pushed the pulls/2.0/testing-library-react branch from 2af0267 to 33391ab Compare May 9, 2023 22:45
sabina-talipova
sabina-talipova approved these changes May 10, 2023
View reviewed changes
Copy link
Contributor

@sabina-talipova sabina-talipova left a comment

Choose a reason for hiding this comment

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

Nice work! Looks good to me 👍

@sabina-talipova sabina-talipova merged commit 5b4d833 into silverstripe:2.0 May 10, 2023
@sabina-talipova sabina-talipova deleted the pulls/2.0/testing-library-react branch May 10, 2023 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sabina-talipova sabina-talipova sabina-talipova approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@emteknetnz @sabina-talipova