RFR: 8360498: [TEST_BUG] Some Mixing test continue to fail [v36]
Khalid Boulanouare
duke at openjdk.org
Tue Feb 17 13:21:48 UTC 2026
On Fri, 13 Feb 2026 15:36:50 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:
>> Khalid Boulanouare has updated the pull request incrementally with one additional commit since the last revision:
>>
>> Re problem list unstable test
>
> test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line 133:
>
>> 131: testedComponent.setBounds(0, 0,
>> 132: testedComponent.getPreferredSize().width,
>> 133: testedComponent.getPreferredSize().height + 20);
>
> This added code fully duplicates the existing code at lines [167–184](https://github.com/kboulanou/jdk/blob/a2b9a388fbcf836de159705500bdb22cedb2d1d9/test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java#L167-L184) except for focus handling. Modify the code below.
>
> Otherwise, lines 167–184 are unreachable.
Code modified as requested.
> test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line 135:
>
>> 133: testedComponent.getPreferredSize().height + 20);
>> 134: Component focusOwner = KeyboardFocusManager.getCurrentKeyboardFocusManager()
>> 135: .getFocusOwner();
>
> Suggestion:
>
> Component focusOwner = KeyboardFocusManager
> .getCurrentKeyboardFocusManager()
> .getFocusOwner();
>
> What do you think about this way? The line isn't as long.
Looks good to me. I updated code accordingly.
> test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line 148:
>
>> 146: }
>> 147: Point lLoc = testedComponent.getLocationOnScreen();
>> 148: lLoc.translate(1, testedComponent.getPreferredSize().height + 1);
>
> `getLocationOnScreen` and `getPreferredSize` are better called on EDT, however, I think it's safe here. You've got a synchronisation point with `invokeAndWait`, neither `getLocationOnScreen` and `getPreferredSize` could change, therefore the returned values would be consistent.
Tasks run in EDT, please have a look. Thanks.
> test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line 165:
>
>> 163:
>> 164: return true;
>> 165: }
>
> This looks weird. I'd separate the logical blocks above with blank lines, but here the blank is absolutely redundant.
>
> Moreover, it seems to me `if (!resize)` should've remained above. That is if `!resize` we don't need the latch, we don't need anything because the test is skipped. So keep it that way: exit from the method early than later.
>
> Everything the follows `if (!resize)` block is where the action is happening.
>
> In other words, what I suggest is
>
>
> protected boolean performTest() {
> if (!super.performTest()) {
> return false;
> }
>
> if (!testResize) {
> return true;
> }
>
> // The above piece of code remains unchanged
>
> // Create the latch and handle focus here
> // ...
I have re-ordered the sequence of execution here. Please have a look. Thanks.
> test/jdk/java/awt/Mixing/AWT_Mixing/JComboBoxOverlapping.java line 29:
>
>> 27: import java.awt.event.ActionEvent;
>> 28: import java.awt.event.ActionListener;
>> 29: import java.lang.Override;
>
> No need to explicitly import anything from the `java.lang` package.
I have removed this unnecess ary import from this class and other classes.
> test/jdk/java/awt/Mixing/AWT_Mixing/JComboBoxOverlapping.java line 70:
>
>> 68: frame.getContentPane().setLayout(new BoxLayout(frame.getContentPane(), BoxLayout.Y_AXIS));
>> 69: frame.setSize(200, 200);
>> 70: cb = new JComboBox(petStrings);
>
> I prefer to keep this blank line, it separates creating the frame and setting its size from creating and configuring the combo box.
I have restored back this space.
> test/jdk/java/awt/Mixing/AWT_Mixing/JComboBoxOverlapping.java line 104:
>
>> 102:
>> 103: loc2.translate(75, 75);
>> 104: robot.mouseMove(0, 0);
>
> I believe this line is asking for comment:
>
>
> Suggestion:
>
> robot.mouseMove(0, 0); // Avoid capturing mouse cursor
Comment added as requested.
> test/jdk/java/awt/Mixing/AWT_Mixing/JMenuBarOverlapping.java line 32:
>
>> 30: import java.awt.event.MouseAdapter;
>> 31: import java.awt.event.MouseEvent;
>> 32: import java.lang.Override;
>
> Suggestion:
>
>
> Redundant import.
Unnecessary comment removed.
> test/jdk/java/awt/Mixing/AWT_Mixing/JMenuBarOverlapping.java line 79:
>
>> 77: frame.setLocationRelativeTo(null);
>> 78: frame.setVisible(true);
>> 79: menuBar = new JMenuBar();
>
> Suggestion:
>
> frame.setLocationRelativeTo(null);
> frame.setVisible(true);
>
> menuBar = new JMenuBar();
>
> Preserve the blank line.
Blank line restored as requested.
> test/jdk/java/awt/Mixing/AWT_Mixing/JMenuBarOverlapping.java line 110:
>
>> 108: propagateAWTControls(frame);
>> 109:
>> 110: }
>
> Suggestion:
>
> }
>
> A blank line right before the closing brace is redundant.
Blank line added.
> test/jdk/java/awt/Mixing/AWT_Mixing/JMenuBarOverlapping.java line 126:
>
>> 124: Robot robot = Util.createRobot();
>> 125: robot.setAutoDelay(ROBOT_DELAY);
>> 126: robot.mouseMove(0, 0);
>
> A comment would be nice.
Comment added as requested.
> test/jdk/java/awt/Mixing/AWT_Mixing/JPopupMenuOverlapping.java line 92:
>
>> 90: frame.setVisible(true);
>> 91: loc = frame.getContentPane().getLocationOnScreen();
>> 92: }
>
> You removed `setLocationRelativeTo` and `setVisible` in `JMenuBarOverlapping.java`, should these be removed too?
>
> `setLocationRelativeTo` is redundant here, as you already called it on line 73.
setLocationRelativeTo and setVisible removed as requested.
> test/jdk/java/awt/Mixing/AWT_Mixing/JPopupMenuOverlapping.java line 99:
>
>> 97: Robot robot = Util.createRobot();
>> 98: robot.setAutoDelay(ROBOT_DELAY);
>> 99: robot.mouseMove(0, 0);
>
> I'd add a comment to explain moving to (0, 0).
Comment added.
> test/jdk/java/awt/Mixing/AWT_Mixing/JPopupMenuOverlapping.java line 104:
>
>> 102:
>> 103: pixelPreCheck(robot, loc, currentAwtControl);
>> 104: try {
>
> If removing a blank line, I'd rather remove the one between `translate` and `pixelPreCheck` instead of this. Those calls are closer related than the `try` block below.
Blank line restored and removed blank line between `translate` and `pixelPreCheck`.
> test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 61:
>
>> 59: public class MixingPanelsResizing {
>> 60:
>> 61: static final int TOLERANCE_MACOSX = 15;
>
> Suggestion:
>
> private static final int TOLERANCE_MACOSX = 15;
>
> For consistency with other declarations.
Field accessor added as requested.
> test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 62:
>
>> 60:
>> 61: static final int TOLERANCE_MACOSX = 15;
>> 62: static volatile boolean failed = false;
>
> The field `failed` in unused, it can be removed then.
Unnecessary field removed as requested.
> test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 119:
>
>> 117: Math.abs(borderShift) == 1 ?
>> 118: borderShift :
>> 119: (borderShift / 2);
>
> Suggestion:
>
> borderShift = Math.abs(borderShift) == 1
> ? borderShift
> : (borderShift / 2);
>
> I prefer this formatting. Yet we don't have a style guide to follow.
>
> In general, wrapping before the operator makes it clearer that it's a continuation line.
Formatting updated as requested.
> test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 169:
>
>> 167: SwingUtilities.invokeAndWait(new Runnable() {
>> 168:
>> 169: public void run() {
>
> Suggestion:
>
> SwingUtilities.invokeAndWait(new Runnable() {
> public void run() {
>
> I'd rather not add this line… On the other hand, it's more or less consistent with other cases where the `Runnable` interface is implemented.
Blank line removed as requested.
> test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 255:
>
>> 253: public static void main(String args[]) throws Exception {
>> 254:
>> 255: if (!Toolkit.getDefaultToolkit().isDynamicLayoutActive()) {
>
> I'm strongly against adding blank lines at the start of a method. The declaration of a method serves as the start of a new logical block by itself.
I have removed blank line as requested.
> test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 277:
>
>> 275: //Timed out, so fail the test
>> 276: throw new RuntimeException(
>> 277: "Timed out after " + sleepTime / 1000 + " seconds");
>
> Suggestion:
>
> "Timed out after " + (sleepTime / 1000) + " seconds");
>
> Parentheses aren't required here, yet they help avoid any ambiguity.
Formatting updated as requested.
> test/jdk/java/awt/Mixing/AWT_Mixing/OpaqueOverlapping.java line 1:
>
>> 1: /*
>
> If there's nothing else modified in the `OpaqueOverlapping.java` file, I'd rather revert these changes and leave the file untouched, moreover importing `java.lang.Override` is not needed.
File restored from previous version.
> test/jdk/java/awt/Mixing/AWT_Mixing/OverlappingTestBase.java line 32:
>
>> 30: import java.awt.Dimension;
>> 31: import java.awt.Font;
>> 32: import java.awt.Helper;
>
> I'd probably put the helper into the section of helper classes… but this means that imports need manual editing after IDE updates them for you. So, probably leave it as is now.
I see, I have left it as it is. Thanks
> test/jdk/java/awt/Mixing/AWT_Mixing/OverlappingTestBase.java line 47:
>
>> 45: import java.io.InputStream;
>> 46: import java.io.InputStreamReader;
>> 47: import java.lang.Override;
>
> No need to import `Override`.
Unnecessary import removed.
> test/jdk/java/awt/Mixing/AWT_Mixing/OverlappingTestBase.java line 389:
>
>> 387: return !(component == null ||
>> 388: (component instanceof Scrollbar) ||
>> 389: (isMac && (component instanceof Button)));
>
> Suggestion:
>
> return !(component == null
> || (component instanceof Scrollbar)
> || (isMac && (component instanceof Button)));
>
> Wrap before the operator to clearly indicate a continuation line.
Operators location updated as requested.
> test/jdk/java/awt/Mixing/AWT_Mixing/OverlappingTestBase.java line 389:
>
>> 387: return !(component == null ||
>> 388: (component instanceof Scrollbar) ||
>> 389: (isMac && (component instanceof Button)));
>
> Does inverting the condition makes it clearer?
>
>
> return component != null
> && !(component instanceof Scrollbar)
> && !(isMac && (component instanceof Button));
>
> That is verify any non-null component except `Scrollbar`, and in addition `Button` on macOS.
Code update as requested.
> test/jdk/java/awt/Mixing/AWT_Mixing/SimpleOverlappingTestBase.java line 161:
>
>> 159: JFrame ancestor = (JFrame) (testedComponent.getTopLevelAncestor());
>> 160: final CountDownLatch latch = new CountDownLatch(1);
>> 161: if (ancestor != null) {
>
> Suggestion:
>
> final CountDownLatch latch = new CountDownLatch(1);
>
> JFrame ancestor = (JFrame) (testedComponent.getTopLevelAncestor());
> if (ancestor != null) {
>
> I suggest moving the declarations around. The `latch` is helper object. You then get the ancestor and work with it.
Order of execution updated as requested.
> test/jdk/java/awt/Mixing/AWT_Mixing/SimpleOverlappingTestBase.java line 171:
>
>> 169: latch.countDown();
>> 170: }
>> 171: try {
>
> Add a blank line between the `if` statement and `try`. The `if` block prepares for the code inside `try`, they're related, but still do separate job.
>
> Reading code that separates logical blocks with blank lines is easier.
Blank line added as requested.
> test/jdk/java/awt/Mixing/AWT_Mixing/SimpleOverlappingTestBase.java line 173:
>
>> 171: try {
>> 172: boolean await = latch.await(1, TimeUnit.SECONDS);
>> 173: if (!await) {
>
> Suggestion:
>
> if (!latch.await(1, TimeUnit.SECONDS)) {
>
> The usage of a local variable doesn't add clarity.
Unnecessary local variable removed as suggested.
> test/jdk/java/awt/Mixing/AWT_Mixing/SimpleOverlappingTestBase.java line 181:
>
>> 179: throw new RuntimeException(e);
>> 180: }
>> 181: if (ancestor != null && isMultiFramesTest()) {
>
> Move the call to `clickAndBlink` back here along with the blank line.
>
> You [say](https://github.com/openjdk/jdk/pull/26625/changes#r2650824338), “`clickAndBlink` happens only when we get focus”, which is directly after the `try` block. If the focus isn't received, an exception is thrown from the `try` block.
Order of execution updated as requested.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816923053
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816931726
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816928616
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816921525
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816938618
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816941556
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816942827
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816989616
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816944949
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816947178
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816988145
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816953294
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816948090
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816951576
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816957213
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816954624
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816958353
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816960534
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816986266
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816962036
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816983209
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816998102
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816963085
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816965367
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816968056
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816970509
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816980813
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816979573
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2816977810
More information about the client-libs-dev
mailing list