On Mon, 21 Apr 2025 15:18:02 GMT, Andy Goryachev <ango...@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
