RFR: 8339561: The test/jdk/java/awt/Paint/ListRepaint.java may fail after JDK-8327401

[Alexey Ivanov]

On Wed, 4 Sep 2024 20:50:38 GMT, Sergey Bylokhov <serb at openjdk.org> 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