-
Notifications
You must be signed in to change notification settings - Fork 829
[JAV-554]Fix bug that when booting use java system properties for ssl configuration #357
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
Conversation
| @Test | ||
| public void testSSLOptionYamlOptionWithProperyFalse() throws Exception { | ||
| System.setProperty("ssl.authPeer", "false"); | ||
| DynamicConfiguration configFromYamlFile = |
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.
The below code is duplicated code, it's better to extract a method for setting up the configuration from system property.
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.
For unit test case, I think it quite important to make each test have only one logic (that is to say, code complexity is one) and code duplication is not a concern. If I extract a method, there got an "if" statement that make complexity is two. So I used to copy code for new test cases.
| @Test | ||
| public void testSSLOptionYamlOptionWithProperyTrue() throws Exception { | ||
| System.setProperty("ssl.authPeer", "true"); | ||
| DynamicConfiguration configFromYamlFile = |
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 seems that finalConfig equals io.servicecomb.config.ConfigUtil.createLocalConfig() result?
…ation