On Fri, 21 Nov 2025 18:43:21 GMT, Ana Maria Mihalceanu <[email protected]> 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
