8276058: Some swing test fails on specific CI macos system #6140
Conversation
|
👋 Welcome back psadhukhan! A progress list of the required criteria for merging this PR into |
Webrevs
|
| robot.mousePress(InputEvent.BUTTON1_DOWN_MASK); | ||
| robot.waitForIdle(); | ||
| Color color = robot.getPixelColor(centerX-10, centerY-10); | ||
|
|
There was a problem hiding this comment.
So the problematic pattern is a mouse move to the location from which you subsequently capture a pixel?
I can see how on a compositing window manager that might be an issue as the cursor is part of the window
whereas on old X11 it isn't part of the root window .. but why isn't this ALWAYS an issue on macOS ?
I wonder how many other tests have the same issue ?
And is an offset of 10 enough ? Its a bit arbitrary and cursors could be a larger shape or different orientation ..
There was a problem hiding this comment.
Robot doesn't capture a cursor, we have a Robot#createScreenCapture spec which clearly says that:
Creates an image containing pixels read from the screen.
This image does not include the mouse cursor.
Both Robot#createScreenCapture and Robot#getPixelColor are using the same CRobot#nativeGetScreenPixels method for getting pixel data.
So the mouse cursor should not affect `Robot#getPixelColor`, this is easy to check on practice
import javax.imageio.ImageIO;
import java.awt.AWTException;
import java.awt.Color;
import java.awt.Frame;
import java.awt.Rectangle;
import java.awt.Robot;
import java.awt.image.BufferedImage;
import java.io.File;
import java.io.IOException;
public class CursorTest {
public static void main(String[] args) throws AWTException, IOException {
Robot robot = new Robot();
Frame frame = new Frame();
frame.setBackground(Color.red);
frame.setUndecorated(true);
Rectangle rectangle = new Rectangle(100,100,50,50);
frame.setBounds(rectangle);
frame.setVisible(true);
robot.waitForIdle();
robot.delay(1000);
robot.mouseMove(rectangle.x + 5, rectangle.y + 5);
robot.waitForIdle();
System.err.println("Moved");
robot.waitForIdle();
robot.delay(3000);
System.err.println("Starting");
BufferedImage screenCapture = robot.createScreenCapture(rectangle);
ImageIO.write(screenCapture, "png", new File("/tmp/out.png"));
for (int i = rectangle.x; i < rectangle.x + rectangle.width; i++) {
for (int j = rectangle.y; j < rectangle.y + rectangle.height; j++) {
Color pixelColor = robot.getPixelColor(i, j);
if (!pixelColor.equals(Color.red)) {
System.err.println(pixelColor);
frame.dispose();
throw new RuntimeException("Unexpected pixel color " + pixelColor);
}
}
}
System.err.println("Finished");
frame.dispose();
}
}There was a problem hiding this comment.
Ok .. yet Prasanta said it did ..
"The png image generated at failure also did not reveal anything abnormal apart from presence of mouse cursor around the same area where the getPixelColor location is pointing to."
| Rectangle screen = new Rectangle(0, 0, (int) screenSize.getWidth(), (int) screenSize.getHeight()); | ||
| BufferedImage img = robot.createScreenCapture(screen); | ||
| javax.imageio.ImageIO.write(img, "png", new java.io.File("image.png")); | ||
|
|
There was a problem hiding this comment.
Are we writing the image or any other type of file in the CWD in other tests ?
Shouldn't this use TESTCLASSES or something like that ?
| } | ||
| System.out.println("Test Passed"); |
There was a problem hiding this comment.
Hmm .. the test tags are
What if the default scale is 2 ?
Or on Windows it could be 1.25 ?
I don't see anything that restricts this test to macOS.
If you are going to explicitly test for 1 and 2 here, shouldn't we explicitly set it ?
And if you aren't then the test here needs to do something based on the actual scale.
|
Since it is unlikely to be the issue related to the cursor, it will be better to check what could be root cause. Is it possible that CI system is configured in some different way as others? Th 55/55/55 color looks similar to the background of the app in the dark mode in macOS, could it be the reason? |
Well .. the cursor issue is a suggestion for root cause [*] Do you have any other ideas ? I mean other than ...
Not that I can see and definitely not dark mode. Permanent light mode. |
|
awt test fix also added FullScreenInsets fails with incorrect pixel so added tolerance MakeWindowAlwaysOnTop fails with same root cause so used same swing tactics of x-10, y-10 |
As noted above our spec mention that the cursor should not be present on the screenshot, and we use the appropriate native API to achieve this. It is hard to say why it does not work, but since the problem is consistent which is great, then the best way to investigate it is by connecting to the system and debugging the problem.
In general, it does not look like a test issue, more like a product bug. So the root cause of this particular issue should be investigated. |
|
I have attached 2 screen capture in JBS taken when PressedIconTest and JInternalFrame test failed and the screenshot at least in this machine shows presence of cursor. |
So maybe something was changed in the native API? It looks like a tck issue that breaks the specified behavior. Is it happen only on this system? Could it be related to the cursor configuration in the settings, size/visibility/scale etc? |
See the comment I just added to https://bugs.openjdk.java.net/browse/JDK-8252813. It still needs verification to see if it affects the macOS API used. But it seems probable something like that is going on. |
|
Yes looks like it is related to the cursor size in the settings |
|
I think we should report this to Apple. |
Sorry, not following .. do you mean you think the cursor is captured when it is enlarged ? BTW when accessing the test system remotely it seems that A11Y feature is disabled. The slider has no effect. |
I can confirm that the changing the size of the cursor on that setting page can change the visibility of the cursor on the screenshot. If the cursor is "small" then it is completely invisible, if the cursor size is changed above some threshold it became visible on the screenshot. So it might be a case when on that system the default cursor is bigger that the threshold. |
So probably the local settings is not default/changed, and what is shown on the setting you see is for remote view? |
Hmm .. well it is at the minimum right now. So throw that one in the bag of things that can go wrong .. but I'm not currently seeing it set to anything except absolute minimum, The mechanism by which any of these things suddenly happen and then suddenly go away is really unclear to me .. |
Well I can move the slider so I think I am seeing the REAL setting but it just doesn't get used in screen sharing mode |
|
I tried it on the other macOS system, this time aarch64. And the cursor is visible there even if the size is set to small. |
|
Looks like sometimes the cursor may be rendered in the framebuffer and sometimes not:
Maybe we should wrap the taking screenshot by the CGDisplayHideCursor/CGDisplayShowCursor. |
If it's not right, then we have to change at so many files..All files uses the same approach so I followed...Where to ask? not sure about jtreg devs contact? |
Missed that, and still cannot find it by searching the discussion above, I probably need to rest |
It was added as part of https://bugs.openjdk.java.net/browse/JDK-8252813 comment mentionedin this PR. |
So it is even a different bug =(.. |
I had written (above many hours ago) - See the comment I just added to https://bugs.openjdk.java.net/browse/JDK-8252813. It still needs verification to see if it affects the macOS API used. But it seems probable something like that is going on. |
|
|
I have removed the tolerance change and PL-ed the test....Any more comments on this PR? |
If tolerance is not needed and the only issue we have is the visible cursor on the screenshot then why do we try to update the tests, maybe we can try to update the jdk instead? |
I guess it was already discussed before as below to have the stability improvement in the tests and you agreed to it also but if the stance is changed, I do not have issue in withdrawing this PR.
|
|
I don't see a reason to discard the various stability improvements.
|
|
I guess the last 2 is already taken care of in latest iteration. |
|
It is a good thing to stabilize the tests and provide additional logging/screenshots at the end, but isn't taking the screenshot out of the mouse is a workaround for the bug? |
|
It probably is but will not affect the regression testing of these tests for its specific JBS issues it was written for. Also, the CI system will have to be kept offline till a product fix is found and tested so I think it is prudent to apply this patch to make this CI system usable again... |
|
ok, it is up to you. |
|
@prrace Do you have any comments? If not, can you please approve? |
|
@prsadhuk This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 360 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
/integrate |
|
Going to push as commit 9160743.
Your commit was automatically rebased without conflicts. |
openjdk
bot
added
integrated
|
I can't see that we did but I have now. |
As per JDK-8252813, some tests fails recurringly in CI macos system. This is an attempt to fix the swing tests.
It was seen from the logs that we have color mismatch in these tests.
For example, in PressedIcon test, we had following log
----------System.out:(1/75)----------
color java.awt.Color[r=55,g=55,b=55] COLOR_1Xjava.awt.Color[r=255,g=0,b=0]
----------System.err:(13/842)----------
java.lang.RuntimeException: Colors is different for scale=1!
at PressedIconTest.main(PressedIconTest.java:97)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:76)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:51)
at java.base/java.lang.reflect.Method.invoke(Method.java:569)
at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
at java.base/java.lang.Thread.run(Thread.java:833)
and JInternalFrame test, we had
----------System.err:(15/939)----------
FRAME_COLOR Red: 255; Green: 200; Blue: 0
Pixel color Red: 55; Green: 55; Blue: 55
java.lang.RuntimeException: Internal frame is not correctly dragged!
at bug8069348.main(bug8069348.java:96)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:76)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:51)
at java.base/java.lang.reflect.Method.invoke(Method.java:569)
at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
at java.base/java.lang.Thread.run(Thread.java:833)
where color is obtained as 55,55,55 instead of required color. The png image generated at failure also did not reveal anything abnormal apart from presence of mouse cursor around the same area where the getPixelColor location is pointing to. And since mouse cursor is also grayish black, it seems robot was wrongly picking up cursor pixels instead of background color.
Modified tests to use location.x-10, location.y-10 to obtain the pixel color. It does not hamper the test as the whole area around the location is coloured with same color in the test so getting pixel color from a bit wide of the centre will not affect the test.
Modified swing tests are passing in the affected system and also in other macos systems and platforms. Link in JBS.
The awt tests that are failing seems to have different root cause and will be handled separately.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6140/head:pull/6140$ git checkout pull/6140Update a local copy of the PR:
$ git checkout pull/6140$ git pull https://git.openjdk.java.net/jdk pull/6140/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 6140View PR using the GUI difftool:
$ git pr show -t 6140Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6140.diff