8353138: Screen capture for test TaskbarPositionTest.java, failure case #24286
Conversation
|
👋 Welcome back rkannathpari! A progress list of the required criteria for merging this PR into |
|
@Renjithkannath 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 106 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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@mrserb, @aivanov-jdk) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
@Renjithkannath The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
There was a problem hiding this comment.
Just a few minor questions.
Also, it seems that the screenshot code is repeated all over the place. Wouldn't it make sense to make a utility/add it to an existing utility class for further use to standardise it and make it simpler to maintain in time? I know this is well beyond the scope of this ticket, but just thought to mention the idea 😃
BufferedImage image = robot.createScreenCapture(bounds);
try {
ImageIO.write(image,"png", new File("Screenshot.png"));
} catch (IOException e) {
...| @@ -74,6 +78,7 @@ public class TaskbarPositionTest implements ActionListener { | |||
| private static JFrame frame; | |||
| private static JPopupMenu popupMenu; | |||
| private static JPanel panel; | |||
| private static Robot robot; | |||
There was a problem hiding this comment.
Do you think it would be better to have Robot initialisation in this way? IMO it will make the robot final, so there is no way to overwrite it in the future as well as it would be easier to debug and not be initialised in the main method of the class.
private static final Robot robot;
static {
try {
robot = new Robot();
} catch (AWTException e) {
throw new RuntimeException(e);
}
}There was a problem hiding this comment.
Since its a test, I didn't expect any override of this class in future, my intention is add this feature with minimal change.
There was a problem hiding this comment.
I agree with Renjith.
If you call saveScreenCapture once in the main method as I suggest, you can pass robot to saveScreenCapture, in which case robot can remain local variable.
| try { | ||
| ImageIO.write(image,"png", new File("Screenshot.png")); | ||
| } catch (IOException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
I'm not sure it's the best idea to hide the error and just print it into stdout. Wouldn't it be better to just throw the error, what do you think?
There was a problem hiding this comment.
I don't think its a good idea
- IOException was not the original intention of this test and its only for additional information
- If we throw the error we will miss the actual exception, potentially that was the next instruction.
There was a problem hiding this comment.
No, we don't care about IOException. It can occur only after the test already failed, and it's important to preserve the original exception.
| @@ -74,6 +78,7 @@ public class TaskbarPositionTest implements ActionListener { | |||
| private static JFrame frame; | |||
There was a problem hiding this comment.
Nit: I don't think * @run main TaskbarPositionTest is needed. But it's fine as it is too, don't have any strong feelings
There was a problem hiding this comment.
It may not work without explicit @run because the test contains @build tags.
@run is implied if there are no other commands, such as @compile or @build, but it's not the case in this test.
| @@ -74,6 +78,7 @@ public class TaskbarPositionTest implements ActionListener { | |||
| private static JFrame frame; | |||
| try { | ||
| ImageIO.write(image,"png", new File("Screenshot.png")); | ||
| } catch (IOException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
No, we don't care about IOException. It can occur only after the test already failed, and it's important to preserve the original exception.
| @@ -74,6 +78,7 @@ public class TaskbarPositionTest implements ActionListener { | |||
| private static JFrame frame; | |||
| private static JPopupMenu popupMenu; | |||
| private static JPanel panel; | |||
| private static Robot robot; | |||
There was a problem hiding this comment.
I agree with Renjith.
If you call saveScreenCapture once in the main method as I suggest, you can pass robot to saveScreenCapture, in which case robot can remain local variable.
| @@ -74,6 +78,7 @@ public class TaskbarPositionTest implements ActionListener { | |||
| private static JFrame frame; | |||
There was a problem hiding this comment.
It may not work without explicit @run because the test contains @build tags.
@run is implied if there are no other commands, such as @compile or @build, but it's not the case in this test.
Hi Reviewers,
Added screen capture in case of test failure using Robot.
Please review and let me know your suggestion if any.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24286/head:pull/24286$ git checkout pull/24286Update a local copy of the PR:
$ git checkout pull/24286$ git pull https://git.openjdk.org/jdk.git pull/24286/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 24286View PR using the GUI difftool:
$ git pr show -t 24286Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24286.diff
Using Webrev
Link to Webrev Comment