On Wed, 23 Aug 2023 23:30:50 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
> Creating the first batch of tests and testing framework that enables writing > behavior tests for javafx controls, focusing on key bindings. The idea is to > make writing such tests a simple process. > > This PR deals with the descendants of TextInputControl (TextField, > PasswordField, TextArea). Most of the tests are headless, but in some cases > (TextArea) a headful test is required because the behavior needs rendered > text to function (example: page up / page down / line start / line end and > the like). > > The tests exercise the key bindings registered by the Skin (or, rather the > associated Behavior) at least once, and sometimes more than once. > > Some mappings cannot be tested due to Robot not supporting keypad events > (created [JDK-8316307](https://bugs.openjdk.org/browse/JDK-8316307)). > > In addition, the key bindings are documented in /doc-files/behavior markdown > documents. A couple quick comments I spotted while skimming this PR. modules/javafx.graphics/src/test/java/test/com/sun/javafx/pgstub/StubToolkit.java line 971: > 969: public static StubToolkit ensure() { > 970: return (StubToolkit)Toolkit.getToolkit(); > 971: } This is unused (and may not be needed). I think you can revert the changes to this file. tests/system/src/test/java/test/javafx/scene/control/behavior/Mod.java line 33: > 31: * Key Modifiers for use in behavior tests. > 32: */ > 33: public enum Mod { If this needs to be public (meaning visible to other tests outside this test package), I might recommend a more descriptive name. tests/system/src/test/java/test/util/Util.java line 67: > 65: private static final boolean MAC = OS.startsWith("Mac"); > 66: private static final boolean LINUX = OS.startsWith("Linux"); > 67: This duplicates the logic in PlatformUtil. We just fixed a few other places to get rid of this sort of duplication, so I recommend removing this. tests/system/src/test/java/test/util/Util.java line 480: > 478: */ > 479: public static boolean isWindows(){ > 480: return WINDOWS; Are these really needed or can the tests call PlatformUtil directly? If there is a good reason to add them, then they should delegate to PlatformUtil rather than duplicate the functionality. ------------- PR Review: https://git.openjdk.org/jfx/pull/1221#pullrequestreview-1627554862 PR Review Comment: https://git.openjdk.org/jfx/pull/1221#discussion_r1326387332 PR Review Comment: https://git.openjdk.org/jfx/pull/1221#discussion_r1326388978 PR Review Comment: https://git.openjdk.org/jfx/pull/1221#discussion_r1326391012 PR Review Comment: https://git.openjdk.org/jfx/pull/1221#discussion_r1326392044
