Re: RFR: 8269373: some tests in jdk/tools/launcher/ fails on localized Windows platform [v2]
On Wed, 4 Aug 2021 07:25:06 GMT, Masanori Yano wrote: >> Hi all, >> >> Could you please review the 8269373 bug fixes? >> >> These tests call java.lang.ProcessBuilder in direct, so not used jtreg >> command option. To run non-localized tests, -Duser.language=en and >> -Duser.country=US options should be added in ProcessBuilder. > > Masanori Yano has updated the pull request incrementally with one additional > commit since the last revision: > > 8269373: use test opts for process arguments I think implicitly expecting locales to be set to en-US by specifying `test.vm.opts` is fragile which would introduce test instability. In fact, looking at some of the tests, e.g., `HelpFlagsTest` at line 332, the intention is to silently exit in case it is not English. I think it is what the test is intended and it is a bug if it fails with other locales. - PR: https://git.openjdk.java.net/jdk/pull/4594
Re: RFR: 8269373: some tests in jdk/tools/launcher/ fails on localized Windows platform [v2]
On Wed, 4 Aug 2021 07:25:06 GMT, Masanori Yano wrote: >> Hi all, >> >> Could you please review the 8269373 bug fixes? >> >> These tests call java.lang.ProcessBuilder in direct, so not used jtreg >> command option. To run non-localized tests, -Duser.language=en and >> -Duser.country=US options should be added in ProcessBuilder. > > Masanori Yano has updated the pull request incrementally with one additional > commit since the last revision: > > 8269373: use test opts for process arguments I think the current tests force the US locale, and false positives are test failures in non-US locale environments. This fix does not change the test results in the US locale, but allows it to work in non-US locale environments. I can't think of a false positive problem with this fix, but what specific cases do you think are the problems? - PR: https://git.openjdk.java.net/jdk/pull/4594
Re: RFR: 8269373: some tests in jdk/tools/launcher/ fails on localized Windows platform [v2]
On Wed, 4 Aug 2021 07:25:06 GMT, Masanori Yano wrote: >> Hi all, >> >> Could you please review the 8269373 bug fixes? >> >> These tests call java.lang.ProcessBuilder in direct, so not used jtreg >> command option. To run non-localized tests, -Duser.language=en and >> -Duser.country=US options should be added in ProcessBuilder. > > Masanori Yano has updated the pull request incrementally with one additional > commit since the last revision: > > 8269373: use test opts for process arguments IMHO, this kind of test that sets the locale forcefully sometimes gives us false positives. I'd rather eliminate that possibility than run tests in different locales - PR: https://git.openjdk.java.net/jdk/pull/4594
Re: RFR: 8269373: some tests in jdk/tools/launcher/ fails on localized Windows platform [v2]
On Wed, 4 Aug 2021 07:25:06 GMT, Masanori Yano wrote: >> Hi all, >> >> Could you please review the 8269373 bug fixes? >> >> These tests call java.lang.ProcessBuilder in direct, so not used jtreg >> command option. To run non-localized tests, -Duser.language=en and >> -Duser.country=US options should be added in ProcessBuilder. > > Masanori Yano has updated the pull request incrementally with one additional > commit since the last revision: > > 8269373: use test opts for process arguments It is true that some other tests say Passed if the locale is not US, but I rather think that is not a good way. I think we should run tests as much as possible to ensure quality even in non-US environments. - PR: https://git.openjdk.java.net/jdk/pull/4594
Re: RFR: 8269373: some tests in jdk/tools/launcher/ fails on localized Windows platform [v2]
On Wed, 4 Aug 2021 07:25:06 GMT, Masanori Yano wrote: >> Hi all, >> >> Could you please review the 8269373 bug fixes? >> >> These tests call java.lang.ProcessBuilder in direct, so not used jtreg >> command option. To run non-localized tests, -Duser.language=en and >> -Duser.country=US options should be added in ProcessBuilder. > > Masanori Yano has updated the pull request incrementally with one additional > commit since the last revision: > > 8269373: use test opts for process arguments Then I would think the better fix would be to run the test if the default locale is `Locale.US`, otherwise the test should exit gracefully. - PR: https://git.openjdk.java.net/jdk/pull/4594
Re: RFR: 8269373: some tests in jdk/tools/launcher/ fails on localized Windows platform [v2]
On Wed, 4 Aug 2021 07:25:06 GMT, Masanori Yano wrote: >> Hi all, >> >> Could you please review the 8269373 bug fixes? >> >> These tests call java.lang.ProcessBuilder in direct, so not used jtreg >> command option. To run non-localized tests, -Duser.language=en and >> -Duser.country=US options should be added in ProcessBuilder. > > Masanori Yano has updated the pull request incrementally with one additional > commit since the last revision: > > 8269373: use test opts for process arguments Sorry for late reply. These tests compare the output of running the jar, and the correct answers to the output results are written in English. When these are run on localized Windows platform, the output will be in the local language and the comparison result will be false. So, these need to add -Duser.language=en and -Duser.country=US to the execution option of the jar. And I fixed to avoid hardcoding by using "test.vm.opts". If we want to run these tests on localized Windows platform, we need to add -Duser.language=en and -Duser.country=US as jtreg command options. - PR: https://git.openjdk.java.net/jdk/pull/4594
Re: RFR: 8269373: some tests in jdk/tools/launcher/ fails on localized Windows platform [v2]
> Hi all, > > Could you please review the 8269373 bug fixes? > > These tests call java.lang.ProcessBuilder in direct, so not used jtreg > command option. To run non-localized tests, -Duser.language=en and > -Duser.country=US options should be added in ProcessBuilder. Masanori Yano has updated the pull request incrementally with one additional commit since the last revision: 8269373: use test opts for process arguments - Changes: - all: https://git.openjdk.java.net/jdk/pull/4594/files - new: https://git.openjdk.java.net/jdk/pull/4594/files/e5546ded..99925f72 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4594&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4594&range=00-01 Stats: 14 lines in 6 files changed: 4 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/4594.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4594/head:pull/4594 PR: https://git.openjdk.java.net/jdk/pull/4594
