RFR: 8347359: RichTextArea API Tests [v8]

Tue Apr 22 22:06:53 UTC 2025

On Mon, 21 Apr 2025 15:18:02 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Additional RichTextArea API tests.
>
> Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 27 commits:
> 
>  - rm junit4
>  - Merge remote-tracking branch 'origin/master' into 8347359.rta.api.tests
>  - Merge remote-tracking branch 'origin/master' into 8347359.rta.api.tests
>  - Merge remote-tracking branch 'origin/master' into 8347359.rta.api.tests
>  - Merge remote-tracking branch 'origin/master' into 8347359.rta.api.tests
>  - Merge remote-tracking branch 'origin/master' into 8347359.rta.api.tests
>  - Merge remote-tracking branch 'origin/master' into 8347359.rta.api.tests
>  - Merge remote-tracking branch 'origin/master' into 8347359.rta.api.tests
>  - Merge remote-tracking branch 'origin/master' into 8347359.rta.api.tests
>  - Merge remote-tracking branch 'origin/8348736.rta.followup.2' into 8347359.rta.api.tests
>  - ... and 17 more: https://git.openjdk.org/jfx/compare/703a9a90...0fb16fdc

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.

modules/jfx.incubator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/CodeAreaTest.java line 77:

> 75: 
> 76:         control = new CodeArea(null);
> 77:         control.setModel(new CodeTextModel() {

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

modules/jfx.incubator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/RichTextAreaTest.java line 103:

> 101: 
> 102:         control = new RichTextArea(null);
> 103:         control.setModel(new CustomStyledTextModel());

Suggestion: save `new CustomStyledTextModel()` in a local var and then `assertSame(model, control.getModel());`

modules/jfx.incubator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/RichTextAreaTest.java line 254:

> 252:     public void appendText() {
> 253:         TextPos p = control.appendText("a");
> 254:         assertEquals(TextPos.ofLeading(0, 1), p);

Suggestion: also check that the character was actually appended, by reading the text back. This suggestion applies to all of the append methods (e.g., when setting a style, check that the style was actually applied).

modules/jfx.incubator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/RichTextAreaTest.java line 307:

> 305:         control.copy();
> 306:         String s = Clipboard.getSystemClipboard().getString();
> 307:         assertEquals("ax", s);

Suggestion: expand this to check various selection segments and boundary conditions (e.g., test copying when the selection has a single character at the beginning, missing, and end) as well as no selection (clipboard string should be empty).

modules/jfx.incubator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/RichTextAreaTest.java line 393:

> 391: 
> 392:     @Test
> 393:     public void getParagraphCount() {

Maybe also assert that the paragraph count is initially null?

modules/jfx.incubator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/RichTextAreaTest.java line 410:

> 408:         control.setModel(null);
> 409:         // on second through, maybe this should return null
> 410:         assertEquals(TextPos.ZERO, control.getParagraphEnd(0));

Are you keeping a list of such API questions?

In this case, I would think that asking for a paragraph that is out of range would throw IOOBE. What does control.getParagraph(N) do when N is out of range? This method should behave similarly.

modules/jfx.incubator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/RichTextAreaTest.java line 444:

> 442: 
> 443: //    @Test
> 444: //    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")`.

modules/jfx.incubator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/RichTextAreaTest.java line 451:

> 449: 
> 450:     @Test
> 451:     public void isRedoable() {

Do you also have functional tests for undo/redo?

modules/jfx.incubator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/RichTextAreaTest.java line 466:

> 464: 
> 465:     @Test
> 466:     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).

modules/jfx.incubator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/RichTextAreaTest.java line 550:

> 548: 
> 549:     @Test
> 550:     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).

modules/jfx.incubator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/support/RTUtil.java line 41:

> 39:  * Utilities for RichTextArea-based tests.
> 40:  */
> 41: public class RTUtil {

Suggestion: add a private constructor

modules/jfx.incubator.richtext/src/test/java/test/jfx/incubator/scene/control/richtext/support/TestStyledInput.java line 47:

> 45:         ArrayList<StyledSegment> ss = new ArrayList<>();
> 46:         for (int i = 0; i < lines.length; i++) {
> 47:             if(i > 0) {

Minor: space after `if`

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

> 38: 
> 39: /**
> 40:  * There should be a common place for module-agnostic test utilities.

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

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

> 40:  * There should be a common place for module-agnostic test utilities.
> 41:  */
> 42: public class TUtil {

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

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

> 47:         Thread.currentThread().setUncaughtExceptionHandler((thread, throwable) -> {
> 48:             if (throwable instanceof RuntimeException) {
> 49:                 throw (RuntimeException)throwable;

Why is RuntimeException treated differently than checked Exception and Error?

-------------

PR Review: https://git.openjdk.org/jfx/pull/1677#pullrequestreview-2785310885
PR Review Comment: https://git.openjdk.org/jfx/pull/1677#discussion_r2054949072
PR Review Comment: https://git.openjdk.org/jfx/pull/1677#discussion_r2054884101
PR Review Comment: https://git.openjdk.org/jfx/pull/1677#discussion_r2054899845
PR Review Comment: https://git.openjdk.org/jfx/pull/1677#discussion_r2054922565
PR Review Comment: https://git.openjdk.org/jfx/pull/1677#discussion_r2054924225
PR Review Comment: https://git.openjdk.org/jfx/pull/1677#discussion_r2054924993
PR Review Comment: https://git.openjdk.org/jfx/pull/1677#discussion_r2054929603
PR Review Comment: https://git.openjdk.org/jfx/pull/1677#discussion_r2054931387
PR Review Comment: https://git.openjdk.org/jfx/pull/1677#discussion_r2054933189
PR Review Comment: https://git.openjdk.org/jfx/pull/1677#discussion_r2054943566
PR Review Comment: https://git.openjdk.org/jfx/pull/1677#discussion_r2054871418
PR Review Comment: https://git.openjdk.org/jfx/pull/1677#discussion_r2054872504
PR Review Comment: https://git.openjdk.org/jfx/pull/1677#discussion_r2054862174
PR Review Comment: https://git.openjdk.org/jfx/pull/1677#discussion_r2054870939
PR Review Comment: https://git.openjdk.org/jfx/pull/1677#discussion_r2054835553