Skip to content
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

[IOTDB-417] Made all tests Locale Invariant by always explicitly defaulting to Locale.ENGLISH #726

Merged
merged 1 commit into from
Jan 10, 2020

Conversation

JulianFeinauer
Copy link
Contributor

@sonarcloud
Copy link

sonarcloud bot commented Jan 9, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sunjincheng121
Copy link
Member

I would like to have a look.

Copy link
Member

@sunjincheng121 sunjincheng121 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @JulianFeinauer
The PR looks good to me for fix this issue, I only left one comment for simple the code change and one suggestion as follows:

I checked the entire project and found that String.format is used in many places(not only in test code). We are fortunate to find this Local problem in ITCase, but we cannot rule out that there are unpredictable problems hidden in other places, so I think would be better make a util similar with String.format in IoTDB , and set the configuration we need in the util, such as Local information, so that even if other problems are found in the future, we can fix it as a whole by modifying one place.

What do you think? @JulianFeinauer @jixuan1989

@@ -236,12 +236,12 @@ public void prepareMerge() throws SQLException {

// prepare BufferWrite data
for (int i = 10001; i <= 20000; i++) {
statement.execute(String.format(insertTemplate, i, i, i, (double) i, "\'" + i + "\'",
statement.execute(String.format(Locale.ENGLISH, insertTemplate, i, i, i, (double) i, "\'" + i + "\'",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion: could you please set the default local in setUp(), e.g., Locale.setDefault(Locale.ENGLISH);, then we only need one line change.
What do you think?

@jixuan1989
Copy link
Member

Thanks for the PR @JulianFeinauer
The PR looks good to me for fix this issue, I only left one comment for simple the code change and one suggestion as follows:

I checked the entire project and found that String.format is used in many places(not only in test code). We are fortunate to find this Local problem in ITCase, but we cannot rule out that there are unpredictable problems hidden in other places, so I think would be better make a util similar with String.format in IoTDB , and set the configuration we need in the util, such as Local information, so that even if other problems are found in the future, we can fix it as a whole by modifying one place.

What do you think? @JulianFeinauer @jixuan1989

I think many String.format() is used for log. Because all messages are in English, I wonder why a Locale is needed...

@JulianFeinauer
Copy link
Contributor Author

Excellent idea... The issue is that with e. G German locals decimal points are printed as comma...

@JulianFeinauer
Copy link
Contributor Author

@jixuan1989 if String.format is used for log only we have no issue but in the places above it is used to concatenate (or expand) Query Strings. And here, for some Locales (like e.g. German) the decimal points are expanded to decimal commas which leads to syntactically wrong statements, of course. So this, for sure, has to be Locale Invariant!

@qiaojialin qiaojialin merged commit 91ccddd into master Jan 10, 2020
@qiaojialin qiaojialin deleted the bugfix/fails-on-ger-locale branch January 10, 2020 11:54
@jixuan1989
Copy link
Member

I have checked. All String.format are used for either printing log or test. So, it should be ok.

@LeiRui LeiRui changed the title Made all tests Locale Invariant by always explicitly defaulting to Lo… [IOTDB-417] Made all tests Locale Invariant by always explicitly defaulting to Lo… Jan 14, 2020
@LeiRui LeiRui changed the title [IOTDB-417] Made all tests Locale Invariant by always explicitly defaulting to Lo… [IOTDB-417] Made all tests Locale Invariant by always explicitly defaulting to Locale.ENGLISH Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants