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

Test failed due to locale message settings. (with non-English locale settings) #3896

Closed
Luolc opened this Issue Mar 2, 2017 · 12 comments

Comments

Projects
None yet
3 participants
@Luolc
Contributor

Luolc commented Mar 2, 2017

With a non-English locale settings (in my case is zh-CN), running CheckerTest#testHaltOnExceptionOff will assert an error as following:

org.junit.ComparisonFailure: error message 0 
Expected :path/to/checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/InputMain.java:0: Got an exception - java.lang.IndexOutOfBoundsException: test
Actual   :path/to/checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/InputMain.java:0: 异常 - java.lang.IndexOutOfBoundsException: test

// something else...

The difference is between Got an exception and 异常, while 异常 is a Chinese translation of Got an exception.

I find that these two words are defined in messages.properties and messages_zh.properties.
In messages.properties

general.exception=Got an exception - {0}

In messages_zh.properties

general.exception=异常 - {0}

And the configuration is loaded by LocalizedMessage. We can set the locale manually by LocalizedMessage#setLocale and this method is called by Checker. I thought setting the locale in the test method should solve the bug.

@Override
    public void finishLocalSetup() throws CheckstyleException {
        final Locale locale = new Locale(localeLanguage, localeCountry);
        LocalizedMessage.setLocale(locale);

So back to the error itself. In method CheckerTest#testHaltOnExceptionOff, I set the locale by adding

checker.setLocaleLanguage(Locale.ENGLISH.getLanguage());
checker.finishLocalSetup();

and the test in Intellij turns green after that.

But the same error still occurs when I run mvn clean verify in Terminal:

Failed tests:
  CheckerTest.testHaltOnExceptionOff:780->BaseCheckTestSupport.verify:231->BaseCheckTestSupport.verify:262 error message 0 expected:<...e/InputMain.java:0: [Got an exception] - java.lang.IndexOu...> but was:<...e/InputMain.java:0: [异常] - java.lang.IndexOu...>

That's the most effort I could do by now. I don't know what to do next.
The bug bothers me a lot for I could not use mvn clean verify to ensure my change is valid before sending a PR. It's really sad :(

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 2, 2017

Member

as you have "zh-CN", java suppose to load "_zh.properties" files.
But looks like smth is not working ......

@Luolc , please try to do copy-paste of _zh.properties to _zh-CN.properties and look in debug what files java is expected to load for your locale.

it is not clear why CI for ("zh") is not failing - https://travis-ci.org/checkstyle/checkstyle/jobs/206991113

Member

romani commented Mar 2, 2017

as you have "zh-CN", java suppose to load "_zh.properties" files.
But looks like smth is not working ......

@Luolc , please try to do copy-paste of _zh.properties to _zh-CN.properties and look in debug what files java is expected to load for your locale.

it is not clear why CI for ("zh") is not failing - https://travis-ci.org/checkstyle/checkstyle/jobs/206991113

@Luolc

This comment has been minimized.

Show comment
Hide comment
@Luolc

Luolc Mar 2, 2017

Contributor

@romani
It loads _zh_CN.properties
I do a copy paste and change 异常 to 异常二 in _zh_CN file. I observed LocalizedMessage load the _zh_CN bundle and the error message is also contains 异常二:

Failed tests:
  CheckerTest.testHaltOnExceptionOff:778->BaseCheckTestSupport.verify:231->BaseCheckTestSupport.verify:262 error message 0 expected:<...e/InputMain.java:0: [Got an exception] - java.lang.IndexOu...> but was:<...e/InputMain.java:0: [异常二] - java.lang.IndexOu...>

I don't think the zh CI should pass.

final String[] expected = {
            "0: Got an exception - java.lang.IndexOutOfBoundsException: test",
        };

        verify(checker, filePath, filePath, expected);

The expected error message is hard coded Got an exception. When the locale is non-English, obviously the error message won't be same.
---
mvn clean integration-test failsafe:verify -DargLine='-Duser.language=en -Duser.country=US' doesn't fail in my environment.
mvn clean integration-test failsafe:verify -DargLine='-Duser.language=zh -Duser.country=CN' or mvn clean verify does fail.

Contributor

Luolc commented Mar 2, 2017

@romani
It loads _zh_CN.properties
I do a copy paste and change 异常 to 异常二 in _zh_CN file. I observed LocalizedMessage load the _zh_CN bundle and the error message is also contains 异常二:

Failed tests:
  CheckerTest.testHaltOnExceptionOff:778->BaseCheckTestSupport.verify:231->BaseCheckTestSupport.verify:262 error message 0 expected:<...e/InputMain.java:0: [Got an exception] - java.lang.IndexOu...> but was:<...e/InputMain.java:0: [异常二] - java.lang.IndexOu...>

I don't think the zh CI should pass.

final String[] expected = {
            "0: Got an exception - java.lang.IndexOutOfBoundsException: test",
        };

        verify(checker, filePath, filePath, expected);

The expected error message is hard coded Got an exception. When the locale is non-English, obviously the error message won't be same.
---
mvn clean integration-test failsafe:verify -DargLine='-Duser.language=en -Duser.country=US' doesn't fail in my environment.
mvn clean integration-test failsafe:verify -DargLine='-Duser.language=zh -Duser.country=CN' or mvn clean verify does fail.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Mar 2, 2017

Member

CheckerTest.testHaltOnExceptionOff:778->BaseCheckTestSupport.verify:231->BaseCheckTestSupport.verify:262 error message 0 expected:<...e/InputMain.java:0: [Got an exception] - java.lang.IndexOu...> but was:<...e/InputMain.java:0: [异常二] - java.lang.IndexOu...>

The expected error message is hard coded Got an exception. When the locale is non-English, obviously the error message won't be same.

Test failure could be related to Issue #3377.

Member

rnveach commented Mar 2, 2017

CheckerTest.testHaltOnExceptionOff:778->BaseCheckTestSupport.verify:231->BaseCheckTestSupport.verify:262 error message 0 expected:<...e/InputMain.java:0: [Got an exception] - java.lang.IndexOu...> but was:<...e/InputMain.java:0: [异常二] - java.lang.IndexOu...>

The expected error message is hard coded Got an exception. When the locale is non-English, obviously the error message won't be same.

Test failure could be related to Issue #3377.

@Luolc

This comment has been minimized.

Show comment
Hide comment
@Luolc

Luolc Mar 3, 2017

Contributor

I find the reason eventually.

Firstly, a way to reproduce the bug is mentioned in Issue #3377 . As MEZk suggested, I add a <argLine>-Duser.country=DE -Duser.language=de</argLine> in my pom.xml and reproduced the bug with German error message. So I guess someone else with English environment can also reproduce the bug by this way.

The different behavior between Intellij test and command line mvn clean verify is not produced by the different classpath of these two situation. The reason is that I run only single method test in Intellij but the in command line it will run all tests.
To prove that, run mvn clean -Dtest=CheckerTest#testHaltOnExceptionOff verify and test pass.
And run test of CheckTest class but not testHaltOnException alone in Intellij, test fail.

So the bug is due to the cache of resource bundle. https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/api/LocalizedMessage.java#L301-L305
The bundle won't be reloaded once it is cached, even if the sLocale changes. It doesn't make sense. i.e. if we change the locale from en_US to zh_CN, the message will still be shown in English. Obviously it is not what users want.

And it is funny that if I add

final Locale locale = Locale.ROOT;
checker.setLocaleLanguage(locale.getLanguage());
checker.setLocaleCountry(locale.getCountry());

to the first method of CheckTest, test of testHaltOnException will pass.
It is definitely unfriendly to developers. We shouldn't have to consider any other test methods when writing a new one. Another thing is all the locale setters (like lines above) in the test methods except the first one will do no effort. It will mislead the developers.

I add a line to clear the BUNDLE_CACHE when setting a different locale at https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/api/LocalizedMessage.java#L363
Then CheckerTest pass but it comes out two similar bugs at https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/checks/header/HeaderCheckTest.java.

I think we need to handle the cache problem and then fix the bugs in HeaderCheckTest.

@rnveach @romani
What's your option?

Contributor

Luolc commented Mar 3, 2017

I find the reason eventually.

Firstly, a way to reproduce the bug is mentioned in Issue #3377 . As MEZk suggested, I add a <argLine>-Duser.country=DE -Duser.language=de</argLine> in my pom.xml and reproduced the bug with German error message. So I guess someone else with English environment can also reproduce the bug by this way.

The different behavior between Intellij test and command line mvn clean verify is not produced by the different classpath of these two situation. The reason is that I run only single method test in Intellij but the in command line it will run all tests.
To prove that, run mvn clean -Dtest=CheckerTest#testHaltOnExceptionOff verify and test pass.
And run test of CheckTest class but not testHaltOnException alone in Intellij, test fail.

So the bug is due to the cache of resource bundle. https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/api/LocalizedMessage.java#L301-L305
The bundle won't be reloaded once it is cached, even if the sLocale changes. It doesn't make sense. i.e. if we change the locale from en_US to zh_CN, the message will still be shown in English. Obviously it is not what users want.

And it is funny that if I add

final Locale locale = Locale.ROOT;
checker.setLocaleLanguage(locale.getLanguage());
checker.setLocaleCountry(locale.getCountry());

to the first method of CheckTest, test of testHaltOnException will pass.
It is definitely unfriendly to developers. We shouldn't have to consider any other test methods when writing a new one. Another thing is all the locale setters (like lines above) in the test methods except the first one will do no effort. It will mislead the developers.

I add a line to clear the BUNDLE_CACHE when setting a different locale at https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/api/LocalizedMessage.java#L363
Then CheckerTest pass but it comes out two similar bugs at https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/checks/header/HeaderCheckTest.java.

I think we need to handle the cache problem and then fix the bugs in HeaderCheckTest.

@rnveach @romani
What's your option?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Mar 3, 2017

Member

So the bug is due to the cache of resource bundle. https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/api/LocalizedMessage.java#L301-L305

This change looks valid to me. The cache is built upon this value, so if it changes, then so should the cache. IMO, I see no problem erasing the cache if the locale changes.

https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/api/LocalizedMessage.java#L302-L304
Looking at the code, it is also dependent on the ClassLoader which comes from the sourceClass. I'm not sure how possible it is the class loader would change between tests or if we have any that use different ones, but the sourceClass could change every instance.

Should we watch out for class loader changes or just ignore this as we are dropping class loader from Checker eventually and going with the default.

Member

rnveach commented Mar 3, 2017

So the bug is due to the cache of resource bundle. https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/api/LocalizedMessage.java#L301-L305

This change looks valid to me. The cache is built upon this value, so if it changes, then so should the cache. IMO, I see no problem erasing the cache if the locale changes.

https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/api/LocalizedMessage.java#L302-L304
Looking at the code, it is also dependent on the ClassLoader which comes from the sourceClass. I'm not sure how possible it is the class loader would change between tests or if we have any that use different ones, but the sourceClass could change every instance.

Should we watch out for class loader changes or just ignore this as we are dropping class loader from Checker eventually and going with the default.

@Luolc

This comment has been minimized.

Show comment
Hide comment
@Luolc

Luolc Mar 6, 2017

Contributor

@rnveach
I think the change of sourceClass won't be a problem. It is not a static field. There are no impact between test methods if we create new instance for each one.
I want to focus on bundle cache currently. We can consider the sourceClass if we find anything else about it should be changed later.

I am going to work on it :)

Contributor

Luolc commented Mar 6, 2017

@rnveach
I think the change of sourceClass won't be a problem. It is not a static field. There are no impact between test methods if we create new instance for each one.
I want to focus on bundle cache currently. We can consider the sourceClass if we find anything else about it should be changed later.

I am going to work on it :)

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 6, 2017

Member

My concern is CI validation, we need to figure out how to reproduce this problem on CI, after that make sure that this fix works. CI should help in future to catch localization problems. Non of us can write completely correct code in localization perspective.

Member

romani commented Mar 6, 2017

My concern is CI validation, we need to figure out how to reproduce this problem on CI, after that make sure that this fix works. CI should help in future to catch localization problems. Non of us can write completely correct code in localization perspective.

@Luolc

This comment has been minimized.

Show comment
Hide comment
@Luolc

Luolc Mar 6, 2017

Contributor

@romani
Agree. Maybe we need a new issue for the CI problem. This issue describes a specific bug while the reproducibility on CI could be more complicated.

Contributor

Luolc commented Mar 6, 2017

@romani
Agree. Maybe we need a new issue for the CI problem. This issue describes a specific bug while the reproducibility on CI could be more complicated.

Luolc added a commit to Luolc/checkstyle that referenced this issue Mar 7, 2017

rnveach added a commit that referenced this issue Mar 13, 2017

@rnveach rnveach added the bug label Mar 13, 2017

@rnveach rnveach added this to the 7.7 milestone Mar 13, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Mar 13, 2017

Member

A fix was merged, but the issue is not done yet.
As suggested at #3942 (review) , We need to remove all ROOT locales from our tests as they force ignoring localization testing.

@Luolc Since you started this, do you plan to finish?

Member

rnveach commented Mar 13, 2017

A fix was merged, but the issue is not done yet.
As suggested at #3942 (review) , We need to remove all ROOT locales from our tests as they force ignoring localization testing.

@Luolc Since you started this, do you plan to finish?

@Luolc

This comment has been minimized.

Show comment
Hide comment
@Luolc

Luolc Mar 13, 2017

Contributor

@rnveach Glad to do so 😄 Shall I open new a PR linked to this issue as well when I finished?

Contributor

Luolc commented Mar 13, 2017

@rnveach Glad to do so 😄 Shall I open new a PR linked to this issue as well when I finished?

@rnveach

This comment has been minimized.

Show comment
Hide comment
Member

rnveach commented Mar 13, 2017

@Luolc yes.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Mar 13, 2017

Member

lets close this issue, as it is merged fix completely cover it.
Lets have more exact issue - #3989.

Member

romani commented Mar 13, 2017

lets close this issue, as it is merged fix completely cover it.
Lets have more exact issue - #3989.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment