RFR: 8294535 : Add screen capture functionality to PassFailJFrame

[Harshitha Onkar]

On Thu, 25 May 2023 11:24:42 GMT, Alexey Ivanov <aivanov at openjdk.org> wrote:

>> @lawrence-andrew 
>> 
>> 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. That being said, the screenshot capability can additionally be helpful for automated tests too.
>> 
>> - 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. 
>> 
>> - 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.
>> 
>> - Is the screenshot saved to the test source or jtreg scratch directory? Saving it to scratch would be ideal.
>> 
>> - Additionally you could add the screenshot img filename to the JOptionPane that shows up the "Screenshot successfully captured" message.
>
>> 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-...

@aivanov-jdk 
> 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.

Sounds good.

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

For me, the generated screenshot was being saved to the test folder, as I was running it as standalone test. I had issues running it as jtreg test, but this is probably due to some config issue on my end and nothing related to code changes.

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

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