RFR: 8321139: jlink's man page does not document the --compress option correctly [v4]
Jaikiran Pai
jpai at openjdk.org
Tue Nov 25 14:37:43 UTC 2025
On Fri, 21 Nov 2025 18:43:21 GMT, Ana Maria Mihalceanu <duke at openjdk.org> wrote:
>> test/jdk/tools/jlink/TaskHelperTest.java line 279:
>>
>>> 277: var remaining = optionsHelper.handleOptions(this, args.tokens);
>>> 278: assertTrue(remaining.isEmpty());
>>> 279: assertEquals(args.expectedCompressValue, compressArgValue);
>>
>> I think these 2 asserts are an oversight. Since we expect `optionsHelper.handleOptions(...)` to always throw a `BadArgs` for these invalid option values, we should replace these 2 asserts with a:
>>
>> fail("processing of " + Arrays.toString(testCase.tokens) + " was expected to fail, but didn't");
>>
>> This will then rightly fail the test if the exception isn't thrown.
>
> To my understanding, for compression plugin the validation of the actual value is performed in `DefaultCompressPlugin` (https://github.com/openjdk/jdk/blob/d57fc1b6dc313eb004892b180960ebcee1cb04c7/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/DefaultCompressPlugin.java#L118 and https://github.com/openjdk/jdk/blob/d57fc1b6dc313eb004892b180960ebcee1cb04c7/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/plugins/DefaultCompressPlugin.java#L115).
>
> At the moment, [`optionsHelper.handleOptions`](https://github.com/openjdk/jdk/blob/3a45e615973727446c9081b5affbbe7ffe7c3bea/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/TaskHelper.java#L527) seems to set up `TaskHelper`'s internal state and it shouldn't throw exceptions for syntactically correct options. In my opinion, this fix is not that much related to the value allowed, but to the syntax allowed for the option. I noticed that there is another test class [`JLinkTest`](https://github.com/openjdk/jdk/blob/master/test/jdk/tools/jlink/JLinkTest.java) that tests for actual values. When I tested locally my build of this modified `jlink`, for invalid values I did get that `IllegalArgumentException`.
>
> However, I am very new to the testing framework and am welcoming more thoughts/ideas 🙏 .
Hello Ana, I had a look at the `JLinkTest` and like you note it already has some testing for the `--compress` option there (the testCompress method). I think we should move the `testCompressInvalidValue()` that we introduced in this PR to that `JLinkTest` and test those invalid usages there, in a similar manner to what's already being tested there (for example the section below "// invalid compress level" in that test)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28359#discussion_r2560210261
More information about the core-libs-dev
mailing list