On Wed, 23 Feb 2022 01:08:21 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. > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - implement review comments > - copyright years test/jdk/java/util/Properties/PropertiesStoreTest.java line 61: > 59: private static final DateTimeFormatter formatter = > DateTimeFormatter.ofPattern(DATE_FORMAT_PATTERN) > 60: .withLocale(Locale.ROOT); > 61: private static final Locale prevLocale = Locale.getDefault(); By convention, static final field names are uppercase. It make the code using them easier to read. test/jdk/java/util/Properties/PropertiesStoreTest.java line 112: > 110: if (!locale.getLanguage().isEmpty() && > !locale.getLanguage().equals("en")) { > 111: nonEnglishLocale = locale; > 112: System.out.println("Selected non-english locale: " + > nonEnglishLocale + " for tests"); TestNG includes the arguments in the output when the test is run. test/jdk/java/util/Properties/PropertiesStoreTest.java line 116: > 114: } > 115: } > 116: if (nonEnglishLocale == null) { Alternatively, create a Set<Locale>, to avoid duplicates, adding the candidates as they are discovered and finally convert the Set to an Object[][]. It may be a bit easier to maintain. private static Object[][] provideLocales() { // pick a non-english locale for testing Set<Locale> locales = Arrays.stream(Locale.getAvailableLocales()) .filter(l -> !l.getLanguage().isEmpty() && !l.getLanguage().equals("en")) .limit(1) .collect(Collectors.toSet()); locales.add(Locale.getDefault()); locales.add(Locale.US); locales.add(Locale.ROOT); return locales.stream() .map(m -> new Locale[] {m}) .toArray(n -> new Object[n][0]); } ------------- PR: https://git.openjdk.java.net/jdk/pull/7558
