On Thu, 25 May 2023 11:24:42 GMT, Alexey Ivanov <aiva...@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
