On Mon, 21 Feb 2022 14:09:50 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
> Can I please get a review of this test only change which fixes the issue > noted in https://bugs.openjdk.java.net/browse/JDK-8282023? > > As noted in that JBS issue these tests fail when the default locale under > which those tests are run isn't `en_US`. Both these tests were introduced as > part of https://bugs.openjdk.java.net/browse/JDK-8231640. At a high level, > these test do the following: > - Use Properties.store(...) APIs to write out a properties file. This > internally ends up writing a date comment via a call to > `java.util.Date.toString()` method. > - The test then runs a few checks to make sure that the written out `Date` is > an expected one. To run these checks it uses the > `java.time.format.DateTimeFormatter` to parse the date comment out of the > properties file. > > All this works fine when the default locale is (for example) `en_US`. > However, when the default locale is (for example `en_CA` or even `hi_IN`) the > tests fail with an exception in the latter step above while parsing the date > comment using the `DateTimeFormatter` instance. The exception looks like: > > > Using locale: he for Properties#store(OutputStream) test > test PropertiesStoreTest.testStoreOutputStreamDateComment(he): failure > java.lang.AssertionError: Unexpected date comment: Mon Feb 21 19:10:31 IST > 2022 > at org.testng.Assert.fail(Assert.java:87) > at PropertiesStoreTest.testDateComment(PropertiesStoreTest.java:255) > at > PropertiesStoreTest.testStoreOutputStreamDateComment(PropertiesStoreTest.java:223) > at > java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) > at java.base/java.lang.reflect.Method.invoke(Method.java:577) > at > org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132) > at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:599) > at > org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174) > at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46) > at > org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:822) > at > org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147) > at > org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146) > at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128) > at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) > at org.testng.TestRunner.privateRun(TestRunner.java:764) > at org.testng.TestRunner.run(TestRunner.java:585) > at org.testng.SuiteRunner.runTest(SuiteRunner.java:384) > at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:378) > at org.testng.SuiteRunner.privateRun(SuiteRunner.java:337) > at org.testng.SuiteRunner.run(SuiteRunner.java:286) > at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:53) > at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:96) > at org.testng.TestNG.runSuitesSequentially(TestNG.java:1218) > at org.testng.TestNG.runSuitesLocally(TestNG.java:1140) > at org.testng.TestNG.runSuites(TestNG.java:1069) > at org.testng.TestNG.run(TestNG.java:1037) > at > com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:94) > at > com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:54) > at > java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) > at java.base/java.lang.reflect.Method.invoke(Method.java:577) > at > com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) > at java.base/java.lang.Thread.run(Thread.java:828) > Caused by: java.time.format.DateTimeParseException: Text 'Mon Feb 21 19:10:31 > IST 2022' could not be parsed at index 0 > at > java.base/java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:2106) > at > java.base/java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1934) > at PropertiesStoreTest.testDateComment(PropertiesStoreTest.java:253) > ... 30 more > > (in the exception above a locale with language `he` is being used) > > The root cause of this failure lies (only) within the tests - the > `DateTimeFormatter` used in the tests is locale sensitive and uses the > current (`Locale.getDefault()`) locale by default for parsing the date text. > This parsing fails because, although `Date.toString()` javadoc states nothing > about locales, it does mention the exact characters that will be used to > write out the date comment. In other words, this date comment written out is > locale insensitive and as such when parsing using `DateTimeFormatter` a > neutral locale is appropriate on the `DateTimeFormatter` instance. This PR > thus changes the tests to use `Locale.ROOT` while parsing this date comment. > Additionally, while I was at it, I updated the `PropertiesStoreTest` to > automate the use of multiple different locales to reproduce this issue > (automatically) and verify the fix. Looks good overall. Some nits follow. Please change the copyright year too. test/jdk/java/util/Properties/PropertiesStoreTest.java line 50: > 48: * @test > 49: * @summary tests the order in which the Properties.store() method writes > out the properties > 50: * @bug 8231640 Add the bug id, also `@modules jdk.localedata` needs to be added. test/jdk/java/util/Properties/PropertiesStoreTest.java line 104: > 102: for (Locale locale : Locale.getAvailableLocales()) { > 103: // skip ROOT locale and ENGLISH language ones > 104: if (!locale.getLanguage().isEmpty() && > !locale.getLanguage().equals(Locale.ENGLISH.getLanguage())) { Could simply compare with "en". test/jdk/java/util/Properties/PropertiesStoreTest.java line 189: > 187: @Test(dataProvider = "localeProvider") > 188: public void testStoreWriterDateComment(final Locale testLocale) > throws Exception { > 189: var prevLocale = Locale.getDefault(); Could be done once at the class loading time. test/jdk/java/util/Properties/PropertiesStoreTest.java line 212: > 210: @Test(dataProvider = "localeProvider") > 211: public void testStoreOutputStreamDateComment(final Locale > testLocale) throws Exception { > 212: var prevLocale = Locale.getDefault(); Same as the above method test/jdk/java/util/Properties/PropertiesStoreTest.java line 253: > 251: // use a neutral locale for parsing, since when the date > comment was written by Properties.store(...), > 252: // it internally calls the Date.toString() which always > writes in a locale insensitive manner > 253: > DateTimeFormatter.ofPattern(DATE_FORMAT_PATTERN).withLocale(Locale.ROOT).parse(comment); This formatter can also be statically initialized. test/jdk/java/util/Properties/StoreReproducibilityTest.java line 430: > 428: // use a neutral locale for parsing, since when the date > comment was written by Properties.store(...), > 429: // it internally calls the Date.toString() which always > writes in a locale insensitive manner > 430: var df = > DateTimeFormatter.ofPattern(DATE_FORMAT_PATTERN).withLocale(Locale.ROOT); Same as the above. ------------- PR: https://git.openjdk.java.net/jdk/pull/7558
