On Thu, 21 Mar 2024 14:14:17 GMT, Alexander Zvegintsev <azveg...@openjdk.org>
wrote:
>> Damon Nguyen has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Review comments
>
> test/jdk/java/awt/image/multiresolution/MultiDisplayTest.java line 33:
>
>> 31: * on HiDPI + non-HiDPI display pair.
>> 32: * @run main MultiDisplayTest
>> 33: */
>
> We usually move this block just before a class declaration, for better
> readability.
Moved.
> test/jdk/java/awt/image/multiresolution/MultiDisplayTest.java line 58:
>
>> 56: """
>> 57: This test is for OS X or Windows only.
>> 58: For other OSes please simply push "Pass".
>
> `please simply push "Pass"` usually means that it can be replaced with
> `PassFailJFrame.forcePass()`, but in this case the `@requires (os.family...`
> can be used to avoid running the test on unwanted platforms.
Added jtreg `@requires` tag.
> test/jdk/java/awt/image/multiresolution/MultiDisplayTest.java line 93:
>
>> 91: generateImage(1, Color.BLACK), generateImage(2, Color.BLUE)});
>> 92:
>> 93: public static void main(String[] args) throws Exception {
>
> While the `@requires (os.family...` should be enough, we can add something
> like
>
> if (!checkOS()) {
> throw new SkippedException("..."); //jtreg.SkippedException
> }
Added. Thanks. Thought about not needing this at all, but it doesn't hurt to
add.
> test/jdk/java/awt/image/multiresolution/MultiDisplayTest.java line 109:
>
>> 107: frame.setLayout(new BorderLayout());
>> 108: Button b = new Button("Start");
>> 109: b.setEnabled(checkOS());
>
> With the changes mentioned above, this will become obsolete.
Removed the `checkOS()` call.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18354#discussion_r1534277257
PR Review Comment: https://git.openjdk.org/jdk/pull/18354#discussion_r1534277092
PR Review Comment: https://git.openjdk.org/jdk/pull/18354#discussion_r1534276882
PR Review Comment: https://git.openjdk.org/jdk/pull/18354#discussion_r1534277948
