On Tue, 26 Apr 2022 21:17:34 GMT, Naoto Sato <na...@openjdk.org> wrote:
>> Ichiroh Takiguchi has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8285517: System.getenv() returns unexpected value if environment variable
>> has non ASCII character
>
> Thanks for fixing the issue.
> As to the test, it has lots of redundancy, and too complicated to me. Can you
> simplify the test?
Hello @naotoj and @RogerRiggs .
I appreciate your comments.
I applied the changes.
Additionally, I put bugid into test/jdk/java/lang/ProcessBuilder/Basic.java
> src/java.base/unix/classes/java/lang/ProcessImpl.java line 146:
>
>> 144: private static final LaunchMechanism launchMechanism =
>> platform.launchMechanism();
>> 145: private static final byte[] helperpath =
>> toCString(StaticProperty.javaHome() + "/lib/jspawnhelper");
>> 146: private static Charset jnuCharset = null;
>
> Can we simply call `jnuCharset()` here?
Change jnuCharset to "private static".
> test/jdk/java/lang/ProcessBuilder/Basic.java line 605:
>
>> 603: String fileEncoding = System.getProperty("file.encoding");
>> 604: Charset cs;
>> 605: if (nativeEncoding != null &&
>> !nativeEncoding.equals(fileEncoding)) {
>
> What's the reason for comparing `file.encoding`? Does
> `Charset.forName(nativeEncoding, Charset.defaultCharset())` suffice?
Remove file.encoding related code.
> test/jdk/java/lang/System/i18nEnvArg.java line 26:
>
>> 24: /*
>> 25: * @test
>> 26: * @bug 8285517
>
> If the test should work only on a particular env, should describe here and
> add `@requires` tag.
Add "@requires (os.family == "linux")"
> test/jdk/java/lang/System/i18nEnvArg.java line 50:
>
>> 48:
>> 49: public static void main(String[] args) throws Exception {
>> 50: ProcessBuilder pb = new ProcessBuilder(javeExe,
>
> Can utilize `jdk.test.lib.process.ProcessTools`.
Use ProcessTools class.
-------------
PR: https://git.openjdk.java.net/jdk/pull/8378
