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

Java 12 functional sanity failure JCL_Test test_toLowerCase_lithuanian #4659

Closed
pshipton opened this issue Feb 7, 2019 · 7 comments
Closed

Comments

@pshipton
Copy link
Member

pshipton commented Feb 7, 2019

I ran a Java 12 functional sanity test using the changes from #3980 and #4531.

https://ci.eclipse.org/openj9/job/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/3
JCL_Test_0 JCL_Test_1

FAILED: test_toLowerCase_lithuanian
java.lang.AssertionError: unexpected I conversion for: 9fe : " 0x69 0x307 0x9fe"
	at org.testng.AssertJUnit.fail(AssertJUnit.java:59) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/TestConfig/lib/testng.jar)
	at org.testng.AssertJUnit.assertTrue(AssertJUnit.java:24) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/TestConfig/lib/testng.jar)
	at org.openj9.test.java.lang.Test_String.test_toLowerCase_lithuanian(Test_String.java:1356) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/functional/Java8andUp/GeneralTest.jar)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) from jrt:/java.base
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) from jrt:/java.base
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) from jrt:/java.base
	at java.base/java.lang.reflect.Method.invoke(Method.java:567) from jrt:/java.base
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:124) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/TestConfig/lib/testng.jar)
	at org.testng.internal.Invoker.invokeMethod(Invoker.java:580) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/TestConfig/lib/testng.jar)
	at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:716) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/TestConfig/lib/testng.jar)
	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:988) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/TestConfig/lib/testng.jar)
	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:125) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/TestConfig/lib/testng.jar)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:109) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/TestConfig/lib/testng.jar)
	at org.testng.TestRunner.privateRun(TestRunner.java:648) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/TestConfig/lib/testng.jar)
	at org.testng.TestRunner.run(TestRunner.java:505) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/TestConfig/lib/testng.jar)
	at org.testng.SuiteRunner.runTest(SuiteRunner.java:455) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/TestConfig/lib/testng.jar)
	at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:450) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/TestConfig/lib/testng.jar)
	at org.testng.SuiteRunner.privateRun(SuiteRunner.java:415) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/TestConfig/lib/testng.jar)
	at org.testng.SuiteRunner.run(SuiteRunner.java:364) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/TestConfig/lib/testng.jar)
	at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/TestConfig/lib/testng.jar)
	at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:84) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/TestConfig/lib/testng.jar)
	at org.testng.TestNG.runSuitesSequentially(TestNG.java:1208) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/TestConfig/lib/testng.jar)
	at org.testng.TestNG.runSuitesLocally(TestNG.java:1137) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/TestConfig/lib/testng.jar)
	at org.testng.TestNG.runSuites(TestNG.java:1049) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/TestConfig/lib/testng.jar)
	at org.testng.TestNG.run(TestNG.java:1017) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/TestConfig/lib/testng.jar)
	at org.testng.TestNG.privateMain(TestNG.java:1354) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/TestConfig/lib/testng.jar)
	at org.testng.TestNG.main(TestNG.java:1323) from jdk.internal.loader.ClassLoaders$AppClassLoader@e635e82(file:/home/jenkins/jenkins-agent/workspace/Test-sanity.functional-JDK12-linux_390-64_cmprssptrs/jvmtest/TestConfig/lib/testng.jar)
@pshipton
Copy link
Member Author

pshipton commented Feb 7, 2019

@fjeremic

@pshipton
Copy link
Member Author

pshipton commented Feb 7, 2019

@fjeremic
Copy link
Contributor

fjeremic commented Feb 8, 2019

@pshipton given it's Lithuanian the JIT won't do any intrinsics here so this will likely fail with -Xint as well. The likely cause here is same as in #2895. Does Java 12 use an updated Unicode standard? In which case we'll need to update the combining above and below arrays similarly to what was done in #2895.

On another thought it seems that we already have coverage for toUpperCase and toLowerCase testing for all locales in [1]. I do believe the same test in [1] which runs at Adopt should provide coverage for Lithuanian. Given that since Java 9 onwards we use the OpenJDK toUpperCase and toLowerCase implementations it would make sense if we use their testing as well. We would not have to maintain these Lithuanian tests ourselves then.

Do any of you (@DanHeidinga @pshipton @theresa-m @JasonFengJ9) have any objections to simply removing the test given the above justification?

[1] https://github.com/ibmruntimes/openj9-openjdk-jdk11/blob/afa710e1295efc46b0284835a7a1643f9cebc702/test/jdk/java/lang/String/UnicodeCasingTest.java#L169-L202

@fjeremic fjeremic self-assigned this Feb 8, 2019
@pshipton
Copy link
Member Author

pshipton commented Feb 8, 2019

Java 12 moved to Unicode 11.0.0 from 10.0.0.

@fjeremic are you ok with not having any PR testing for toUpper/LowerCase? Does the JIT affect these results? I'm thinking we should just remove the Lithuanian sub-test for now.

I'm concerned about the timing of anybody noticing one of these failing tests at Adopt. Getting these tests into an "always green" state is WIP. Until we reach that goal I'm not sure how long it would take before a failure would be noticed and brought to our attention.
@smlambert @ben-walsh

@smlambert
Copy link
Contributor

https://ci.adoptopenjdk.net/view/Test_openjdk/job/openjdk11_j9_openjdktest_x86-64_linux/277/testReport/java_lang_Character_UnicodeCasingTest/java/UnicodeCasingTest/ is passing for jdk11, and we have not turned on jdk12 builds yet at Adopt so have not seen the UnicodeCasingTest fail yet.

At the moment, all sanity.openjdk tests (3,262 tests, which include the UnicodeCasingTest) are passing. If we turn on jdk12 and have only a handful of failures to triage, it would be reported fairly quickly. Unknown amount of time if there are many failures to triage initially.

@fjeremic
Copy link
Contributor

fjeremic commented Feb 8, 2019

@fjeremic are you ok with not having any PR testing for toUpper/LowerCase? Does the JIT affect these results? I'm thinking we should just remove the Lithuanian sub-test for now.

@pshipton we already added unit tests for the JIT intrinsics as part of #2098. See [1] for the tests that were added. The tests in Test_String.java are more so for the -Xint implementation. I agree we should remove the Lithuanian sub-tests as those are the special ones we're having to maintain, unnecessarily so.

[1] https://github.com/eclipse/openj9/blob/9b955472ce5516212faff92f9d40d8232ce9dbbd/test/functional/Java8andUp/src/org/openj9/test/com/ibm/jit/Test_JITHelpers.java#L110-L334

and we have not turned on jdk12 builds yet at Adopt so have not seen the UnicodeCasingTest fail yet.

@smlambert they won't fail at Adopt because presumably the OpenJDK tests have been updated to the new Unicode 11.0.0 standard and because our String implementation uses OpenJDK toUpperCase and toLowerCase implementations everything should just work.

It's just the fact that we have our own special Lithuanian tests (in OpenJ9) that we're having to maintain at each JDK level which updates which Unicode standard we use. We should just remove our special testing and let OpenJDK test it for us for free.

fjeremic added a commit to fjeremic/openj9 that referenced this issue Feb 8, 2019
AdoptOpenJDK testing already has coverage for all locales and the
various special cases, including Lithuanian. Since Java 9 onward we
have started to use the OpenJDK implementations for `toUpperCase` and
`toLowerCase`. As such we will pick up updated test material from Adopt
whenever the Unicode version changes in future Java releases and we
will not have to continuously update these Lithuanian tests.

See the discussion in eclipse-openj9#4659 for further details and examples of tests.

Fixes: eclipse-openj9#4659

Signed-off-by: Filip Jeremic <[email protected]>
@smlambert
Copy link
Contributor

Yes, I understand, thanks. I was just reporting that we have no jdk12 builds yet and that the tests will run nightly (as they are part of sanity.openjdk) when we do.

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

No branches or pull requests

3 participants