On Fri, 28 Mar 2025 18:22:56 GMT, Andy Goryachev <ango...@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
