On Thu, 19 Feb 2026 21:12:41 GMT, Eirik Bjørsnøs <[email protected]> wrote:
>> Justin Lu has updated the pull request incrementally with three additional >> commits since the last revision: >> >> - Eirik comments >> - Naoto comment >> - Andrey comments > > 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` ? Thanks for the review and helpful comments! Looks like this property was renamed in [JDK-8207846](https://bugs.openjdk.org/browse/JDK-8207846). > 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. The reason I wrapped those tests was to control the executing test class, (i.e. `TestExtra` vs `TestJarExtra`). However, you are right that it's clutter and also doubling the instances. Keeping the inner `TestJarExtra` class is problematic because in its current `static` form, tests have to be run by the outer class leading to the manual ctors. If we make it a non static inner class and use `@nested` it causes a JUnit cycle error for the class hierarchy since the inner class also extends the outer class. I pulled `TestJarExtra` into its own test file which gets rid of the manual instance handling and is cleaner. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/29828#discussion_r2834609443 PR Review Comment: https://git.openjdk.org/jdk/pull/29828#discussion_r2834609236
