CIF-1657: [React Components] Clean up the unit tests warnings #419
Conversation
hbacila
requested review from
dplaton,
herzog31,
LSantha,
cjelger,
laurentiumagureanu and
mhaack
Codecov Report
@@ Coverage Diff @@
## master #419 +/- ##
============================================
- Coverage 85.85% 84.84% -1.01%
Complexity 1023 1023
============================================
Files 209 214 +5
Lines 5244 5320 +76
Branches 751 773 +22
============================================
+ Hits 4502 4514 +12
- Misses 593 657 +64
Partials 149 149
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -17,6 +17,9 @@ const setCartCookie = jest.fn(); | |||
| const setUserCookie = jest.fn(); | |||
| const dispatch = jest.fn(); | |||
|
|
|||
| // avoid console errors logged during testing | |||
| console.error = jest.fn(); | |||
There was a problem hiding this comment.
I would rather add the --silent parameter to the jest call than mocking the console.error calls here. This has the advantage that you can easily decide whether or not you want to print the console statements. Sometimes they are useful.
There was a problem hiding this comment.
The problem with --silent is that it supresses all console logs including the warnings that may indicate an issue (such the ones we fixed here).
| <ConfigContextProvider config={configObject}> | ||
| <Component {...props} /> | ||
| </ConfigContextProvider> | ||
| <I18nextProvider i18n={i18n}> |
There was a problem hiding this comment.
Since this the root of our custom component tree that we use in a lot of tests I propose we decorate render so that it includes it automatically (like in https://github.com/adobe/aem-core-cif-components/blob/master/react-components/src/utils/test-utils.js#L105).
Same with ConfigContextProvider.
Alternately, we can mock useTranslation so that it doesn't require the context provider anymore. Either way, we should simplify our mock components
There was a problem hiding this comment.
Simplify the mock components so that they won't require the I18nextProvider and the ConfigContextProvider every time. For the latter, we can always provide the full configuration.
We can do the same for the MockedProvider, but we need to allow passing the mock array to the render function
…ls/test-utils.js) which decorates the mock component with MockedProvider, ConfigContextProvider, UserContextProvider and I18nextProvider
|
Refactored the unit tests so they all use the custom render (from
|
| </UserContextProvider> | ||
| </MockedProvider> | ||
| ); | ||
| const { getByTestId } = render(<ContextWrapper />, { mocks: mocks }); |
There was a problem hiding this comment.
You can write {mocks} instead of {mocks: mocks} - shorthand notation in ES6
The unit tests for the React Components module all pass, but they throw a lot of warnings regarding missing props and the ever present
"Warning: An update to XXXX inside a test was not wrapped in act(...)."Description
This change fixes the warnings above. It also fixes a styling issue for
AccountDetails/editFormdue to emptyeditForm.cssMotivation and Context
We must cleanup these warnings as they can potentially hide some implementation issues.
How Has This Been Tested?
Types of changes
Checklist: