test: replacing snapshot tests with rtl tests part 12 #2222
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @jacobo-dominguez-wgu! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
jacobo-dominguez-wgu
added
the
mao-onboarding
jacobo-dominguez-wgu
force-pushed
the
depr-shallow-spanshot-test-12
branch
from
96cc163 to
e111ebc
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2222 +/- ##
==========================================
+ Coverage 94.09% 94.18% +0.08%
==========================================
Files 1164 1167 +3
Lines 24507 24678 +171
Branches 5315 5270 -45
==========================================
+ Hits 23061 23243 +182
+ Misses 1369 1367 -2
+ Partials 77 68 -9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Adding my review:
|
|
Video Editor Component
Side note - I checked on both this branch and master, and if you have multiple transcripts added, and delete 1 of them, all rows get deleted and user has to start over: |
|
Problem Editor Component:
I wasn't sure how to test the Randomization settings of the component from a product/user perspective but tests did pass. |
| import { | ||
| render, screen, initializeMocks, fireEvent, | ||
| } from '@src/testUtils'; | ||
| import { formatMessage } from '../../../../../../../testUtils'; |
There was a problem hiding this comment.
Curious if we should use a barrel file or a baseUrl in the tsconfig settings to avoid these long relative paths. I see a couple of imports like this throughout these tests. See: https://stackoverflow.com/a/34926643 for examples.
jacobo-dominguez-wgu
force-pushed
the
depr-shallow-spanshot-test-12
branch
from
e111ebc to
f1b2e9f
Compare
| describe('mapDispatchToProps', () => { | ||
| test('setBlockTitle from actions.app.setBlockTitle', () => { | ||
| expect(mapDispatchToProps.setBlockTitle).toEqual(actions.app.setBlockTitle); | ||
| }); | ||
| }); | ||
|
|
||
| describe('mapDispatchToProps', () => { | ||
| test('updateSettings from actions.problem.updateSettings', () => { | ||
| expect(mapDispatchToProps.updateSettings).toEqual(actions.problem.updateSettings); | ||
| }); | ||
| }); | ||
|
|
||
| describe('mapDispatchToProps', () => { | ||
| test('updateField from actions.problem.updateField', () => { | ||
| expect(mapDispatchToProps.updateField).toEqual(actions.problem.updateField); | ||
| }); | ||
| }); | ||
|
|
||
| describe('mapDispatchToProps', () => { | ||
| test('updateAnswer from actions.problem.updateAnswer', () => { | ||
| expect(mapDispatchToProps.updateAnswer).toEqual(actions.problem.updateAnswer); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
I don't think these are useful tests, and they're definitely not user-centric in any way. In particular, mapDispatchToProps should be eventually replaced with useDispatch hooks, and mapStateToProps should be replaced with useSelector hooks (ref), so I don't really want the tests to be depending on that sort of implementation detail.
If it's a coverage issue that you're trying to solve, it may be necessary to just do that mapDispatchToProps -> useDispatch refactor now (it's easy), and the lines will be covered since the hooks will be used in the component render.
There was a problem hiding this comment.
Actually those test were already there, I did not remove/update them because were not using any of the elements of the react-unit-test-utils library. I will remove it now since they are not providing any extra value and are not user centric.
...or/components/EditProblemView/SettingsWidget/settingsComponents/Randomization/index.test.tsx
Outdated
Show resolved
Hide resolved
| describe('mapDispatchToProps', () => { | ||
| const dispatch = jest.fn(); | ||
| test('updateField from actions.video.updateField', () => { | ||
| // @ts-ignore | ||
| expect(mapDispatchToProps.updateField).toEqual(dispatch(actions.video.updateField)); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Same here, don't think there's any value in doing basic tests on the values of mapDispatchToProps
There was a problem hiding this comment.
Removed.
jacobo-dominguez-wgu
force-pushed
the
depr-shallow-spanshot-test-12
branch
from
f1b2e9f to
b958d75
Compare
Description
This pr is replacing shallow and snapshot tests with rtl tools in order to deprecate @edx/react-unit-test-utils package.
Closes #2122
UT modified on this pr:
The snapshots related to these tests were removed.
The tests were turned into typescript files.