On Wed, 6 Nov 2024 23:59:24 GMT, Phil Race <[email protected]> wrote: > This test has had reports of failures over the years notably on Linux. > I think the large number of XVisuals may have something to do with it. > I've updated it quite a bit and limited the number of dialogs it creates - we > really don't need to test 300 of them > This new version has passed hundreds of iterations.
Changes requested by aivanov (Reviewer). test/jdk/java/awt/Graphics2D/ScaledTransform/ScaledTransform.java line 38: > 36: /* > 37: * @test > 38: * @bug 8069362 Is it accidental or intentional? [JDK-8069361](https://bugs.openjdk.org/browse/JDK-8069361): _SunGraphics2D.getDefaultTransform() does not include scale factor_ is the correct bug. The new bug id, JDK-8069362, seems unrelated. test/jdk/java/awt/Graphics2D/ScaledTransform/ScaledTransform.java line 49: > 47: private static volatile Dialog dialog; > 48: private static volatile long startTime = 0; > 49: private static volatile long endTime = 0; Initialisers aren't needed: they set fields to their default values, and the values are reset before each test. test/jdk/java/awt/Graphics2D/ScaledTransform/ScaledTransform.java line 57: > 55: if (ge.isHeadlessInstance()) { > 56: return; > 57: } Shall we remove `ge.isHeadlessInstance()`? The test is marked with `@key headful`, we can let it fail in headless environment. test/jdk/java/awt/Graphics2D/ScaledTransform/ScaledTransform.java line 92: > 90: > 91: private static void showDialog(final GraphicsConfiguration gc) { > 92: Can you remove this blank line at the start of the method, please? test/jdk/java/awt/Graphics2D/ScaledTransform/ScaledTransform.java line 93: > 91: private static void showDialog(final GraphicsConfiguration gc) { > 92: > 93: System.out.println("Creating dialog for gc="+gc+" with > tx="+gc.getDefaultTransform()); Suggestion: System.out.println("Creating dialog for gc=" + gc + " with tx=" + gc.getDefaultTransform()); test/jdk/java/awt/Graphics2D/ScaledTransform/ScaledTransform.java line 95: > 93: System.out.println("Creating dialog for gc="+gc+" with > tx="+gc.getDefaultTransform()); > 94: > 95: dialog = new Dialog((Frame) null, "Test", true, gc); Suggestion: dialog = new Dialog((Frame) null, "ScaledTransform", true, gc); Set a unique title. test/jdk/java/awt/Graphics2D/ScaledTransform/ScaledTransform.java line 97: > 95: dialog = new Dialog((Frame) null, "Test", true, gc); > 96: > 97: dialog.setSize(100, 100); Suggestion: dialog.setSize(300, 100); Larger width ensures the longer title is fully seen and not truncated. test/jdk/java/awt/Graphics2D/ScaledTransform/ScaledTransform.java line 108: > 106: System.out.println("GTX = " + gTx); > 107: passed = (gcTx.getScaleX() == gTx.getScaleX()) && > 108: (gcTx.getScaleY() == gTx.getScaleY()); Suggestion: passed = (gcTx.getScaleX() == gTx.getScaleX()) && (gcTx.getScaleY() == gTx.getScaleY()); [Java Code Conventions](https://www.oracle.com/technetwork/java/codeconventions-150003.pdf)¹ recommend wrapping before the binary operator; if a line starts with an operator, it is clearly a continuation line rather than a new statement. ¹ Section 4.2 Wrapping Lines, bullet 2: _“Break before an operator.”_ test/jdk/java/awt/Graphics2D/ScaledTransform/ScaledTransform.java line 113: > 111: } > 112: painted.countDown(); > 113: endTime = System.currentTimeMillis(); Suggestion: endTime = System.currentTimeMillis(); painted.countDown(); Both are volatile, yet it may still be possible that the main thread read a stale value of `endTime` before it's updated on EDT. test/jdk/java/awt/Graphics2D/ScaledTransform/ScaledTransform.java line 126: > 124: dialog.setVisible(false); > 125: dialog.dispose(); > 126: } Suggestion: if (dialog != null) { System.out.println("Disposing dialog"); dialog.setVisible(false); dialog.dispose(); } Amend indentation to 4 spaces as everywhere. ------------- PR Review: https://git.openjdk.org/jdk/pull/21942#pullrequestreview-2420639676 PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1832483434 PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1832630857 PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1832602281 PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1832605490 PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1832495798 PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1832488803 PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1832490104 PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1832616170 PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1832610325 PR Review Comment: https://git.openjdk.org/jdk/pull/21942#discussion_r1832613055
