Re: RFR: 8316540: StoreReproducibilityTest fails on some locales [v2]
On Thu, 21 Sep 2023 18:35:57 GMT, Naoto Sato wrote: > But yes, it is appropriate to replace the invocation. Replaced them all. Thanks - PR Review Comment: https://git.openjdk.org/jdk/pull/15829#discussion_r1333926851
Re: RFR: 8316540: StoreReproducibilityTest fails on some locales [v2]
On Thu, 21 Sep 2023 07:17:43 GMT, Alan Bateman wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Reflects review comments
>
> test/jdk/java/util/Properties/StoreReproducibilityTest.java line 137:
>
>> 135: final Path tmpFile = Files.createTempFile("8231640",
>> ".props");
>> 136: storedFiles.add(tmpFile);
>> 137: final ProcessBuilder processBuilder =
>> ProcessTools.createJavaProcessBuilder(
>
> ProcessTools.createJavaProcessBuilder came up in another PR because it
> doesn't prepend the VM and java opts. As all usages of PB are being updated
> in this test then it makes me wonder if it should be changed to use
> createTestJvm while you're there.
Actually, I was limiting the change to a single JVM invocation 😄
But yes, it is appropriate to replace the invocation. Replaced them all.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15829#discussion_r1333464550
Re: RFR: 8316540: StoreReproducibilityTest fails on some locales [v2]
On Wed, 20 Sep 2023 16:12:36 GMT, Naoto Sato wrote:
>> Fixing a test case that fails in some time zones. Making sure the test is
>> run in `UTC` zone will fix the issue. Confirmed the fix by manually setting
>> machine's time zone to Europe/Dublin.
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reflects review comments
test/jdk/java/util/Properties/StoreReproducibilityTest.java line 137:
> 135: final Path tmpFile = Files.createTempFile("8231640",
> ".props");
> 136: storedFiles.add(tmpFile);
> 137: final ProcessBuilder processBuilder =
> ProcessTools.createJavaProcessBuilder(
ProcessTools.createJavaProcessBuilder came up in another PR because it doesn't
prepend the VM and java opts. As all usages of PB are being updated in this
test then it makes me wonder if it should be changed to use createTestJvm while
you're there.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15829#discussion_r1332585761
Re: RFR: 8316540: StoreReproducibilityTest fails on some locales [v2]
On Wed, 20 Sep 2023 16:12:36 GMT, Naoto Sato wrote: >> Fixing a test case that fails in some time zones. Making sure the test is >> run in `UTC` zone will fix the issue. Confirmed the fix by manually setting >> machine's time zone to Europe/Dublin. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reflects review comments Thank you Naoto for the update. This looks good to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15829#pullrequestreview-1636736028
Re: RFR: 8316540: StoreReproducibilityTest fails on some locales [v2]
On Wed, 20 Sep 2023 16:12:36 GMT, Naoto Sato wrote: >> Fixing a test case that fails in some time zones. Making sure the test is >> run in `UTC` zone will fix the issue. Confirmed the fix by manually setting >> machine's time zone to Europe/Dublin. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reflects review comments LGTM - Marked as reviewed by jlu (Committer). PR Review: https://git.openjdk.org/jdk/pull/15829#pullrequestreview-1636650667
Re: RFR: 8316540: StoreReproducibilityTest fails on some locales [v2]
On Wed, 20 Sep 2023 16:12:36 GMT, Naoto Sato wrote: >> Fixing a test case that fails in some time zones. Making sure the test is >> run in `UTC` zone will fix the issue. Confirmed the fix by manually setting >> machine's time zone to Europe/Dublin. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reflects review comments Marked as reviewed by joehw (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15829#pullrequestreview-1636561895
Re: RFR: 8316540: StoreReproducibilityTest fails on some locales [v2]
On Wed, 20 Sep 2023 16:12:36 GMT, Naoto Sato wrote: >> Fixing a test case that fails in some time zones. Making sure the test is >> run in `UTC` zone will fix the issue. Confirmed the fix by manually setting >> machine's time zone to Europe/Dublin. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reflects review comments Thanks, Jai. Since `assertCurrentDate()` is apart from the actual JVM invoking test method `testEmptySysPropValue()`, I thought it would be safer to use UTC in all test cases so that if someone calls `assertCurrentDate()` in other test methods, the test wouldn't break. But you are right that they are not needed in other locations right now. I removed those locations and instead added some instructions in `assertCurrentDate()` for future proof. - PR Comment: https://git.openjdk.org/jdk/pull/15829#issuecomment-1728050059
Re: RFR: 8316540: StoreReproducibilityTest fails on some locales [v2]
> Fixing a test case that fails in some time zones. Making sure the test is run > in `UTC` zone will fix the issue. Confirmed the fix by manually setting > machine's time zone to Europe/Dublin. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Reflects review comments - Changes: - all: https://git.openjdk.org/jdk/pull/15829/files - new: https://git.openjdk.org/jdk/pull/15829/files/429b2369..30b0af53 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15829&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15829&range=00-01 Stats: 12 lines in 1 file changed: 3 ins; 8 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15829.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15829/head:pull/15829 PR: https://git.openjdk.org/jdk/pull/15829
