RFR: 8299335: Monkey Tester Application [v14]
Kevin Rushforth
kcr at openjdk.org
Fri Apr 28 21:39:32 UTC 2023
On Fri, 28 Apr 2023 16:47:24 GMT, Andy Goryachev <angorya at openjdk.org> wrote:
>> Monkey Tester - a JavaFX application designed to support manual ad-hoc testing of individual JavaFX controls. Unlike Ensemble, the goal of this application is to facilitate manual testing rather than demonstrate the capabilities of JavaFX.
>>
>> Feedback and suggestions are always welcome.
>>
>> 
>
> Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision:
>
> review comments
tests/manual/monkey/README.md line 10:
> 8: ## Prerequisites
> 9:
> 10: JavaFX SDK is required to build the tool. The latest SDK can be found here:
Since this is largely a developer tool, I think it might be worth adding something like "You can use a JavaFX SDK that you build or you can download the JavaFX SDK. The latest SDK ..."
tests/manual/monkey/README.md line 38:
> 36: Applications stores the user preferences (window position, currently selected page, etc.) in `~/.MonkeyTester` directory.
> 37:
> 38: To use a different directory, redefine the `user.home` system property, `-Duser.home=<DIR>`.
I presume this is mainly so you can run multiple MonkeyTester instances without having them fight over the preferences, right? Maybe add a sentence to that effect?
Possible future RFE: This might have other implications (e.g., it will change the location of the default WebView user data directory). Do you think it would it be better to have a custom property that, when set, will be used instead of asking users to change `user.home`? This could be done later if you think it's worth doing.
tests/manual/monkey/README.md line 40:
> 38: To use a different directory, redefine the `user.home` system property, `-Duser.home=<DIR>`.
> 39:
> 40: To disable saving, specify `-Ddisable.settings=true` VM agrument.
This also disables loading of settings (as expected), so maybe say "disable loading and saving"?
tests/manual/monkey/build.xml line 97:
> 95:
> 96: <!-- build all -->
> 97: <target name="build-all" depends="compile, copy-resources, make-jar, sha-jar, copy-jar" />
Suggestion: Add a `jar` target that depends on `build-all` (so it can be used as an alias, since most of our apps use that).
tests/manual/monkey/src/com/oracle/tools/fx/monkey/pages/AccordionPage.java line 35:
> 33:
> 34: /**
> 35: *
Maybe add something here?
tests/manual/monkey/src/com/oracle/tools/fx/monkey/pages/ChartPage.java line 49:
> 47: */
> 48: public class ChartPage extends TestPaneBase {
> 49: public enum Mode {
Possible future RFE: Add Pie chart?
tests/manual/monkey/src/com/oracle/tools/fx/monkey/pages/DemoPage.java line 31:
> 29:
> 30: /**
> 31: *
Maybe add a comment here?
tests/manual/monkey/src/com/oracle/tools/fx/monkey/pages/DualFocusPage.java line 1:
> 1: // Copyright © 2018-2019 Andy Goryachev <andy at goryachev.com>
You need to replace this with a standard copyright header.
tests/manual/monkey/src/com/oracle/tools/fx/monkey/settings/FxSettings.java line 43:
> 41: /**
> 42: * This facility coordinates saving UI settings to and from persistent media.
> 43: * All the calls, excepr useProvider(), are expected to happen in an FX application thread.
Minor typo: "excepr" --> "except"
tests/manual/monkey/src/com/oracle/tools/fx/monkey/settings/FxSettingsFileProvider.java line 67:
> 65: } finally {
> 66: rd.close();
> 67: }
Suggestion: Consider try-with-resources instead of try...finally
tests/manual/monkey/src/com/oracle/tools/fx/monkey/settings/FxSettingsFileProvider.java line 84:
> 82: } finally {
> 83: wr.close();
> 84: }
Same suggestion here (try-with-resources)
tests/manual/monkey/src/com/oracle/tools/fx/monkey/util/FX.java line 46:
> 44:
> 45: /**
> 46: * Shortcuts and convenience methods that should be a part of JavaFX.
It's a debatable point that they "should be" part of JavaFX. I recommend changing it to: "perhaps could be added to JavaFX"
tests/manual/monkey/src/com/oracle/tools/fx/monkey/util/FX.java line 155:
> 153: public static Background background(Color c) {
> 154: return new Background(new BackgroundFill(c, null, null));
> 155: }
Btw, we already have a utility method that does this. See [Background::fill](https://github.com/openjdk/jfx/blob/jfx20/modules/javafx.graphics/src/main/java/javafx/scene/layout/Background.java#L359).
tests/manual/monkey/src/com/oracle/tools/fx/monkey/util/OptionPane.java line 43:
> 41: public OptionPane() {
> 42: // no such thing
> 43: // https://stackoverflow.com/questions/20454021/how-to-set-padding-between-columns-of-a-javafx-gridpane
I'd prefer we didn't reference stackoverflow.
tests/manual/monkey/src/module-info.java line 1:
> 1: module monkey_tester {
This file needs a standard copyright header.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180788354
PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180787264
PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180788771
PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180789785
PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180792641
PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180802943
PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180811048
PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180811546
PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180813483
PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180817616
PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180817807
PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180821129
PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180824373
PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180826474
PR Review Comment: https://git.openjdk.org/jfx/pull/1097#discussion_r1180819728
More information about the openjfx-dev
mailing list