On Tue, 23 May 2023 02:52:03 GMT, lawrence.andrews <[email protected]> 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
