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

[Kevin Rushforth]

On Fri, 28 Mar 2025 18:22:56 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.
> 
> A possible improvement could be to output a data URL
> 
> `data:image/png;base64,iVBORw0KGgoAAAANSUhEU...`
> 
> so it can be rendered in Safari (Chrome truncates the image possibly due to following a url length limit)

I haven't tested it yet, but look forward to doing so.

My main comment is that this needs to be 'opt in' on a gradle property. Something like this (probably with a better name) :


gradle ... -PUSE_ROBOT=true -PSCREEN_DUMP=true


Or if we can figure out a good way to limit the number of tests that will be captured:


gradle ... -PUSE_ROBOT=true -PSCREEN_DUMP_LIMIT=10

tests/system/src/test/java/test/robot/javafx/scene/control/behavior/TextAreaBehaviorRobotTest.java line 40:

> 38:  * since the mapped functions require rendered text.
> 39:  */
> 40: @ExtendWith(ScreenCaptureTestWatcher.class)

A JUnit5 annotation seams like an easy way to mark our robot tests for capture. Are there any drawbacks with this approach?

tests/system/src/test/java/test/util/ScreenCaptureTestWatcher.java line 44:

> 42: 
> 43: /**
> 44:  * Standard Test Watcher for Headful Tests (JUnit 5 Only).

I think the "JUnit 5 only" comment isn't needed. All of our system tests already use only JUnit 5.

tests/system/src/test/java/test/util/ScreenCaptureTestWatcher.java line 47:

> 45:  * <p>
> 46:  * This facility takes a screenshot of any failed test, then logs the base64-encoded screenshot
> 47:  * to {@code stderr}.

This should not be on by default, but should be "opt in" on a System property. I recommend defining a gradle property to do this and map it to that System property, much like we do for "UNSTABLE_TEST" and others.

tests/system/src/test/java/test/util/ScreenCaptureTestWatcher.java line 62:

> 60:  * -ea
> 61:  * }</pre>
> 62:  * <p>

I don't much like IDE-specific documentation in our class files.

tests/system/src/test/java/test/util/ScreenCaptureTestWatcher.java line 64:

> 62:  * <p>
> 63:  * WARNING: using this utility may significantly increase the size of your logs!
> 64:  * Make sure there is plenty of free disk space.

Given my earlier comment about this needing to be qualified by a system property, this comment should also go in `build.gradle` where the property is defined.

tests/system/src/test/java/test/util/ScreenCaptureTestWatcher.java line 67:

> 65:  */
> 66: // TODO investigate having a hard-coded or programmable via property limit on the number of
> 67: // captured screenshots.

I had the same thought. Given that each test runs in its own JVM, I can't think of a good way to do it other than recording information in a file in tests/system/build.

tests/system/src/test/java/test/util/ScreenCaptureTestWatcher.java line 71:

> 69:     @Override
> 70:     public void testAborted(ExtensionContext extensionContext, Throwable err) {
> 71:         err.printStackTrace();

Is this needed or is it just for debugging? We already get a stack trace when a test fails, so it might be redundant.

tests/system/src/test/java/test/util/ScreenCaptureTestWatcher.java line 76:

> 74:     @Override
> 75:     public void testFailed(ExtensionContext extensionContext, Throwable err) {
> 76:         err.printStackTrace();

Same question as about about printing the stack trace.

tests/system/src/test/java/test/util/ScreenCaptureTestWatcher.java line 77:

> 75:     public void testFailed(ExtensionContext extensionContext, Throwable err) {
> 76:         err.printStackTrace();
> 77:         System.err.println(generateScreenshot("Screenshot:{", "}"));

When does the `testFailed` method run? After a failing `@Test` method that throws the exception? What if there is more than one failing test? Ideally what we want is something that runs after all failing tests, but before the `@AfterEach` method.

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

PR Review: https://git.openjdk.org/jfx/pull/1746#pullrequestreview-2726765623
PR Review Comment: https://git.openjdk.org/jfx/pull/1746#discussion_r2019373251
PR Review Comment: https://git.openjdk.org/jfx/pull/1746#discussion_r2019375500
PR Review Comment: https://git.openjdk.org/jfx/pull/1746#discussion_r2019376822
PR Review Comment: https://git.openjdk.org/jfx/pull/1746#discussion_r2019377220
PR Review Comment: https://git.openjdk.org/jfx/pull/1746#discussion_r2019381676
PR Review Comment: https://git.openjdk.org/jfx/pull/1746#discussion_r2019385645
PR Review Comment: https://git.openjdk.org/jfx/pull/1746#discussion_r2019386416
PR Review Comment: https://git.openjdk.org/jfx/pull/1746#discussion_r2019392578
PR Review Comment: https://git.openjdk.org/jfx/pull/1746#discussion_r2019408150