RFR: 8360498: [TEST_BUG] Some Mixing test continue to fail [v18]
Alexey Ivanov
aivanov at openjdk.org
Thu Dec 11 21:13:58 UTC 2025
On Thu, 25 Sep 2025 14:37:52 GMT, Khalid Boulanouare <duke at openjdk.org> wrote:
>> This PR will consolidate fixes of the following bugs:
>>
>> https://bugs.openjdk.org/browse/JDK-8361188
>> https://bugs.openjdk.org/browse/JDK-8361189
>> https://bugs.openjdk.org/browse/JDK-8361190
>> https://bugs.openjdk.org/browse/JDK-8361191
>> https://bugs.openjdk.org/browse/JDK-8361192
>> https://bugs.openjdk.org/browse/JDK-8361193
>> https://bugs.openjdk.org/browse/JDK-8361195
>>
>> This PR depends on https://github.com/openjdk/jdk/pull/25971
>>
>> For test : java/awt/Mixing/AWT_Mixing/JComboBoxOverlapping.java, the fix suggested is to return false in method isValidForPixelCheck for embedded frame, in which case the component is set to null. For more details see bug: [JDK-8361188](https://bugs.openjdk.org/browse/JDK-8361188)
>>
>> For test : test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java, I had to create a a tolerance color matching method for mac for the tests to pass. Also, the jbuttons needed to have different color than the color of the background frame, in order for test to pass. For more detail see bug: https://bugs.openjdk.org/browse/JDK-8361193
>>
>> For test : test/jdk/java/awt/Mixing/AWT_Mixing/JSplitPaneOverlapping.java, it seems that color selected for lightweight component matches the background color of the frame. And this will cause the test to fail when matching colors. Choosing any color different than the background color will get the test to pass. For more details, see bug: https://bugs.openjdk.org/browse/JDK-8361192
>>
>> For test test/jdk/java/awt/Mixing/AWT_Mixing/JPopupMenuOverlapping.java, it looks like the frame when visible, the popup test does not work properly. The frame needs to be hidden for the test to click on popup. For more details see bug: https://bugs.openjdk.org/browse/JDK-8361191
>>
>> For test test/jdk/java/awt/Mixing/AWT_Mixing/JMenuBarOverlapping.java, the test runs successfully but it times out after the default 2 minutes of jtreg. increasing the timeout to 3 minutes get the test to pass. For more details please refer to bug: https://bugs.openjdk.org/browse/JDK-8361190
>
> Khalid Boulanouare has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 37 commits:
>
> - Merge branch 'pr/25971' into jdk-8360498
> - Merge branch 'openjdk:master' into jdk-8360498
> - Resolves confict for when there is a merge with jdk-8158801
> - Merge branch 'openjdk:master' into jdk-8360498
> - Removes not needed changes
> - Revert "Removes not needed changes"
>
> This reverts commit e76780d50cc390e35443dccb193cfbc9a1cec1cb.
> - Removes not needed changes
> - Removes extra white lines
> - Merge branch 'pr/25971' into jdk-8360498
> - Merge branch 'pr/25971' into jdk-8360498
> - ... and 27 more: https://git.openjdk.org/jdk/compare/59a937ac...900a7943
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line 45:
> 43:
> 44:
> 45: import javax.swing.JFrame;
Imports look inconsistent.
test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line 135:
> 133: });
> 134: if (testResize) {
> 135: wasLWClicked = false;
Suggestion:
if (!testResize) {
return true;
Invert the condition. We did this in #25971, it removes additional indentation and makes the code clearer.
You probably want to move this condition above the focus latch.
test/jdk/java/awt/Mixing/AWT_Mixing/GlassPaneOverlappingTestBase.java line 157:
> 155: } catch (InvocationTargetException ex) {
> 156: fail(ex.getMessage());
> 157: }
Suggestion:
} catch (InterruptedException | InvocationTargetException ex) {
fail(ex.getMessage());
}
Merge them together.
test/jdk/java/awt/Mixing/AWT_Mixing/JComboBoxOverlapping.java line 71:
> 69:
> 70:
> 71: cb = new JComboBox(petStrings);
Suggestion:
frame.setVisible(true);
cb = new JComboBox(petStrings);
One blank between code sections is enough.
test/jdk/java/awt/Mixing/AWT_Mixing/JMenuBarOverlapping.java line 77:
> 75: frame.setSize(200, 200);
> 76: frame.setLocationRelativeTo(null);
> 77: frame.setVisible(true);
There are calls to `setLocationRelativeTo(null)` and `setVisible(true)` in the bottom of the method.
test/jdk/java/awt/Mixing/AWT_Mixing/JPopupMenuOverlapping.java line 80:
> 78: lwClicked = true;
> 79: frame.setVisible(false);
> 80: }
Why is it needed to hide the frame now? It wasn't hidden previously.
test/jdk/java/awt/Mixing/AWT_Mixing/JSplitPaneOverlapping.java line 74:
> 72: sp1 = new JScrollPane(p);
> 73: currentAwtControl.setForeground(Color.white);
> 74: currentAwtControl.setBackground(Color.white);
Suggestion:
currentAwtControl.setForeground(Color.WHITE);
currentAwtControl.setBackground(Color.WHITE);
Let's use upper-case *constants*.
test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 82:
> 80: private static int frameBorderCounter() {
> 81:
> 82: String JAVA_HOME = System.getProperty("java.home");
Please don't add redundant blank lines at the very start of a method.
test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 85:
> 83: try {
> 84: Process p = Runtime.getRuntime()
> 85: .exec(JAVA_HOME + "/bin/java FrameBorderCounter");
Did I already asked this question?
Why can't the code in `FrameBorderCounter` be run in the same JVM? I'm pretty sure it can be.
It's like for a separate refactoring, yet I'd like to have a tracking bug for getting rid of launching another process to count something that can be done by executing `FrameBorderCounter.main` directly and ensuring the frame is disposed of.
test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 106:
> 104: private static String readInputStream(InputStream is) throws IOException {
> 105:
> 106: byte[] buffer = new byte[4096];
Remove this redundant line.
test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 122:
> 120: borderShift = frameBorderCounter();
> 121: borderShift =
> 122: Math.abs(borderShift) == 1 ? borderShift : (borderShift / 2);
I'd leave it as is, or, if you prefer updating, rather reformat it like this:
Suggestion:
borderShift = Math.abs(borderShift) == 1
? borderShift
: (borderShift / 2);
test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 171:
> 169: public void run() {
> 170:
> 171: lLoc = frame.getLocationOnScreen();
Remove this redundant blank line…
However, many `Runnable`s have a blank line right after the `run()` method declaration. In this case, I'd follow this style for consistency.
test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 185:
> 183: public void run() {
> 184:
> 185: Point btnLoc = jbutton.getLocationOnScreen();
My preference would be to have only one of these blank lines.
test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 257:
> 255: public static void main(String args[]) throws Exception {
> 256:
> 257: try {
Suggestion:
public static void main(String args[]) throws Exception {
try {
A blank line at the start of a method is redundant.
test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 311:
> 309: public static synchronized void pass() {
> 310:
> 311: System.out.println("The test passed.");
All these added blank lines are redundant.
test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 349:
> 347:
> 348: System.out.println("Comparing color: " + color + " with reference " +
> 349: "color: " + refColor);
Suggestion:
private static boolean isAlmostEqualColor(Color color, Color refColor) {
System.out.println("Comparing color: " + color + " with reference " +
"color: " + refColor);
test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 356:
> 354: TOLERANCE_MACOSX &&
> 355: Math.abs(color.getBlue() - refColor.getBlue()) <
> 356: TOLERANCE_MACOSX;
Suggestion:
return color.equals(refColor)
|| (Math.abs(color.getRed() - refColor.getRed()) < TOLERANCE_MACOSX
&& Math.abs(color.getGreen() - refColor.getGreen()) < TOLERANCE_MACOSX
&& Math.abs(color.getBlue() - refColor.getBlue()) < TOLERANCE_MACOSX);
I strongly prefer this style of wrapping: wrapping before a binary operator makes it clear that it's a continuation; parentheses explicitly group comparison of color components.
I would not wrap the lines with `< TOLERANCE_MACOSX` although they don't fit 80-column limit. Wrapping doesn't help parsing the expression.
You can shorten `TOLERANCE_MACOSX` to `TOLERANCE`. Yet the current name suggests the tolerance is in effect only on macOS.
test/jdk/java/awt/Mixing/AWT_Mixing/MixingPanelsResizing.java line 361:
> 359: static class TestPassedException extends RuntimeException {
> 360:
> 361: }
I see no reason to add a blank line inside the declaration of the `TestPassedException` class.
test/jdk/java/awt/Mixing/AWT_Mixing/OverlappingTestBase.java line 387:
> 385: return !(component == null ||
> 386: (component instanceof java.awt.Scrollbar) ||
> 387: (isMac && (component instanceof java.awt.Button)));
Suggestion:
return !(component instanceof java.awt.Scrollbar)
&& (!isMac || (!(component instanceof java.awt.Button)));
The expression can be simplified further.
`java.awt.Scrollbar` is already imported, so `Scrollbar` can safely be used. I'm for importing `java.awt.Button` and using unqualified `Button` in the expression.
test/jdk/java/awt/Mixing/AWT_Mixing/SimpleOverlappingTestBase.java line 171:
> 169: } else {
> 170: latch.countDown();
> 171: }
`latch` isn't declared in this scope.
test/jdk/java/awt/Mixing/AWT_Mixing/SimpleOverlappingTestBase.java line 173:
> 171: }
> 172: try {
> 173: boolean await = latch.await(1, TimeUnit.SECONDS);
`latch` isn't declared in this scope.
test/jdk/java/awt/Mixing/AWT_Mixing/SimpleOverlappingTestBase.java line 187:
> 185: throw new RuntimeException(
> 186: "Ancestor frame didn't receive focus");
> 187: }
Indentation is wrong.
test/jdk/java/awt/Mixing/AWT_Mixing/SimpleOverlappingTestBase.java line 199:
> 197:
> 198:
> 199: return wasLWClicked;
One blank line seems enough between code sections.
-------------
PR Review: https://git.openjdk.org/jdk/pull/26625#pullrequestreview-3569053278
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611877977
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611867427
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611872778
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611876184
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611881799
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611885553
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611903091
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611909297
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2612020583
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611919078
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611924803
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611929707
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611933903
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611940143
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611942003
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611943663
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611965281
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611945982
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611979119
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611999199
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2612003424
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2611987770
PR Review Comment: https://git.openjdk.org/jdk/pull/26625#discussion_r2612005602
More information about the client-libs-dev
mailing list