RFR: 8353138: Screen capture for test TaskbarPositionTest.java, failure case [v2]

[Alexey Ivanov]

On Tue, 1 Apr 2025 11:16:01 GMT, Renjith Kannath Pariyangad <rkannathpari at openjdk.org> wrote:

>> Hi Reviewers,
>> 
>> Added screen capture in case of test failure using Robot. 
>> 
>> Please review and let me know your suggestion if any.
>
> Renjith Kannath Pariyangad has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Updated copyright year

Changes requested by aivanov (Reviewer).

test/jdk/javax/swing/Popup/TaskbarPositionTest.java line 218:

> 216: 
> 217:     // for debugging purpose, saves screen capture when test fails.
> 218:     private static void saveScreenCapture(Rectangle bounds) {

The comment is redundant.

I propose to move this method to the bottom of the class — it's the least important piece of code in the entire test class.

Modify the `saveScreenCapture` method to save a screenshot of all the screens.

test/jdk/javax/swing/Popup/TaskbarPositionTest.java line 221:

> 219:         BufferedImage image = robot.createScreenCapture(bounds);
> 220:         try {
> 221:             ImageIO.write(image,"png", new File("Screenshot.png"));

Suggestion:

            ImageIO.write(image, "png", new File("Screenshot.png"));

Space after the comma.

test/jdk/javax/swing/Popup/TaskbarPositionTest.java line 234:

> 232:             saveScreenCapture(checkBounds);
> 233:             throw new RuntimeException("Popup not visible");
> 234:         }

I don't really like this approach, you're modifying several places where an exception can be thrown, but there are other places.

I suggest adding a catch section into the `main` method:


            robot.waitForIdle();
        } catch (Throwable t) {
            saveScreenCapture(screens);
            throw t;
        } finally {
            SwingUtilities.invokeAndWait(() -> {


This way you capture screenshot in one place and it automatically covers *all the cases* where an error is thrown, which avoids adding explicit calls to save the screenshots.

You may pass `robot` too.

I think you should make a screenshot of all the screens.

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

PR Review: https://git.openjdk.org/jdk/pull/24286#pullrequestreview-2739457459
PR Review Comment: https://git.openjdk.org/jdk/pull/24286#discussion_r2026773666
PR Review Comment: https://git.openjdk.org/jdk/pull/24286#discussion_r2026774176
PR Review Comment: https://git.openjdk.org/jdk/pull/24286#discussion_r2026768384