On Tue, 25 Nov 2025 14:31:33 GMT, Jaikiran Pai <[email protected]> wrote:
>> 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) Hi Jaikiran, I followed your advice and moved the tests for invalid values to `JLinkTest`. Please let me know if you notice anything else that I missed. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/28359#discussion_r2560867281
