RFR: 8328716: [TestBug] Screen capturing utility for failed tests [v6]

[Kevin Rushforth]

On Wed, 2 Apr 2025 15:04:54 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Introduce a facility, in the form of JUnit5 annotation, to allow for capturing a desktop screenshot of a failed test.
>> 
>> The primary intent is to be able to debug an intermittent test case, rather than wholesale addition of the new annotation to all the tests.
>> 
>> The log contains a base-64 encoded screenshot (like this: `data:image/png;base64,iVBORw0KGgoAAAANSUhEU...` )
>> so it can be rendered in Safari (Chrome truncates the image possibly due to following a url length limit)
>> 
>> Additionally, provided a utility class to capture the screenshots at the specific point in a test and write it to stdout/stderr:
>> 
>> 
>> // write a base-64 encoded screenshot to stderr
>> ScreenshotCapture.writeScreenshot(System.err);
>> 
>> 
>> Example:
>> 
>> ![jenkins-screenshot](https://github.com/user-attachments/assets/abebd76f-747a-4d6d-a9a6-63c6e9426830)
>
> Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   javadoc

The functionality looks good, and I confirm that there are now no uses of the new utility API or test watcher in our check-in tests. I left a couple comments on the API for the test methods.

One other thing that might be helpful is class docs that describe the two different ways of using it (the watcher annotation and the direct call to the utility method).

tests/system/src/test/java/test/util/ScreenshotCapture.java line 60:

> 58:      * @throws IOException when an I/O error occurs
> 59:      */
> 60:     public static byte[] takeScreenshot() throws IOException {

I have no idea how to interpret this byte array. What is the use case for this method?

tests/system/src/test/java/test/util/ScreenshotCapture.java line 93:

> 91:      */
> 92:     public static void writeScreenshot(PrintStream out) {
> 93:         out.println(ScreenshotCapture.takeScreenshotBase64("Screenshot:\ndata:image/png;base64,", null));

With the change I am suggesting to `takeScreenshotBase64 `, this would become:


        out.println("Screenshot:");
        out.println(takeScreenshotBase64());


That makes the `takeScreenshotBase64` API cleaner with a better separation of concerns.

tests/system/src/test/java/test/util/ScreenshotCapture.java line 107:

> 105:      * @return the screenshot in Base-64 encoded PNG, or an error message
> 106:      */
> 107:     public static String takeScreenshotBase64(String prefix, String postfix) {

Passing in the `data:image/png;base64,` as a prefix is odd. This method takes a screen shot and turns it into a base64-encoded PNG. Don't make the caller pass in something that has to match this internal behavior of this method. I would recommend removing the prefix and postfix entirely, and having this method return a simple base64-encoded PNG with a `data:image/png;base64,` data URL. The caller can add any other String before or after this.

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

PR Review: https://git.openjdk.org/jfx/pull/1746#pullrequestreview-2744035824
PR Comment: https://git.openjdk.org/jfx/pull/1746#issuecomment-2779684965
PR Review Comment: https://git.openjdk.org/jfx/pull/1746#discussion_r2029394657
PR Review Comment: https://git.openjdk.org/jfx/pull/1746#discussion_r2029401366
PR Review Comment: https://git.openjdk.org/jfx/pull/1746#discussion_r2029399198