On Tue, 9 Sep 2025 11:19:06 GMT, Matthew Donovan <[email protected]> wrote:
>> Mikhail Yankelevich has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Matthew's comments
>
> test/jdk/sun/security/pkcs11/Config/ReadConfInUTF16Env.java line 51:
>
>> 49: Provider p = Security.getProvider("SunPKCS11");
>> 50: if (p == null) {
>> 51: throw new SkippedException("Skipping test - no PKCS11
>> provider available");
>
> minor wording nit: you don't need to say "skipping test" in the message of a
> SkippedException.
Done in the next commit
> test/jdk/sun/security/pkcs11/PKCS11Test.java line 784:
>
>> 782: private void premain(Provider p) throws Exception {
>> 783: if (skipTest(p)) {
>> 784: throw new SkippedException("Test is skipped due to
>> skipTest() result, please see the log");
>
> maybe the message could just say "See logs for details"? Ideally, I think
> we'd just refactor `skipTest()` into something like `checkSkippedTest()` and
> the method itself threw the SkippedException but that would require a lot of
> refactoring.
I agree, this can just say "See logs for details", should be done in the next
commit.
Refactoring skipTest will cause a change in classes that have methods
overwriting this one. I'm not convinced that such a large change would be
feasible given that overwritten methods log the errors anyway.
> test/jdk/sun/security/pkcs11/ec/ReadCertificates.java line 82:
>
>> 80: public void main(Provider p) throws Exception {
>> 81: if (p.getService("Signature", "SHA1withECDSA") == null) {
>> 82: throw new SkippedException("Provider does not support ECDSA,
>> skipping...");
>
> here (and the other ec/, rsa/, sslecc/ tests), you could remove the
> "skipping..." from the message.
Done in the next commit
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27166#discussion_r2333459439
PR Review Comment: https://git.openjdk.org/jdk/pull/27166#discussion_r2333452857
PR Review Comment: https://git.openjdk.org/jdk/pull/27166#discussion_r2333458924
