On Mon, 13 Apr 2026 15:57:45 GMT, Mikhail Yankelevich <[email protected]> wrote:
> * Improving the coverage of the `DomainKeyStore.java` > * converting existing tests to junit in order to make logs easier to read > * Further coverage for `DomainKeyStore.java` would be required, but should be > done as a separate [ticket](https://bugs.openjdk.org/browse/JDK-8382096) > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). Hopefully these comments are useful, if not please let me know. They are only suggestions; feel free to ignore them, I am not an OpenJDK member. test/jdk/sun/security/provider/KeyStore/DKSSystemPropertiesTest.java line 43: > 41: // Load and store entries in domain keystores > 42: > 43: public class DKSSystemPropertiesTest{ Is this missing space intended? Suggestion: public class DKSSystemPropertiesTest { test/jdk/sun/security/provider/KeyStore/DKSSystemPropertiesTest.java line 73: > 71: keystore.load( > 72: new DomainLoadStoreParameter(config, > 73: Collections.<String, > KeyStore.ProtectionParameter>emptyMap())); Are these type args needed? Suggestion: Collections.emptyMap())); test/jdk/sun/security/provider/KeyStore/DKSSystemPropertiesTest.java line 76: > 74: > 75: int expected; > 76: expected = keystore.size(); Simplify this? Suggestion: int expected = keystore.size(); test/jdk/sun/security/provider/KeyStore/DKSSystemPropertiesTest.java line 94: > 92: Date.from(keystore.getCreationInstant(alias))) > 93: ) { > 94: throw new RuntimeException( For consistency with the other exception thrown in this method, can this simply throw `Exception` as well? Suggestion: throw new Exception( test/jdk/sun/security/provider/KeyStore/DKSTest.java line 29: > 27: * @library /test/lib > 28: * @summary Support the logical grouping of keystores > 29: * @run junit DKSTest In some of the other JUnit PRs it was recommended to use `${test.main.class}` to avoid repetition. Would that work here as well? Suggestion: * @run junit ${test.main.class} test/jdk/sun/security/provider/KeyStore/DKSTest.java line 95: > 93: private static final Map<String, KeyStore.ProtectionParameter> > 94: WRONG_PASSWORDS = new HashMap<>() {{ > 95: put("policy_keystore", Is it intended that the indentation here is inconsistent with the one for `PASSWORDS` above? test/jdk/sun/security/provider/KeyStore/DKSTest.java line 113: > 111: final KeyStore ks = KeyStore.getInstance("DKS"); > 112: ks.load(new DomainLoadStoreParameter(config, > WRONG_PASSWORDS)); > 113: throw new RuntimeException("Expected exception not thrown"); Use JUnit's `assertThrows`? For example: var e = assertThrows(IOException.class, ...); ... // check e.getCause() test/jdk/sun/security/provider/KeyStore/DKSTest.java line 211: > 209: // get the new trusted certificate entry > 210: System.out.println("Getting new trusted certificate entry: " + > alias); > 211: if (!keystore.isCertificateEntry(alias)) { Use `assertTrue` for this `isCertificateEntry` check? test/jdk/sun/security/provider/KeyStore/DKSTest.java line 216: > 214: } > 215: keystore.setEntry(alias, > 216: new KeyStore.TrustedCertificateEntry(cert), null); Is this `setEntry` call here just verifying that no exception is thrown, or should there be a subsequent `isCertificateEntry` check? test/jdk/sun/security/provider/KeyStore/DKSTest.java line 429: > 427: } > 428: return false; > 429: } Maybe it would make sense to rewrite this `causedBy` as custom assertion method, since that is the only way this is currently used? // asserts that an exception was caused by specified exception class private static void assertCausedBy(Exception e, Class klass) { Throwable cause = e; while ((cause = cause.getCause()) != null) { if (cause.getClass().equals(klass)) { return; } } fail("Exception not caused by " + klass, e); } ------------- PR Review: https://git.openjdk.org/jdk/pull/30712#pullrequestreview-4100994595 PR Review Comment: https://git.openjdk.org/jdk/pull/30712#discussion_r3074977033 PR Review Comment: https://git.openjdk.org/jdk/pull/30712#discussion_r3074987036 PR Review Comment: https://git.openjdk.org/jdk/pull/30712#discussion_r3074990011 PR Review Comment: https://git.openjdk.org/jdk/pull/30712#discussion_r3074998546 PR Review Comment: https://git.openjdk.org/jdk/pull/30712#discussion_r3075018167 PR Review Comment: https://git.openjdk.org/jdk/pull/30712#discussion_r3075031541 PR Review Comment: https://git.openjdk.org/jdk/pull/30712#discussion_r3075034664 PR Review Comment: https://git.openjdk.org/jdk/pull/30712#discussion_r3075079757 PR Review Comment: https://git.openjdk.org/jdk/pull/30712#discussion_r3075077528 PR Review Comment: https://git.openjdk.org/jdk/pull/30712#discussion_r3075112778
