8327856: Convert applet test SpanishDiacriticsTest.java to a main program by prrace · Pull Request #18208 · openjdk/jdk

prrace · 2024-03-11T23:02:53Z

Converts the manual applet test in the files ..
SpanishDiacriticsTest/SpanishDiacriticsTest.html
SpanishDiacriticsTest/SpanishDiacriticsTest.java
.. to a main program using PassFailJFrame

Added requires windows since it seems very windows-specific.

I got rid of the redundant extra directory which makes the .java file "new" according to github but it is largely changed anyway

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8327856: Convert applet test SpanishDiacriticsTest.java to a main program (Bug - P4)

Reviewers

Dmitry Markov (@dmarkov20 - Reviewer) ⚠️ Review applies to a2cb3493
Alexey Ivanov (@aivanov-jdk - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18208/head:pull/18208
$ git checkout pull/18208

Update a local copy of the PR:
$ git checkout pull/18208
$ git pull https://git.openjdk.org/jdk.git pull/18208/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18208

View PR using the GUI difftool:
$ git pr show -t 18208

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18208.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-03-11T23:03:36Z

👋 Welcome back prr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-03-11T23:04:11Z

@prrace The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-03-11T23:07:24Z

Webrevs

openjdk · 2024-03-12T10:09:11Z

@prrace This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8327856: Convert applet test SpanishDiacriticsTest.java to a main program

Reviewed-by: dmarkov, aivanov

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 23 new commits pushed to the master branch:

94b4ed5: 8327096: (fc) java/nio/channels/FileChannel/Size.java fails on partition incapable of creating large files
b9c3dc3: 8327738: Remove unused internal method sun.n.w.p.h.HttpURLConnection.setDefaultAuthenticator
5b41466: 8327729: Remove deprecated xxxObject methods from jdk.internal.misc.Unsafe
313e814: 8324682: Remove redefinition of NULL for XLC compiler
8a3bdd5: 8327995: Remove unused Unused_Variable
201042f: 8327487: Further augment WorstCaseTests with more cases
379ad1f: 8312444: Delete unused parameters and variables in SocketPermission
782206b: 8327748: Convert javax/swing/JFileChooser/6798062/bug6798062.java applet test to main
0776fff: 8327794: RISC-V: enable extension features based on impid (Rivos specific change)
cfd9209: 8327751: Convert javax/swing/JInternalFrame/6726866/bug6726866.java applet test to main
... and 13 more: https://git.openjdk.org/jdk/compare/0a6e64e2f50cb5eb969dbd0807ad3edc1c353923...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

aivanov-jdk · 2024-03-12T12:47:46Z

test/jdk/java/awt/InputMethods/SpanishDiacriticsTest.java

+          that they take several seconds to display. That is an
+          expected behaviour for this test.
+
+       If the text field displays ''o, (i.e. o should be without an acute)


Suggested change

If the text field displays ''o, (i.e. o should be without an acute)

If the text field displays \u00f3, (i.e. o should be without an acute)

Use the real character?

It isn't a single char. It is as written - the sequence of 3 characters - "single quote" "single quote", "latin lower case o"

I misread it. If the text is ''o instead of ó…

What if we spell the conditions explicitly:

The text field should display ''o (i.e. two apostrophes and o without an acute), press Pass.
If you see ó (i.e. o with an acute), press Fail.

I will reword it but the text is the same as was in the HTML file with just some minor grammatical changes in the surrounding text.

Making the instructions clearer helps to avoid confusion. Thank you for updating them, looks clearer now.

aivanov-jdk · 2024-03-12T12:50:16Z

test/jdk/java/awt/InputMethods/SpanishDiacriticsTest.java

+        frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
+        frame.pack();
+        frame.setVisible(true);


Suggested change

frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);

frame.pack();

frame.setVisible(true);

frame.pack();

Calling setVisible(true) here causes flickering as the frame gets shown at its default location and later gets moved to its final location.

Setting close operation is redundant, PassFailJFrame handles closing test UI (when you use the builder pattern) and fails the test if it's closed.

Calling setVisible(true) does not affect functionality, purely aesthetic effect to avoid window flickering.

aivanov-jdk · 2024-03-12T12:51:12Z

test/jdk/java/awt/InputMethods/SpanishDiacriticsTest.java

+                      .instructions(INSTRUCTIONS)
+                      .rows(18)
+                      .columns(50)
+                      .testUI(() -> createTestUI())


Suggested change

.testUI(() -> createTestUI())

.testUI(SpanishDiacriticsTest::createTestUI)

I prefer method references in such cases, it looks cleaner.

aivanov-jdk · 2024-03-12T12:51:31Z

test/jdk/java/awt/InputMethods/SpanishDiacriticsTest.java

+                      .rows(18)
+                      .columns(50)
+                      .testUI(() -> createTestUI())
+                      .testTimeOut(5)


Default timeout could be omitted.

aivanov-jdk

Looks good to me except for setVisible(true) in createUI.

prrace · 2024-03-12T18:27:00Z

/integrate

openjdk · 2024-03-12T18:27:26Z

Going to push as commit 22f10e0.
Since your change was applied there have been 25 commits pushed to the master branch:

7283c8b: 8327972: Convert java/awt/FileDialog/SaveFileNameOverrideTest/SaveFileNameOverrideTest.html applet test to main
30249c4: 8327838: Convert java/awt/FileDialog/MultipleMode/MultipleMode.html applet test to main
94b4ed5: 8327096: (fc) java/nio/channels/FileChannel/Size.java fails on partition incapable of creating large files
b9c3dc3: 8327738: Remove unused internal method sun.n.w.p.h.HttpURLConnection.setDefaultAuthenticator
5b41466: 8327729: Remove deprecated xxxObject methods from jdk.internal.misc.Unsafe
313e814: 8324682: Remove redefinition of NULL for XLC compiler
8a3bdd5: 8327995: Remove unused Unused_Variable
201042f: 8327487: Further augment WorstCaseTests with more cases
379ad1f: 8312444: Delete unused parameters and variables in SocketPermission
782206b: 8327748: Convert javax/swing/JFileChooser/6798062/bug6798062.java applet test to main
... and 15 more: https://git.openjdk.org/jdk/compare/0a6e64e2f50cb5eb969dbd0807ad3edc1c353923...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-03-12T18:27:31Z

@prrace Pushed as commit 22f10e0.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8327856

a2cb349

openjdk bot changed the title ~~8327856~~ 8327856: Convert applet test SpanishDiacriticsTest.java to a main program Mar 11, 2024

openjdk bot added the rfr Pull request is ready for review label Mar 11, 2024

openjdk bot added the client client-libs-dev@openjdk.org label Mar 11, 2024

dmarkov20 approved these changes Mar 12, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 12, 2024

aivanov-jdk reviewed Mar 12, 2024

View reviewed changes

prrace added 2 commits March 12, 2024 10:31

8327856

b56d817

8327856

7fb3fe7

aivanov-jdk approved these changes Mar 12, 2024

View reviewed changes

8327856

73aa1b7

aivanov-jdk approved these changes Mar 12, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 12, 2024

openjdk bot closed this Mar 12, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 12, 2024

	If the text field displays ''o, (i.e. o should be without an acute)
	If the text field displays \u00f3, (i.e. o should be without an acute)

	.testUI(() -> createTestUI())
	.testUI(SpanishDiacriticsTest::createTestUI)

Conversation

prrace commented Mar 11, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Mar 11, 2024

Uh oh!

openjdk bot commented Mar 11, 2024

Uh oh!

mlbridge bot commented Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

openjdk bot commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk left a comment

Choose a reason for hiding this comment

Uh oh!

prrace commented Mar 12, 2024

Uh oh!

openjdk bot commented Mar 12, 2024

Uh oh!

openjdk bot commented Mar 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

prrace commented Mar 11, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 11, 2024 •

edited

Loading

openjdk bot commented Mar 12, 2024 •

edited

Loading

aivanov-jdk Mar 12, 2024 •

edited

Loading