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

8272358: Some tests may fail when executed with other locales than the US #5098

Closed
wants to merge 2 commits into from

Conversation

mrserb
Copy link
Member

@mrserb mrserb commented Aug 12, 2021

As mentioned in the bug report this issue was reported a few times, I have checked all tests in tier1/tier2/tier3 and found these four tests which fail because of non-US locale. The common issue is that the output depends on the locale(ex: 3.14 VS 3,14).


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8272358: Some tests may fail when executed with other locales than the US

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5098/head:pull/5098
$ git checkout pull/5098

Update a local copy of the PR:
$ git checkout pull/5098
$ git pull https://git.openjdk.java.net/jdk pull/5098/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5098

View PR using the GUI difftool:
$ git pr show -t 5098

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5098.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 12, 2021

👋 Welcome back serb! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Aug 12, 2021

@mrserb The following labels will be automatically applied to this pull request:

  • compiler
  • kulla

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added compiler kulla labels Aug 12, 2021
@mrserb mrserb marked this pull request as ready for review Aug 12, 2021
@openjdk openjdk bot added the rfr label Aug 12, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 12, 2021

Webrevs

@mrserb
Copy link
Member Author

@mrserb mrserb commented Aug 31, 2021

Any volunteers to take a look at the tests update?

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Sep 25, 2021

There are still 1500+ failures due to non-US locale after this patch.

So my question is: does it make sense to fix only 4 of them?
Thanks.

@mrserb
Copy link
Member Author

@mrserb mrserb commented Sep 25, 2021

These are the only tests reported in Fr and Ru locales in t1/t2/t3. Are you sure that 1500 failures are caused by the tests issues and not a problem in JDK?

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Sep 25, 2021

These are the only tests reported in Fr and Ru locales in t1/t2/t3. Are you sure that 1500 failures are caused by the tests issues and not a problem in JDK?

If I test with JTREG="VM_OPTIONS=-Duser.language=en -Duser.country=US", most of the failures would disappear.

make test TEST="tier1 tier2 tier3" with your patch

==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR
>> jtreg:test/hotspot/jtreg:tier1                     1810  1809     1     0 <<
>> jtreg:test/jdk:tier1                               2069  2065     4     0 <<
>> jtreg:test/langtools:tier1                         4238  2643  1593     2 <<
   jtreg:test/jaxp:tier1                                 0     0     0     0
   jtreg:test/lib-test:tier1                             0     0     0     0
   jtreg:test/hotspot/jtreg:tier2                      444   444     0     0
>> jtreg:test/jdk:tier2                               3765  3727    38     0 <<
>> jtreg:test/langtools:tier2                           12    10     2     0 <<
>> jtreg:test/jaxp:tier2                               452   449     3     0 <<
   jtreg:test/hotspot/jtreg:tier3                       85    85     0     0
   jtreg:test/jdk:tier3                                174   174     0     0
   jtreg:test/langtools:tier3                            0     0     0     0
   jtreg:test/jaxp:tier3                                 0     0     0     0
==============================
TEST FAILURE

make test JTREG="VM_OPTIONS=-Duser.language=en -Duser.country=US" TEST="tier1 tier2 tier3" with your patch

==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR
>> jtreg:test/hotspot/jtreg:tier1                     1810  1809     1     0 <<
   jtreg:test/jdk:tier1                               2069  2069     0     0
>> jtreg:test/langtools:tier1                         4238  4230     8     0 <<
   jtreg:test/jaxp:tier1                                 0     0     0     0
   jtreg:test/lib-test:tier1                             0     0     0     0
   jtreg:test/hotspot/jtreg:tier2                      444   444     0     0
>> jtreg:test/jdk:tier2                               3765  3747    18     0 <<
   jtreg:test/langtools:tier2                           12    12     0     0
>> jtreg:test/jaxp:tier2                               452   451     1     0 <<
   jtreg:test/hotspot/jtreg:tier3                       85    85     0     0   
   jtreg:test/jdk:tier3                                174   174     0     0
   jtreg:test/langtools:tier3                            0     0     0     0
   jtreg:test/jaxp:tier3                                 0     0     0     0
==============================
TEST FAILURE

@mrserb
Copy link
Member Author

@mrserb mrserb commented Sep 25, 2021

If I test with JTREG="VM_OPTIONS=-Duser.language=en -Duser.country=US", most of the failures would disappear.

If the failure disappeared if the "user.language=en" is used does not mean that this is not a JDK bug or some other bugs in the tests. What is the reason for the falure?

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Sep 25, 2021

If I test with JTREG="VM_OPTIONS=-Duser.language=en -Duser.country=US", most of the failures would disappear.

If the failure disappeared if the "user.language=en" is used does not mean that this is not a JDK bug or some other bugs in the tests. What is the reason for the falure?

An important reason is that the expected output doesn't match due to different languages.

 STDERR:
  stdout: [];
  stderr: [??: ?? emptynumbootstrapmethods1 ???? main ??, ?? main ?????:
    public static void main(String[] args) 
 ?? JavaFX ?????????javafx.application.Application
 ]
  exitValue = 1
 
 java.lang.RuntimeException: 'Main method not found in class emptynumbootstrapmethods1' missing from stdout/stderr 
 
        at jdk.test.lib.process.OutputAnalyzer.shouldContain(OutputAnalyzer.java:221)
        at TestEmptyBootstrapMethodsAttr.main(TestEmptyBootstrapMethodsAttr.java:56)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:568)
        at com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:312)
        at java.base/java.lang.Thread.run(Thread.java:833)
 
 JavaTest Message: Test threw exception: java.lang.RuntimeException
 JavaTest Message: shutting down test
 
 
 TEST RESULT: Failed. Execution failed: `main' threw exception: java.lang.RuntimeException: 'Main method not found in class emptynumbootstrapmethods1' missing from stdout/stderr

E.g., Test runtime/classFileParserBug/TestEmptyBootstrapMethodsAttr.java with the locale:

LANG=en_US.UTF-8 \
LC_ALL=C \

@mrserb
Copy link
Member Author

@mrserb mrserb commented Sep 25, 2021

There are still 1500+ failures due to non-US locale after this patch.

So my question is: does it make sense to fix only 4 of them?

This error means that your OpenJDK build contains the translation for your locale, and different tools use the resource bundles to translate different outputs. Looks like nobody validates that the translated output is correct for the various cases since the tests fail on non-US translated locales. I guess in the ideal world that tests should be updated to validate the output for each locale, or probably even better they should check that the output is correct for some specific "key" in the resource bundle.

The current issue is different, it fixes the tests depended on some specific float format and has no dependency on the resource bundles.

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Sep 25, 2021

There are still 1500+ failures due to non-US locale after this patch.
So my question is: does it make sense to fix only 4 of them?

This error means that your OpenJDK build contains the translation for your locale, and different tools use the resource bundles to translate different outputs. Looks like nobody validates that the translated output is correct for the various cases since the tests fail on non-US translated locales. I guess in the ideal world that tests should be updated to validate the output for each locale, or probably even better they should check that the output is correct for some specific "key" in the resource bundle.

The current issue is different, it fixes the tests depended on some specific float format and has no dependency on the resource bundles.

I still don't understand why the failures are different.

But the fix doesn't work even with these 4 tests.

  • Before this fix: make test TEST="..."
==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR
   jtreg:test/langtools/jdk/jshell/ToolBasicTest.java
>>                                                       1     0     1     0 <<
   jtreg:test/langtools/jdk/jshell/ToolSimpleTest.java
>>                                                       1     0     1     0 <<
   jtreg:test/langtools/tools/javac/lambda/lambdaExecution/LambdaTranslationTest1.java
                                                         1     1     0     0
   jtreg:test/langtools/tools/javac/lambda/lambdaExecution/LambdaTranslationTest2.java
                                                         1     1     0     0
==============================
TEST FAILURE
  • After this fix: make test TEST="..."
==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR
   jtreg:test/langtools/jdk/jshell/ToolBasicTest.java
>>                                                       1     0     1     0 <<
   jtreg:test/langtools/jdk/jshell/ToolSimpleTest.java
>>                                                       1     0     1     0 <<
   jtreg:test/langtools/tools/javac/lambda/lambdaExecution/LambdaTranslationTest1.java
                                                         1     1     0     0
   jtreg:test/langtools/tools/javac/lambda/lambdaExecution/LambdaTranslationTest2.java
                                                         1     1     0     0
==============================
TEST FAILURE

But if I run with make test JTREG="VM_OPTIONS=-Duser.language=en -Duser.country=US" TEST="...", all of them get passed.

@mrserb
Copy link
Member Author

@mrserb mrserb commented Sep 25, 2021

I still don't understand why the failures are different.

The difference is that if the test uses golden data for EN locale only while the tested tool like javac supports more than one locale for some specific output message then the test should check that specific localized message.

But the fix doesn't work even with these 4 tests.

Look like you use one of ja or zh_CN locales since only for these locales there are translations, the test should take it into account. It is not strictly correct to run add "-Duser.language=en -Duser.country=US" to it to fix such issues.

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Sep 25, 2021

I still don't understand why the failures are different.

The difference is that if the test uses golden data for EN locale only while the tested tool like javac supports more than one locale for some specific output message then the test should check that specific localized message.

But the fix doesn't work even with these 4 tests.

Look like you use one of ja or zh_CN locales since only for these locales there are translations, the test should take it into account. It is not strictly correct to run add "-Duser.language=en -Duser.country=US" to it to fix such issues.

Thanks @mrserb for your clarification.

Shall we also fix ToolBasicTest.java and ToolSimpleTest.java just as what is done for LambdaTranslationTest1.java and LambdaTranslationTest2.java?
If so, the 4 tests would be also fixed on our MacOS platforms.

And do you test the fix on Linux?
Thanks.

@mrserb
Copy link
Member Author

@mrserb mrserb commented Oct 1, 2021

Shall we also fix ToolBasicTest.java and ToolSimpleTest.java just as what is done for LambdaTranslationTest1.java and LambdaTranslationTest2.java? If so, the 4 tests would be also fixed on our MacOS platforms.

I do not want to add a global locale parameter to the places which may trigger loading of the different translations.
Probably we missed the bugs like this(not exactly but similar to) https://bugs.openjdk.java.net/browse/JDK-8274544 because everybody passes US locale to the tests. Probably we missed some other bugs as well.

And do you test the fix on Linux? Thanks.

On Linux I have checked "JA" and "RU" locales, nothing changed for "JA" it uses the same float format as the US, so the LambdaTranslationTest1/LambdaTranslationTest2 passed before/after the fix and the ToolBasicTest/ToolSimpleTest failed in the "JA" locale, one of the subtests compare the expected output saved for US locale with the real output of the test.

For RU locale the tests fail before the fix and passed after because there is no specific translation so the US text is used, and this PR fixes the problem in the float format.

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Oct 1, 2021

So this change doesn't fix the failure with Chinese and JA locales.

@mrserb
Copy link
Member Author

@mrserb mrserb commented Oct 1, 2021

So this change doesn't fix the failure with Chinese and JA locales.

Yes, I can rephrase it as "it fixes the locales which do not have translations".

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Oct 1, 2021

So this change doesn't fix the failure with Chinese and JA locales.

Yes, I can rephrase it as "it fixes the locales which do not have translations".

Then people with RU locale should look at this fix.

@mrserb
Copy link
Member Author

@mrserb mrserb commented Oct 1, 2021

@aivanov-jdk please take a look

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 29, 2021

@mrserb This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@mrserb
Copy link
Member Author

@mrserb mrserb commented Oct 29, 2021

@aivanov-jdk Alexey do you have any comments or suggestions?

@aivanov-jdk
Copy link
Member

@aivanov-jdk aivanov-jdk commented Oct 29, 2021

@aivanov-jdk Alexey do you have any comments or suggestions?

Sorry for my delayed reply. I have never used Russian locale when working with JDK, if I remember correctly, I even experienced JDK build failures with Russian locale.

To me, the change looks good: it makes the tests more stable, these updated tests should pass even if the current locale is different than the US. I believe it's a common practice, as it's mentioned in Testing the JDK:

If your locale is non-US, some tests are likely to fail. To work around this you can set the locale to US.

However, I haven't tried running the tests under Russian or French locale yet.

As for translations, I believe the tests which specifically verify translations should also set the locale explicitly and check the expected output.

Copy link
Member

@aivanov-jdk aivanov-jdk left a comment

I can confirm that these fixes make these tests pass in Russian locale.

Two of the tests fail with timeout for me: ToolBasicTest.java and ToolSimpleTest.java; the other two LambdaTranslationTest{1,2}.java fail without the fix and pass with the proposed fix applied. I'm inclined to trust that the first pair also passes successfully with the fix.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 3, 2021

@mrserb This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8272358: Some tests may fail when executed with other locales than the US

Reviewed-by: aivanov

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Nov 3, 2021
@mrserb
Copy link
Member Author

@mrserb mrserb commented Nov 17, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Nov 17, 2021

Going to push as commit 29e552c.
Since your change was applied there have been 19 commits pushed to the master branch:

  • ce4471f: 8277346: ProblemList 7 serviceability/sa tests on macosx-x64
  • 45a60db: 8277045: G1: Remove unnecessary set_concurrency call in G1ConcurrentMark::weak_refs_work
  • 6bb0462: 8277224: sun.security.pkcs.PKCS9Attributes.toString() throws NPE
  • d8c0280: 8277316: ciReplay: dump_replay_data is not thread-safe
  • 007ad7c: 8277303: Terminology mismatch between JLS17-3.9 and SE17's javax.lang.model.SourceVersion method specs
  • 8881f29: 8277310: ciReplay: @CPI MethodHandle references not resolved
  • 262d070: 8277246: Check for NonRepudiation as well when validating a TSA certificate
  • a907b2b: 8276177: nsk/jvmti/RedefineClasses/StressRedefineWithoutBytecodeCorruption failed with "assert(def_ik->is_being_redefined()) failed: should be being redefined to get here"
  • b687664: 8277159: Fix java/nio/file/FileStore/Basic.java test by ignoring /run/user/* mount points
  • 8f5a8f7: 8264293: Create implementation for NSAccessibilityMenu protocol peer
  • ... and 9 more: https://git.openjdk.java.net/jdk/compare/b0a463fa59a1c3c554f48267525729bf89a2c5be...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Nov 17, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 17, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 17, 2021

@mrserb Pushed as commit 29e552c.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@mrserb mrserb deleted the JDK-8272358 branch Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler integrated kulla
3 participants