On Fri, 21 Nov 2025 10:35:50 GMT, Jaikiran Pai <[email protected]> wrote:
>> Ana Maria Mihalceanu has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add some negative tests for suggested values.
>
> 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 🙏 .
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28359#discussion_r2550661428
