On Thu, 19 Feb 2026 20:18:17 GMT, Justin Lu <[email protected]> wrote:
> This PR migrates the java/util/jar tests to use _JUnit_. > > https://github.com/openjdk/jdk/commit/afe0aeee746bccbbe4bc6c9a8cd2302228ecc2f6 > includes changes for _testNG_ based tests. > https://github.com/openjdk/jdk/commit/c5a7f75840f96fa77ec3ed7faa713990adb84de6 > includes changes for `main` based tests. > > Before: Framework-based tests: 125 = 125 TestNG + 0 JUnit. > After: Framework-based tests: 174 = 0 TestNG + 174 JUnit Left some comments here and there :-) test/jdk/java/util/jar/JarEntry/GetMethodsReturnClones.java line 73: > 71: for (JarEntry je : jarEntries) { > 72: Certificate[] certs = je.getCertificates(); > 73: if (certs != null) { % jarsigner -verify test/jdk/java/util/jar/JarEntry/test.jar The jar will be treated as unsigned, because it is signed with a weak algorithm that is now disabled. This test effectively does nothing. Filed https://bugs.openjdk.org/browse/JDK-8378291 to track this. test/jdk/java/util/jar/JarFile/SignedJarPendingBlock.java line 75: > 73: } > 74: > 75: @ParameterizedTest There are two positive and a single negative test here. I don't think using `@ParameterizedTest` carries its weight in terms of the indirection cost. I think just three regular `@Test` methods would make this test easier to read. test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarSecurity.java line 56: > 54: static final int MAJOR_VERSION = Runtime.version().major(); > 55: > 56: private static final String userdir = > System.getProperty("user.dir","."); Debatable whether `private` adds value for a test class like this or just adds clutter. test/jdk/java/util/jar/JarFile/mrjar/MultiReleaseJarSecurity.java line 79: > 77: CertsAndSigners vcas = new CertsAndSigners(jf, > jf.getJarEntry("version/Version.class")); > 78: CertsAndSigners rcas = new CertsAndSigners(jf, > jf.getJarEntry("META-INF/versions/" + MAJOR_VERSION + > "/version/Version.class")); > 79: assertTrue(Arrays.equals(rcas.getCertificates(), > vcas.getCertificates())); Could `assertArrayEquals` be used here and below? test/jdk/java/util/jar/JarFile/mrjar/TestVersionedStream.java line 127: > 125: } > 126: > 127: public static Stream<Runtime.Version> data() { I would rename this `arguments()`, `versions()` or something. `data()` seems a `@DataProvider` remnant. test/jdk/java/util/jar/JarFile/mrjar/TestVersionedStream.java line 173: > 171: if (!match) { > 172: fail("versioned entries not in same order as unversioned > entries"); > 173: } Tempting to use `assertIterableEquals` here: // also keep the names List<String> versionedNames = versionedEntries.stream() .map(JarEntry::getName) .collect(Collectors.toList()); // verify the correct order while building enames List<String> unversionedOrder = new ArrayList<>(unversionedEntryNames); unversionedOrder.retainAll(versionedNames); assertIterableEquals(unversionedOrder, versionedNames, "versioned entries not in same order as unversioned entries"); test/jdk/java/util/jar/Manifest/IncludeInExceptionsTest.java line 47: > 45: * @run junit/othervm -Djdk.includeInExceptions=jar > IncludeInExceptionsTest > 46: * @run junit/othervm IncludeInExceptionsTest > 47: * @summary Verify that the property jdk.net.includeInExceptions works as > expected `jdk.net.includeInExceptions` is not the property being tested here, should be `jdk.includeInExceptions` ? test/jdk/java/util/jar/Manifest/IncludeInExceptionsTest.java line 80: > 78: @ParameterizedTest > 79: @MethodSource("manifests") > 80: void testInvalidManifest(Callable<?> attempt, boolean > includeInExceptions) { `includeInExceptions` is a static final, passing it around as a parameter seems unneccesary. If we use `includeInExceptions` directly, could `manifests()` return simply `Stream<Callable<?>` ? test/jdk/java/util/jar/Manifest/WriteBinaryStructure.java line 53: > 51: "Manifest-Version: 1.0\r\n" + > 52: "Key: Value\r\n" + > 53: "\r\n").getBytes(UTF_8), buf.toByteArray()); Consider putting the actual on a separate line here and below. test/jdk/java/util/jar/TestExtra.java line 78: > 76: @Test > 77: void extraHeaderPlusDataTest() throws IOException { > 78: new TestExtra().testHeaderPlusData(); Consider letting JUnit handle the instance state management to avoid these wrapper test methods. ------------- PR Review: https://git.openjdk.org/jdk/pull/29828#pullrequestreview-3828563009 PR Review Comment: https://git.openjdk.org/jdk/pull/29828#discussion_r2830276442 PR Review Comment: https://git.openjdk.org/jdk/pull/29828#discussion_r2831635421 PR Review Comment: https://git.openjdk.org/jdk/pull/29828#discussion_r2830302554 PR Review Comment: https://git.openjdk.org/jdk/pull/29828#discussion_r2830306594 PR Review Comment: https://git.openjdk.org/jdk/pull/29828#discussion_r2830316302 PR Review Comment: https://git.openjdk.org/jdk/pull/29828#discussion_r2830426138 PR Review Comment: https://git.openjdk.org/jdk/pull/29828#discussion_r2830193164 PR Review Comment: https://git.openjdk.org/jdk/pull/29828#discussion_r2832074614 PR Review Comment: https://git.openjdk.org/jdk/pull/29828#discussion_r2832080235 PR Review Comment: https://git.openjdk.org/jdk/pull/29828#discussion_r2832092148
