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

[Kevin Rushforth]

On Fri, 4 Apr 2025 20:42:03 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> 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?
>
> To save to a file, or send to a stream, for example.  This is a general purpose test utility.  The javadoc says "in PNG format".

OK, I see that now. You might say that it's a PNG in the `@return` statement (like you do for the other methods). Maybe also add ", returning it as a byte array" at the end of the first sentence?

>> 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.
>
> I wanted to avoid creating multiple multi-megabyte strings.
> 
> The method to use is writeScreenshot(), which should be simple enough to use, it's the method mentioned at the class level javadoc.
> 
> Related: I was debating whether we should have two specific methods
> 
> writeScreenshotStdout();
> writeSceeenshotStderr();
> 
> instead of passing `System:out` or `System::err`.  What do you think?

My suggestion doesn't change anything about how many or what size strings are allocated in the common case. All it does is moves which method tacks on the needed "data:image/png;base64," piece from the caller to the method that actually creates the PNG.

As for two methods vs one, I like the flexibility as you have it, although really only the System.err version is likely to be used (with gradle as the test runner, writing it to stdout isn't generally as useful).

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1746#discussion_r2029424186
PR Review Comment: https://git.openjdk.org/jfx/pull/1746#discussion_r2029429884