On Thu, 6 Feb 2025 15:36:48 GMT, Fernando Guallini <fguall...@openjdk.org>
wrote:
>> Mikhail Yankelevich has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> changes to improve readability and removal of unneded System.out.print
>
> test/jdk/sun/security/util/Pem/PemEncoding.java line 47:
>
>> 45: File.separator,
>> 46: File.separator,
>> 47: File.separator);
>
> nit: the following may be easier to read
>
> Suggestion:
>
> final var certPath = Path.of(System.getProperty("test.src", "."))
> .getParent()
> .resolve("HostnameChecker")
> .resolve("cert5.crt");
agree, done in the next commit
> test/jdk/sun/security/util/Pem/PemEncoding.java line 58:
>
>> 56: final var subProcessOutput = result.getStdout();
>> 57: final var subProcessErr = result.getStderr();
>> 58: System.out.printf("Output:\n%s%n", subProcessOutput);
>
> No need to print the output here, JTreg automatically creates a .log file in
> the scratch directory with the subprocess stderr and stdout for debugging
removed all of the output analysis in the next commit, as the change you've
mentioned below will do this in a more neat way imo
> test/jdk/sun/security/util/Pem/PemEncoding.java line 72:
>
>> 70: // Some systems may automatically convert non-ASCII to ?
>> 71: // so this will detect the signature in all possible scenarios
>> 72: final var isUTF16Used = matcher.find() ||
>> subProcessOutput.contains("??: ?");
>
> IMO, this is confusing. **isUTF16Used** seems misleading, as it actually
> checks for non-ASCII characters in the output, and it does not confirm that
> UTF-16 was used.
>
> If the goal is to double-check the default file encoding in the subprocess,
> instead I would simply add a verification in PemEncodingTest:
>
>
> if (!"UTF-16".equals(Charset.defaultCharset().displayName())) {
> throw new RuntimeException("File encoding is not UTF-16");
> }
done in the next commit
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23492#discussion_r1945199033
PR Review Comment: https://git.openjdk.org/jdk/pull/23492#discussion_r1945199414
PR Review Comment: https://git.openjdk.org/jdk/pull/23492#discussion_r1945199205
