-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Extract i18n bundles and enable component test cases in react #7815
Conversation
@deepu105 , should I push changes for test cases fix here itself or you want separate merge request as that's dependent on this. |
@vishal423 since they are related push it here, so that I can test them together |
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.
LGTM.
@@ -404,7 +406,7 @@ const files = { | |||
condition: generator => generator.authenticationType !== 'oauth2', | |||
path: TEST_SRC_DIR, | |||
templates: [ | |||
'spec/app/modules/account/register/register.spec.tsx', | |||
// 'spec/app/modules/account/register/register.spec.tsx', |
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.
why is this commented out?
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.
if you look into source code, then, all test cases are commented in this file. During execution Jest marks this as failure with no executable test cases. I tried to convert one test case to valid, but, later during app generation it leads to some conflicts with prettier. We can create separate issue to enable test case of this class and while doing so can uncomment this
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.
oh, ok now I remember, I think there was some issue in this specific spec, so ya its fine
TranslatorContext.setLocale(currentLocale); | ||
} | ||
}); | ||
store.dispatch(setLocale(savedLocale)); | ||
return savedLocale; |
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.
wasnt the return used anywhere?
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 used
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.
ok
moduleFileExtensions: ['ts', 'tsx', 'js', 'jsx', 'json', 'node'], | ||
moduleNameMapper: { | ||
'app/(.*)': '<rootDir>/src/main/webapp/app/$1' | ||
'app/(.*)': '<rootDir>/src/main/webapp/app/$1', | ||
'\\.(css|scss)$': 'identity-obj-proxy' |
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.
why is this needed?
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.
If we don't proxy css modules, then, I noted issue in few test cases like for header.tsx
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.
ok
@@ -26,7 +26,7 @@ describe('private-route component', () => { | |||
expect(error.length).toEqual(1); | |||
expect(error.find('.alert-danger').html()).toEqual( | |||
<%_ if (enableTranslation) { _%> | |||
'<div class="alert alert-danger"><span>translation-not-found[error.http.403]</span></div>' | |||
'<div class="alert alert-danger"><span>You are not authorized to access this page.</span></div>' |
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.
nice, this was always bothering me
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.
:)
I may not get enough time in next few days to cover pending items. If you agree, then those can be taken care in separate issue. |
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.
I'll test and merge this later
Pending:
Few configuration files, settings can be conditional based on i18n support.
Please make sure the below checklist is followed for Pull Requests.
Travis tests are green
Tests are added where necessary
Documentation is added/updated where necessary
Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed