-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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.
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
Language provider test improvement #2832
Language provider test improvement #2832
Conversation
- Merged App and HomePage slices - RepoListItem becomes a component since we determine ownership of repo in the saga - Delete all useless actions/constants/reducer files - Update all broken tests - Add ESLint exception for reducer files
- Add tests to improve coverage
* Update generators for switch to Redux Toolkit * Remove useless comments from tests * Turn multiline comment into many single line comments ;)
# Conflicts: # app/containers/App/index.js # app/containers/LanguageProvider/reducer.js # app/containers/LocaleToggle/index.js # internals/templates/containers/LanguageProvider/reducer.js # package.json
# Conflicts: # app/containers/App/selectors.js # package.json
defaultMessage: 'This is some default message', | ||
en: 'This is some en message', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's my understanding that react-intl
doesn't work like this for adding translations. I actually feel this format makes more sense than having a translation file for each locale but maybe I'm in the minority.
Because of this, I changed the id
to match the format found in app/translations/en.json
</Provider>, | ||
); | ||
expect(queryByText(text)).toBeInTheDocument(); | ||
}); | ||
|
||
it('should render the default language messages', () => { | ||
const { queryByText } = render( | ||
<Provider store={store}> | ||
<Provider store={configureStore({ language: { locale: undefined } })}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because DEFAULT_LOCALE
is set to en
in app/locales.js
, we can't properly test the defaultMessage
value unless we set the language store locale to be undefined
.
if ( | ||
locale === 'en' || | ||
translationMessages[locale][messages.someMessage.id] | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test assumes that en
will always have a test "translation" different from defaultMessage
, but won't fail if any languages added in the future don't have one defined. That being said, if another language does have a translation defined, but the rendered value is different than expected for the locale, it should still throw an error.
|
||
export default function LocaleToggle() { | ||
const { locale } = useSelector(stateSelector); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not super important, but this {
}
difference is what inspired this pull request. The relevant tests will fail if { locale }
is used instead of locale
now since { locale }
will be undefined
when the locale actually is defined.
All changes outside of the ones I commented on are from #2823 and I made the mistake of making this branch off of those changes. I don't see much harm in waiting until #2823 is merged in to accept this pull request, but if we want it branched off of |
# Conflicts: # .all-contributorsrc # app/containers/App/selectors.js # app/containers/HomePage/selectors.js # app/containers/LanguageProvider/index.js # app/containers/LanguageProvider/selectors.js # app/containers/LocaleToggle/index.js # internals/templates/containers/App/selectors.js # internals/templates/containers/LanguageProvider/index.js # internals/templates/containers/LanguageProvider/selectors.js
I'm not following. This PR contains the changes of #2823 as well as the testing improvements for the LanguageProvider? Is that on purpose? |
@julienben you originally asked I add these changes #2823, but once I really got into it I felt like adding them there would possibly convolute that pull request. I was thinking this one could wait until #2823 was merged in. If you would like I can remove the changes in #2823 from this pull request, and make it standalone. In hindsight, I probably should have just done that from the start |
Merging due to the major v5 release. Read the discussion here |
Edit: Sorry for merging this somewhere else due to #2935. If you want please re submit this PR to the dev branch if that means anything to you |
This pull request solves #2824. It was requested to be committed to my pull request #2823, but it ended up being a bit bigger of a change than I anticipated, so I decided to make a separate pull request to be sure these changes don't hold up merging in #2823.
Diving into the LanguageProvider tests, I realized the current tests didn't test changing locales as thoroughly as I felt they should. I believe this pull request tests multiple locales more thoroughly. Like my last pull request, I'm still very new to test suites, but I feel more confident than on my last pull request.
React Boilerplate
Before opening a pull request, please ensure:
dev
and targetsdev
Be kind to code reviewers, please try to keep pull requests as small and focused as possible :)
IMPORTANT: By submitting a patch, you agree to allow the project
owners to license your work under the terms of the MIT License.