test: CollectionControl #13656
test: CollectionControl #13656
Conversation
|
|
||
| test('Should render', () => { | ||
| render(<CollectionControl {...defaultProps} />); | ||
| expect(screen.getByTestId('CollectionControl')).toBeInTheDocument(); |
There was a problem hiding this comment.
Just a tip, if you can't assign any reasonable role to the container component, you can always access it using:
const { container } = render(<CollectionControl {...defaultProps} />);
expect(container).toBeInTheDocument();
😉
There was a problem hiding this comment.
I know, but I avoid using const {container} = render() as much as possible. I find it more reliable to use screen methods. I've had some problems with containers, so I avoid them whenever is possible.
There was a problem hiding this comment.
I'm curious what sort of problems you were running into there that might be worth avoiding them for. I like @michael-s-molina 's implementation - it's clean, and it makes less noise in the DOM.
There was a problem hiding this comment.
@rusackas It was not in this specific case. But in the past I've had problems that container had a very different result than screen. As screen is theoretically the HTML that will be rendered in the browser, I prefer to use it. Any change in behavior or configuration will not change the test result. That way I'm sure the test is right.
I only use container when I'm doing unit tests, and even in these cases I avoid it as much as possible.
There was a problem hiding this comment.
@yardz I don't know if in the past the implementation was different. Nowadays it's very similar. It will be different when we specify a container for render that is not the default.
Kent C. Dodds in his excellent article Common mistakes with React Testing Library recommends using screen whenever possible to avoid destructuring.
I also think we should always use screen. That being said, in the case where we CANNOT find a satisfactory role/text/placeholder for our top component and we just want to test that the div has rendered, both test-id or container are valid options. Since both options are not accessible, I prefer the one where we don't leave fingerprints in the component's code.
There was a problem hiding this comment.
Very nice post, several tips!
Well, I don't think that adding this test-id has any negative side effects at any level so the discussion comes down to "use or not the test-id." When it comes down to that it seems to me more like a discussion of "taste" than of "good practices".
If you think this is a block for the merge I can change it.
There was a problem hiding this comment.
@yardz For me, it's not a blocker as indicated by the ✔️.
There was a problem hiding this comment.
I have not seen it haha
superset-frontend/src/explore/components/controls/CollectionControl/index.jsx
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #13656 +/- ##
==========================================
- Coverage 77.35% 77.22% -0.14%
==========================================
Files 918 934 +16
Lines 46652 47414 +762
Branches 5723 5921 +198
==========================================
+ Hits 36087 36614 +527
- Misses 10429 10658 +229
- Partials 136 142 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
superset-frontend/src/explore/components/controls/CollectionControl/CollectionControl.test.tsx
Show resolved
Hide resolved
superset-frontend/src/explore/components/controls/CollectionControl/CollectionControl.test.tsx
Outdated
Show resolved
Hide resolved
|
/testenv up |
|
@rusackas Ephemeral environment spinning up at http://54.191.3.22:8080. Credentials are |
* Tests for CollectionControl * add role to icon * applying factory to props
* Tests for CollectionControl * add role to icon * applying factory to props
mistercrunch
added
🏷️ bot
SUMMARY
Tests for CollectionControl component
TEST PLAN
No behavior changes. All tests must pass.