RFR: 8339984: Open source AWT MenuItem related tests [v2]

[Abhishek Kumar]

On Tue, 17 Sep 2024 19:00:55 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>> Abhishek Kumar has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Review comment fix
>
> test/jdk/java/awt/MenuItem/LotsOfMenuItemsTest.java line 48:
> 
>> 46:     private static final int NUM_WINDOWS = 400;
>> 47:     private static TestFrame firstFrame, testFrame;
>> 48:     private static Rectangle rect;
> 
> `rect` is never used.

Removed.

> test/jdk/java/awt/MenuItem/LotsOfMenuItemsTest.java line 86:
> 
>> 84:                 testFrame.setTitle("Last Frame");
>> 85:             }
>> 86:         }
> 
> Suggestion:
> 
>         for (int i = 1; i < NUM_WINDOWS - 1; ++i) {
>             testFrame = new TestFrame("Running(" + i + ")...");
>             testFrame.setVisible(false);
>             testFrame.dispose();
>         }
> 
>         testFrame = new TestFrame("Last Frame");
> 
> 
> Doesn't it make the code clearer and shorter?

Yeah.. updated now.

> test/jdk/java/awt/MenuItem/LotsOfMenuItemsTest.java line 98:
> 
>> 96:         firstFrame.setLocation(970, 350);
>> 97:         testFrame.setLocation(970, 510);
>> 98:     }
> 
> Hard-coding coordinates is not the best solution. The workaround is to position the first frame using `PassFailJFrame` and then use the location of the first frame to position the second (test) frame.
> Suggestion:
> 
>     @Override
>     public void componentShown(ComponentEvent e) {
>         PassFailJFrame.positionTestWindow(firstFrame,
>                                           PassFailJFrame.Position.HORIZONTAL);
>         testFrame.setLocation(firstFrame.getX(),
>                               firstFrame.getY() + firstFrame.getWidth() + 8);
>     }

I gave a try to get PassFailFrame position but some error occured and then hard coded. I agree hard-coding is not a best solution and thanks for suggesting the other way around. A bit of correction in setting the testFrame location, y co-ordinate should be firstFrame.getY() + firstFrame.getHeight() + 8 else the gap is more between firstFrame and testFrame.

> test/jdk/java/awt/MenuItem/LotsOfMenuItemsTest.java line 103:
> 
>> 101: }
>> 102: 
>> 103: class TestFrame extends Frame {
> 
> Could you please make `TestFrame` a static nested class inside `LotsOfMenuItemsTest`.
> 
> I avoids conflicts when you mark a folder as Test Source in an IDE and that folder contains multiple tests.
> 
> In fact, you don't need to extend `Frame` at all: create an instance of the `Frame` class and add menu bar and set the size of the frame. However, I agree, keeping this stuff in constructor simplifies test.

Updated.

> test/jdk/java/awt/MenuItem/MenuSetFontTest.java line 54:
> 
>> 52:                 .rows((int) INSTRUCTIONS.lines().count() + 2)
>> 53:                 .columns(40)
>> 54:                 .testTimeOut(5)
> 
> You may omit the default timeout.

Removed from all tests.

>Wouldn't it be better to keep the logical and visual order of the buttons consistent? Or is the intention to set the label to "text" after null followed by ""?

I Just left them as it was before conversion.

> test/jdk/java/awt/MenuItem/UnicodeMenuItemTest.java line 81:
> 
>> 79: 
>> 80:         MenuItem mi6 = new MenuItem("\u012d");
>> 81:         m.add(mi6);
> 
> The numbering is weird here: 2, 6, 7 — but there's no `mi1` or `m3`…

Ok.. Updated.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1764371719
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1764370259
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1764371347
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1764369981
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1764372023
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1764372474
PR Review Comment: https://git.openjdk.org/jdk/pull/21029#discussion_r1764372546