RFR: 8349532: Refactor ./util/Pem/encoding.sh to run in java [v2]
Mikhail Yankelevich
duke at openjdk.org
Thu Feb 6 18:11:31 UTC 2025
On Thu, 6 Feb 2025 15:36:48 GMT, Fernando Guallini <fguallini at 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
More information about the security-dev
mailing list