RFR: 8294535 : Add screen capture functionality to PassFailJFrame
Harshitha Onkar
honkar at openjdk.org
Wed May 24 23:09:54 UTC 2023
On Tue, 23 May 2023 02:52:03 GMT, lawrence.andrews <duke at openjdk.org> wrote:
> 1) Used builder pattern
> 2) Tested AWT tests and it passed
@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.
-------------
PR Review: https://git.openjdk.org/jdk/pull/14094#pullrequestreview-1442859692
More information about the client-libs-dev
mailing list