RFR: 8294535 : Add screen capture functionality to PassFailJFrame

[Alexey Ivanov]

On Wed, 24 May 2023 23:07:11 GMT, Harshitha Onkar <honkar at openjdk.org> wrote:

> Here is a consolidated review and few suggestions -
> 
> Firstly, the use of builder pattern for screenshot capability looks well structured and clean. It is easy for extensibility and does not break the existing functionality.

This is the idea. New tests can use newer syntax, the old tests don't need any modifications.

With a couple more updates which are planned, the usage will be even more streamlined.

> That being said, the screenshot capability can additionally be helpful for automated tests too.

It's a good point. However, taking screenshots doesn't require a lot of code and the screenshot could be limited to the relevant portion of the screen or displayed UI.

> Screenshot capability might be more helpful in case of Client CI / Automated Testing since this feature will make it easier to capture screenshots at different instances when running the test. There are few automated tests currently that capture screenshot when the test fails. The screen capture code is repetitive and redundant and can be changed to use the common functionality that you have designed.

This is one of the problems… A really stand-alone test which has _no dependencies_ is easier to run: compile and run; as of Java 17 you can even skip the compile step and pass the `.java` file directly to `java` launcher which compiles it for you without creating the external `.class` file(s).

If there are dependencies, you have to employ _tricks_ to run the test.

Overall, all GUI tests are somewhat repetitive, tests have duplicate code. It is possible to abstract this into helper classes. In fact, we have such classes: [`SwingTestHelper`](https://github.com/openjdk/jdk/blob/master/test/jdk/javax/swing/regtesthelpers/SwingTestHelper.java), [`JRobot`](https://github.com/openjdk/jdk/blob/master/test/jdk/javax/swing/regtesthelpers/JRobot.java), already mentioned [`Util`](https://github.com/openjdk/jdk/blob/master/test/jdk/javax/swing/regtesthelpers/Util.java)…

I don't say it's bad. I'm for avoiding code duplication. Yet I also appreciate the easiest way possible to run a test. The simpler and easier, the better. It's especially important during code reviews.

> In case it is decided to extend this functionality to automated tests then having this is `regtesthelpers/Util` instead of PassFailJFrame would be more appropriate since PassFailJFrame is more of a manual framework and moving the functionality to Util will look cleaner.

This can easily be done in a follow-up PR.

The only thing that can really be abstracted is the call to `Robot.createScreenCapture` or `Robot.createMultiResolutionScreenCapture` and saving to a file. Well, yes, taking the screenshots of the entire screen can be a utility method.

Then, the jtreg framework does make screenshots in the CI if headful test fails. I've seen it multiple times. They're not always useful because the test UI is usually closed by the time when screenshot is taken. Yet you can see if there are other windows open on the system which could interfere with the test.

> Is the screenshot saved to the test source or jtreg scratch directory? Saving it to scratch would be ideal.

It saves the images to _the current directory_. When the test is run by jtreg, the current directory is the `scratch` directory.

> Additionally you could add the screenshot img filename to the JOptionPane that shows up the "Screenshot successfully captured" message.

There could be multiple files. In fact, I question the need for a modal dialog confirming that the operation completed successfully. If an error occurs, a modal dialog is a good way of notifying. If no error occurs, the message is somewhat redundant.

If the displayed message isn't modal, like a tooltip or a notification which disappears automatically after a certain time, it could be better appreciated. But it's harder to implement.

What could be helpful though is an option to open the current directory in File Explorer, Finder… to allow viewing the collected screenshots. It's more helpful than displaying file names, isn't it?

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

PR Comment: https://git.openjdk.org/jdk/pull/14094#issuecomment-1562735444