On Wed, 4 Sep 2024 20:50:38 GMT, Sergey Bylokhov <[email protected]> wrote:
> Several tests modified by https://github.com/openjdk/jdk/pull/19339 have been > tweaked, see inline comments. > > Notes: > * We have a few XXXRepaint.java tests and in this patch, I updated all of > them to follow the change added to the ListRepaint.java > > @azvegint @aivanov-jdk please take a look. Changes requested by aivanov (Reviewer). test/jdk/java/awt/List/KeyEventsTest/KeyEventsTest.java line 53: > 51: > 52: public class KeyEventsTest { > 53: private volatile TestState currentState; I'd rather not declare it a `volatile`. It could give a false impression of being thread-safe but it is not. The `volatile` modifier has a meaning only when it's written and read subsequently. If the reference doesn't change, it has no effect on the visibility of the internal object state. The value is assigned to `currentState` while holding a lock `LOCK`. At the same time, `currentState.setAction(true)` is called without any synchronisation and adding `volatile` won't make the change of the state thread-safe. test/jdk/java/awt/Paint/ButtonRepaint.java line 35: > 33: public final class ButtonRepaint extends Button { > 34: > 35: public static void main(final String[] args) { Could expand imports, remove `@author` tag and the second `*` from `/**`? test/jdk/java/awt/Paint/CheckboxRepaint.java line 34: > 32: public final class CheckboxRepaint extends Checkbox { > 33: > 34: public static void main(final String[] args) { Could expand imports, remove `@author` tag and the second `*` from `/**`? test/jdk/java/awt/Paint/LabelRepaint.java line 37: > 35: public final class LabelRepaint extends Label { > 36: > 37: public static void main(final String[] args) { Could you remove `@author` tag, update copyright year? test/jdk/java/awt/Paint/ListRepaint.java line 35: > 33: * @author Sergey Bylokhov > 34: */ > 35: public final class ListRepaint extends List { Could you remove `@author` tag and the second `*` from `/**`? test/jdk/javax/swing/JButton/bug4490179.java line 56: > 54: try { > 55: SwingUtilities.invokeAndWait(() -> { > 56: frame = new JFrame("bug4490179"); Instead of three volatile fields — `pt`, `buttonW`, `buttonH` — there could be only one: SwingUtilities.invokeAndWait(() -> { Point loc = button.getLocationOnScreen(); Dimension size = button.getSize(); pt = new Point(loc.x + size.width / 2, loc.y + size.height / 2); }); I think this better conveys the idea, less variables / fields to track in your mind. test/jdk/javax/swing/JButton/bug4490179.java line 61: > 59: button.addActionListener(new ActionListener() { > 60: public void actionPerformed(ActionEvent e) { > 61: passed = false; Maybe use two `CountDownLatch`es: if ((e.getModifiers() & InputEvent.BUTTON1_MASK) != InputEvent.BUTTON1_MASK) { wrongMouseButton.countDown(); } else { mouseButton1.countDown(); } Then in main: robot.mouseRelease(InputEvent.BUTTON3_DOWN_MASK); // robot.delay(3000); if (wrongMouseButton.await(2, TimeUnit.SECONDS)) { // Restore robot state robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK); throw new RuntimeException("ActionEvent delivered from mouse button 3"); } robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK); if (mouseButton1.await(2, TimeUnit.SECONDS)) { throw new RuntimeException("ActionEvent not delivered from mouse button 1"); } test/jdk/javax/swing/JButton/bug4490179.java line 62: > 60: public void actionPerformed(ActionEvent e) { > 61: passed = false; > 62: } The updated code here is correct; Alexander has restored the condition that was present in the original test which was closed-source and manual. However, it may be not necessary. Instead, the test should fail immediately if `ActionEvent` contains anything but `BUTTON1_DOWN_MASK`. (Well, cleaning up robot state to release mouse buttons is a good thing to do.) test/jdk/javax/swing/JButton/bug4490179.java line 89: > 87: robot.mouseRelease(InputEvent.BUTTON1_DOWN_MASK); > 88: > 89: if (!result) { Perhaps, we should ensure that an `ActionEvent` gets delivered at this point. Is it expected or not? test/jdk/javax/swing/plaf/basic/BasicMenuUI/4983388/bug4983388.java line 97: > 95: }); > 96: > 97: if (!bMenuSelected) { Could you also expand imports and remove `@author` tag? ------------- PR Review: https://git.openjdk.org/jdk/pull/20861#pullrequestreview-2290709302 PR Review Comment: https://git.openjdk.org/jdk/pull/20861#discussion_r1750819337 PR Review Comment: https://git.openjdk.org/jdk/pull/20861#discussion_r1750830425 PR Review Comment: https://git.openjdk.org/jdk/pull/20861#discussion_r1750831557 PR Review Comment: https://git.openjdk.org/jdk/pull/20861#discussion_r1750833000 PR Review Comment: https://git.openjdk.org/jdk/pull/20861#discussion_r1750842554 PR Review Comment: https://git.openjdk.org/jdk/pull/20861#discussion_r1750877763 PR Review Comment: https://git.openjdk.org/jdk/pull/20861#discussion_r1750872680 PR Review Comment: https://git.openjdk.org/jdk/pull/20861#discussion_r1750863160 PR Review Comment: https://git.openjdk.org/jdk/pull/20861#discussion_r1750857895 PR Review Comment: https://git.openjdk.org/jdk/pull/20861#discussion_r1750879942
