RFR: 8378111: Migrate java/util/jar tests to JUnit

[Eirik Bjørsnøs]

On Thu, 19 Feb 2026 20:18:17 GMT, Justin Lu <jlu at openjdk.org> 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