[Help wanted] Renovate unit tests against deprecated findDOMNode API
#3200
Replies: 12 comments 23 replies
-
The additional commentThis comment is meant to provide hints about how to refactor the tests in specific scenarios. Rendering a React componentTo render a React component, old tests use getDOMNode(<Component />);New tests uses render(<Component />);Rendering multiple instances of a React component with different propsIn old tests, we do this by calling const instance1 = getDOMNode(<Component {...props1} />);
const instance2 = getDOMNode(<Component {...props2} />);
expect(instance1).some.thing;
expect(instance2).some.thing;In new tests, we use const { rerender } = render(<Component {...props1} />);
expect(...).some.thing;
rerender(<Component {...props2} />);
expect(...).some.thing;Getting the root element of rendered componentTo access the root element of rendered element, old tests uses the returned value of const instance = getDOMNode(<Component />);In new tests, most of the time we don't directly access the root element, but follow the guiding principles of React Testing Library. But if it does consider the element detail (e.g. testing it has expected class names), most of time you can access the root element via the const elementRef = render(<Component ref={elementRef} />);
expect(elementRef.current).some.thing;For those components where const { container } = render(<Component />)
expect(container.firstChild).some.thing;Testing class names, styles, attributesTo check that an element has expected class names, old tests cases might go const instance = getDOMNode(<Component />);
expect(instance.className).to.include('some-class');
// or
assert.include(instance.className, 'some-class');New tests make use of the const { container } = render(<Component />);
expect(container.firstChild).to.have.class('some-class');Same rule applies to checking element's style and attributes. Use Common test casesMost components has these 3 common test cases:
In old tests, they usually look like this: it('Should have a custom className', () => {
const instance = getDOMNode(<Component className="custom" />);
assert.include(instance.className, 'custom');
});
it('Should have a custom style', () => {
const fontSize = '12px';
const instance = getDOMNode(<Component style={{ fontSize }} />);
assert.equal(instance.style.fontSize, fontSize);
});
it('Should have a custom className prefix', () => {
const instance = getDOMNode(<Component classPrefix="custom-prefix" />);
assert.include(instance.className, 'custom-prefix');
});In new tests, they are replaced by a test util testStandardProps(<Component />) |
Beta Was this translation helpful? Give feedback.
-
|
@SevenOutman |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for taking part and thanks for being the first to volunteer! I've marked these components you pick as assigned. Feel free to ask anything about the development while you roll! |
Beta Was this translation helpful? Give feedback.
-
|
@SevenOutman |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for taking part, that's quite many components! I've marked these components you pick as assigned. Feel free to ask anything about the development while you roll! |
Beta Was this translation helpful? Give feedback.
-
|
@SevenOutman |
Beta Was this translation helpful? Give feedback.
-
|
I guess you mean FormControl,FormControlLabel,FormErrorMessage,FormGroup,FormHelpText,Grid as you mentioned in this comment. I've marked these components as assigned to you. Enjoy your second roll! |
Beta Was this translation helpful? Give feedback.
-
|
hi @SevenOutman, I'd like to help with components below (from Header -to Modal) during this weekend :)
|
Beta Was this translation helpful? Give feedback.
-
|
Thanks for taking part! I've marked these components you pick as assigned. Enjoy your coding weekend! Feel free to ask anything about the development while you roll! |
Beta Was this translation helpful? Give feedback.
-
|
@SevenOutman |
Beta Was this translation helpful? Give feedback.
-
|
Wow, that's so quick! I would suggest we get you first PR reviewed and merged first, but it's alright if you want to pick more tasks right now :). Please confirm in a reply if you decide to take new tasks now, and I'll assign those modules to you. |
Beta Was this translation helpful? Give feedback.
-
|
I think that I should deal with my first PR first.After I merged the PR,I will apply to pick more component |
Beta Was this translation helpful? Give feedback.
-
|
@SevenOutman |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for taking part! I've marked Tag as assigned to you. Have a good time testing! Feel free to ask anything about the development while you roll! BTW, would you like to also take TagGroup, as Tag and TagGroup are quite closely related? |
Beta Was this translation helpful? Give feedback.
-
|
Yes, that's a great idea! I would love to include TagGroup as well. Thank you for suggesting it. |
Beta Was this translation helpful? Give feedback.
-
|
Great! I've marked TagGroup as assigned to you as well. |
Beta Was this translation helpful? Give feedback.
-
|
@SevenOutman
By the way, if you have the time, I would appreciate it if you could review these pull requests related to the previous task: |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for being so helpful! I've peeked those PR and they generally look good to me. I've marked these components as assigned to you, happy coding & testing! |
Beta Was this translation helpful? Give feedback.
-
|
@SevenOutman |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for taking part! I've marked these components you pick as assigned to you. Hope you enjoy the coding & testing! Feel free to ask anything about the development while you roll! |
Beta Was this translation helpful? Give feedback.
-
|
@SevenOutman |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for being willing to help! I've marked these components you pick as assigned to you. Enjoy the game and feel free to ask anything about the development while you roll! BTW, would you like to also take Tree, as Tree and TreePicker are quite closely related? |
Beta Was this translation helpful? Give feedback.
-
|
Yes. No problem, I didn't saw that it was available. I'll take care of it. |
Beta Was this translation helpful? Give feedback.
-
|
That's great! I've marked Tree as assigned to you as well. |
Beta Was this translation helpful? Give feedback.
-
|
@SevenOutman |
Beta Was this translation helpful? Give feedback.
-
Yes
@Gemini-Kang Could you please confirm this? |
Beta Was this translation helpful? Give feedback.
-
|
@SevenOutman |
Beta Was this translation helpful? Give feedback.
-
|
That's great. I've marked |
Beta Was this translation helpful? Give feedback.
-
|
Question over here. What do you prefer, this way: |
Beta Was this translation helpful? Give feedback.
-
|
I prefer explicitly declaring the rules to disable, so that others (or me afterwards) get more info about what's going on here. |
Beta Was this translation helpful? Give feedback.
-
|
Hi, sorry for the delay. Wild weeks at work. UploaderTrigger isn't passing "should accept custom style" from testStandardProps(). Only that one. I'm missing something here? I saw this small difference on the older way to test it. Maybe it's related: Any idea? |
Beta Was this translation helpful? Give feedback.
-
|
This is because |
Beta Was this translation helpful? Give feedback.
-
|
Hi community! I've tried to make a few tests but got some strange behavior. |
Beta Was this translation helpful? Give feedback.
-
|
The https://github.com/rsuite/rsuite/blob/main/karma.conf.js#L21 # only run tests in src/Modal directory
M=Modal npm run tdd# only run tests in src/Modal/test/ModalDialogSpec.tsx
F=src/Modal/test/ModalDialogSpec.tsx npm run tdd |
Beta Was this translation helpful? Give feedback.
-
|
Ok, I get it. Thank you for explanation @simonguo ) |
Beta Was this translation helpful? Give feedback.
-
Hi community, here's the first "good first issue" in rsuite repo (not an actual issue though), which means here're some tasks that we want to invite you to participate.
I think it's a good opportunity for you if you either:
What's the task?
TL;DR To update the way we used to render React components in test scripts - which is using
getDOMNode()util fromtest/testUtils.js- to using React Testing Libraryrender().Background
Old unit tests in rsuite repo uses a util function called
getDOMNode()to render a React component and retrieves its root element. This util function does not have proper cleanup and uses the deprecatedfindDOMNodeAPI under the hood. Last year we adopted React Testing Library for new tests, but left old tests as is. But as we're updating the React version, it becomes necessary to drop the usage of deprecated APIs.What could I learn?
How do I contribute?
If you find yourself interested in contributing, please leave a comment below and tell me which components/modules you would like to work on.
Here's a (long) list of components/modules that are using
getDOMNode()util in their unit tests and needs to be renovated.Expand
You would checkout a new branch based on latest
mainbranch, finish your edit, then open a pull request targetingmainbranch.For your reference, I'll post an additional comment to provide several examples of how to refactor the tests in specific scenarios.
Additionally, I've already updated some modules (excluded from the list) in #3046. You could always refer to that PR to see real-world examples.
Beta Was this translation helpful? Give feedback.
All reactions