8347359: RichTextArea API Tests #1677

andy-goryachev-oracle · 2025-01-14T19:44:01Z

Additional RichTextArea API tests.

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-8347359: RichTextArea API Tests (Bug - P4)

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1677

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1677.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-01-14T19:44:48Z

👋 Welcome back angorya! 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 · 2025-01-14T19:45:28Z

@andy-goryachev-oracle 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:

8347359: RichTextArea API Tests

Reviewed-by: kcr

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 3 new commits pushed to the master branch:

1a12966: 8354478: Improve StageStyle documentation
4df2326: 8352162: Update libxml2 to 2.13.8
46b36fe: 8354876: Update SQLite to 3.49.1

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

8347359.rta.api.tests

openjdk · 2025-01-28T17:46:28Z

⚠️ @andy-goryachev-oracle This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

…7359.rta.api.tests

andy-goryachev-oracle · 2025-01-29T16:42:04Z

one reviewer is probably fine, but feel free to take a look everyone.

mlbridge · 2025-01-29T16:45:22Z

Webrevs

09: Full - Incremental (8d3f3ecd)
08: Full (4f07d5f2)
07: Full (0fb16fdc)
06: Full (0adcd2a5)
05: Full (f78f5415)
04: Full (c0cde4a9)
03: Full (b8e59bd7)
02: Full (b3a60874)
01: Full (d6e9ba3f)
00: Full (25845fa8)

bridgekeeper · 2025-02-26T21:54:55Z

@andy-goryachev-oracle This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

andy-goryachev-oracle · 2025-02-26T21:55:50Z

keep-alive

bridgekeeper · 2025-03-27T01:48:21Z

@andy-goryachev-oracle This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

andy-goryachev-oracle · 2025-03-27T14:21:15Z

@arapte or @Ziad-Mid please review

kevinrushforth

First batch of comments. This looks like a good start for RichTextArea and CodeArea. I presume you'll handle the rest of the API with other bugs / PRs?

Some of my comments are about expanding the test coverage to handle more cases, and that could be done when you add functional tests.

kevinrushforth · 2025-04-22T20:36:55Z

modules/jfx.incubator.richtext/src/test/java/test/jfx/incubator/scene/util/TUtil.java

+            if (throwable instanceof RuntimeException) {
+                throw (RuntimeException)throwable;


Why is RuntimeException treated differently than checked Exception and Error?

hmmm... This pattern was copied from some other test. We have 32 more instances of this pattern elsewhere.

I suspect it is not avoid declaring a checked exception.
What would you suggest? Forward the throwable to Thread.currentThread().getThreadGroup().uncaughtException(thread, throwable); unconditionally?

That's interesting that other tests do this. When re-throwing an exception you often need to special case checked exceptions (to wrap them so that the caller doesn't need to explicitly declare it), but the UncaughtExceptionHandler can handle both, so I don't know why it was done that way.

You might try just unconditionally forwarding all throwables, but not sure.

Turns out, this is required by junit framework to properly handle the failed tests.

If we pass all the Throwables to the thread group, then a RuntimeException thrown from a @Test does not mark the unit test as failed, though the stacktrace does appear in stderr; a thrown Error will mark the test as failed.

kevinrushforth · 2025-04-22T20:57:46Z

modules/jfx.incubator.richtext/src/test/java/test/jfx/incubator/scene/util/TUtil.java

+import javafx.beans.property.SimpleObjectProperty;
+
+/**
+ * There should be a common place for module-agnostic test utilities.


javafx.base might eventually be a good place for this sort of utility.

It might, or perhaps it could be packaged into a jar so we can run single source files + library via JEP 458 --class-path 'libs/*' argument?

see "Using pre-compiled classes" section in
https://openjdk.org/jeps/458

Unit tests already need to load the JavaFX modules, so I don't see how packaging it in a jar would help. All it would do it add one more thing to put on the path.

I meant the single-file manual tests, continuing discussion in #1747

Ah. I didn't get that from the context of this discussion, since we are discussing automated tests in this PR.

I'm not sure there is enough value in sharing utilities between manual tests and our JUnit-based automated tests for us to want to deal with the complexities that it would bring, but we can discuss that separately (not here).

kevinrushforth · 2025-04-22T21:04:47Z

modules/jfx.incubator.richtext/src/test/java/test/jfx/incubator/scene/util/TUtil.java

+/**
+ * There should be a common place for module-agnostic test utilities.
+ */
+public class TUtil {


Since this is intended to be a static utility class, maybe add a private constructor to underscore this?

kevinrushforth · 2025-04-22T21:05:09Z

...cubator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/support/RTUtil.java

+/**
+ * Utilities for RichTextArea-based tests.
+ */
+public class RTUtil {


Suggestion: add a private constructor

kevinrushforth · 2025-04-22T21:06:04Z

...ichtext/src/test/java/test/jfx/incubator/scene/control/richtext/support/TestStyledInput.java

+        String[] lines = text.split("\n");
+        ArrayList<StyledSegment> ss = new ArrayList<>();
+        for (int i = 0; i < lines.length; i++) {
+            if(i > 0) {


Minor: space after if

kevinrushforth · 2025-04-22T21:44:33Z

...bator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/RichTextAreaTest.java

+    }
+
+//    @Test
+//    public void insertLineBreak() {


Why is this commented out? A comment explaining why would be good. Or if there is a functional bug that prevents it, uncomment it and skip it with @Disabled("JDK-8888888").

created https://bugs.openjdk.org/browse/JDK-8355415

kevinrushforth · 2025-04-22T21:46:02Z

...bator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/RichTextAreaTest.java

+//    }
+
+    @Test
+    public void isRedoable() {


Do you also have functional tests for undo/redo?

yes, undo() and redo()

I tried to exercise every public method to some extent.

kevinrushforth · 2025-04-22T21:47:31Z

...bator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/RichTextAreaTest.java

+    }
+
+    @Test
+    public void modelChangeClearsSelection() {


Since the actual data is sorted in the model, it might also be good to test that a model change clears all of the text that you have previously appended (in another test method).

Since the actual data is sorted

Typo, I meant "stored"

each test starts with a freshly created model, but it's a good idea to add a check for the content.

kevinrushforth · 2025-04-22T21:53:41Z

...bator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/RichTextAreaTest.java

+    }
+
+    @Test
+    public void redo() {


A test of the undo / redo stack would be helpful (this tests a single undo/redo value, so is a good first step, but doesn't test its "stack" nature).

kevinrushforth · 2025-04-22T21:59:45Z

...incubator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/CodeAreaTest.java

+        assertTrue(control.getModel() == null);
+
+        control = new CodeArea(null);
+        control.setModel(new CodeTextModel() {


Suggestion: Check that the model you set can be read back.

andy-goryachev-oracle · 2025-04-22T22:59:49Z

Thank you, Kevin, good suggestions.

I presume you'll handle the rest of the API with other bugs / PRs?

Correct - the idea is to follow up with headful behavioral test(s).

kevinrushforth

LGTM. I'll reapprove if you make further changes.

kevinrushforth · 2025-04-23T20:43:46Z

...bator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/RichTextAreaTest.java

+    @Disabled("JDK-8355415")
+    // TODO combine with copy()


Minor: I might switch these two lines to keep all the annotations together.

andy-goryachev-oracle · 2025-04-23T21:43:36Z

/integrate

openjdk · 2025-04-23T21:45:05Z

Going to push as commit 1b2f022d0c08d556d0decdf71d6e0c9d13dbe6f8.
Since your change was applied there have been 3 commits pushed to the master branch:

1a12966: 8354478: Improve StageStyle documentation
4df2326: 8352162: Update libxml2 to 2.13.8
46b36fe: 8354876: Update SQLite to 3.49.1

Your commit was automatically rebased without conflicts.

openjdk · 2025-04-23T21:45:17Z

@andy-goryachev-oracle Pushed as commit 1b2f022.

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

8347359: RichTextArea API Tests

cb5ae41

andy-goryachev-oracle added 11 commits January 14, 2025 13:02

css

05bd4fe

Merge remote-tracking branch 'origin/master' into 8347359.rta.api.tests

3fdff3c

binding

9b4e71b

functional

d7ea56f

whitespace

15282cf

Merge branch 'master' into 8347359.rta.api.tests

9e4ce63

methods

09a90ed

code area

adb76eb

Merge branch 'master' into 8347359.rta.api.tests

6612f55

get text

6ff69e1

clamp

174cc7a

andy-goryachev-oracle mentioned this pull request Jan 27, 2025

8348736: RichTextArea clamp and getText #1684

Closed

3 tasks

andy-goryachev-oracle added 4 commits January 27, 2025 14:32

cleanup

5390d1f

check

86a2d12

newlines

a55dc8d

Merge remote-tracking branch 'origin/8348736.rta.followup.2' into

7d9159f

8347359.rta.api.tests

andy-goryachev-oracle added 3 commits January 28, 2025 09:56

review comments

ff8942b

Merge remote-tracking branch 'origin/8348736.rta.followup.2' into 834…

e8085fe

…7359.rta.api.tests

Merge remote-tracking branch 'origin/master' into 8347359.rta.api.tests

25845fa

andy-goryachev-oracle marked this pull request as ready for review January 29, 2025 16:41

openjdk bot added the rfr Ready for review label Jan 29, 2025

kevinrushforth self-requested a review January 29, 2025 20:22

Merge remote-tracking branch 'origin/master' into 8347359.rta.api.tests

d6e9ba3

andy-goryachev-oracle added 3 commits February 26, 2025 13:56

Merge remote-tracking branch 'origin/master' into 8347359.rta.api.tests

b3a6087

Merge remote-tracking branch 'origin/master' into 8347359.rta.api.tests

b8e59bd

Merge remote-tracking branch 'origin/master' into 8347359.rta.api.tests

c0cde4a

andy-goryachev-oracle added 4 commits April 7, 2025 14:50

Merge remote-tracking branch 'origin/master' into 8347359.rta.api.tests

f78f541

Merge remote-tracking branch 'origin/master' into 8347359.rta.api.tests

0adcd2a

Merge remote-tracking branch 'origin/master' into 8347359.rta.api.tests

92afaa6

rm junit4

0fb16fd

kevinrushforth reviewed Apr 22, 2025

View reviewed changes

Merge remote-tracking branch 'origin/master' into 8347359.rta.api.tests

5ff1983

review comments

4f07d5f

kevinrushforth approved these changes Apr 23, 2025

View reviewed changes

openjdk bot added the ready Ready to be integrated label Apr 23, 2025

review comments

8d3f3ec

andy-goryachev-oracle requested a review from kevinrushforth April 23, 2025 21:18

openjdk bot removed the ready Ready to be integrated label Apr 23, 2025

kevinrushforth approved these changes Apr 23, 2025

View reviewed changes

openjdk bot added the ready Ready to be integrated label Apr 23, 2025

openjdk bot added the integrated Pull request has been integrated label Apr 23, 2025

openjdk bot closed this Apr 23, 2025

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Apr 23, 2025

andy-goryachev-oracle deleted the 8347359.rta.api.tests branch April 23, 2025 21:48

		if (throwable instanceof RuntimeException) {
		throw (RuntimeException)throwable;

8347359: RichTextArea API Tests #1677

8347359: RichTextArea API Tests #1677

Conversation

andy-goryachev-oracle commented Jan 14, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 14, 2025

openjdk bot commented Jan 14, 2025 • edited Loading

openjdk bot commented Jan 28, 2025

andy-goryachev-oracle commented Jan 29, 2025

mlbridge bot commented Jan 29, 2025 • edited Loading

Webrevs

bridgekeeper bot commented Feb 26, 2025

andy-goryachev-oracle commented Feb 26, 2025

bridgekeeper bot commented Mar 27, 2025

andy-goryachev-oracle commented Mar 27, 2025

kevinrushforth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-goryachev-oracle commented Apr 22, 2025

kevinrushforth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-goryachev-oracle commented Apr 23, 2025

openjdk bot commented Apr 23, 2025

openjdk bot commented Apr 23, 2025

andy-goryachev-oracle commented Jan 14, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jan 14, 2025 •

edited

Loading

mlbridge bot commented Jan 29, 2025 •

edited

Loading