RFR: 8349750: [TestBug] NodeInitializationStressTest: remaining Nodes

Tue Feb 25 18:09:06 UTC 2025

On Sat, 15 Feb 2025 00:08:22 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

> ## Added Missing Controls
> 
> ButtonBar
> ProgressIndicator
> Separator
> Slider,
> 
> ## Added Node Classes
> 
> AnchorPane
> AmbientLight
> Arc
> BorderPane
> Box
> Circle
> CubicCurve
> Cylinder
> DialogPane
> DirectionalLight
> Ellipse
> FlowPane
> GridPane
> Group
> HBox
> ImageView
> Line
> MediaView
> MeshView
> Pane
> ParallelCamera
> Path
> PerspectiveCamera
> PointLight
> Polygon
> Polyline
> QuadCurve
> Rectangle
> Region
> Sphere
> StackPane
> SVGPath
> TilePane
> VBox
> 
> 
> ## Miscellaneous
> 
> - minor improvements
> - remove tests that execute show() in non-fx threads per [JDK-8350048](https://bugs.openjdk.org/browse/JDK-8350048)
> 
> 
> 
> ## Not Included Due to Threading Limitations
> 
> HTMLEditor
> MenuBar
> SwingNode
> WebView
> 
> ## Note to the Reviewers
> 
> To avoid merge conflicts, the preferred order of integrations:
> 
> #1697 
> #1713 
> #1717

The newly added tests look good, as do the modified tests to eliminate calling show / hide from a background thread.

One more test that would be useful is one that verifies that Scene can be constructed and modified (root node changed and descendants of the root node modified / added / removed) on a background thread.

I left a few questions / comments inline as well.

tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 224:

> 222:  * Also, the visible node gets accessed periodically in the FX application thread just to shake things up.
> 223:  */
> 224: @TestMethodOrder(MethodOrderer.MethodName.class)

Why is this needed? Our tests use the JUnit5 default (which is random) without a compelling reason to do otherwise. I recommend removing it.

tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 227:

> 225: public class NodeInitializationStressTest extends RobotTestBase {
> 226:     /* debugging aid: set this flag to true and comment out assumeFalse(SKIP_TEST) to run specific test(s). */
> 227:     private static final boolean SKIP_TEST = false;

Maybe it's time to remove this flag? Does Eclipse not provide a way to run a single test?

tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 328:

> 326:             c.setCenter(n);
> 327:             BorderPane.setAlignment(n, nextEnum(Pos.class));
> 328:             accessNode(c);

Should this call `accessRegion` instead?

tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 656:

> 654: 
> 655:     @Test
> 656:     public void imageView() {

You mentioned that there was a problem with modifying a writable image on a background thread. Similarly, there might be a problem loading animated GIF images on a background thread. Is there a bug filed?

tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 737:

> 735:             c.setFitHeight(nextDouble(200));
> 736:             c.setFitWidth(nextDouble(200));
> 737:             //c.setMediaPlayer(new MediaPlayer(new Media("no-data-url-support")));

Would it be useful to load a media file? If so, you could use the default media file that Ensemble uses.

tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 1124:

> 1122:     }
> 1123: 
> 1124:     //@Test disabled because it times out

Please file a bug and add an `@Disabled` annotation using that bug ID.

tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 1388:

> 1386: 
> 1387:     private static void accessPane(Pane p, Consumer<Node> onChild) {
> 1388:         ObservableList<Node> children = p.getChildren();

It might be useful for this method to call `accessRegion` (as is done from `accessControl`).

tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 1423:

> 1421:         c.setDrawMode(nextEnum(DrawMode.class));
> 1422:         PhongMaterial m = new PhongMaterial();
> 1423:         //m.setSelfIlluminationMap(Image); // what is expected?

There is a good description of the material properties and how they work, complete with illustrations, in the `PhongMaterial` class docs.

tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 1426:

> 1424:         m.setSpecularColor(nextColor());
> 1425:         //m.setSpecularMap(Image); // what is expected?
> 1426:         m.setSpecularPower(nextDouble(100)); // what is the acceptable range?

There is technically no upper limit, but 1.0 - 200.0 is a good range (so maybe add 1.0 to the value returned by `nextDouble`).

tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 1671:

> 1669: 
> 1670:     private static PathElement createPathElement() {
> 1671:             switch (nextInt(7)) {

Minor: fix Indentation

tests/system/src/test/java/test/robot/javafx/scene/NodeInitializationStressTest.java line 1755:

> 1753: 
> 1754:     private static String createSvgPath() {
> 1755:         switch(nextInt(4)) {

Minor: space after switch

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

PR Review: https://git.openjdk.org/jfx/pull/1713#pullrequestreview-2641843482
PR Review Comment: https://git.openjdk.org/jfx/pull/1713#discussion_r1970196814
PR Review Comment: https://git.openjdk.org/jfx/pull/1713#discussion_r1970198168
PR Review Comment: https://git.openjdk.org/jfx/pull/1713#discussion_r1970258183
PR Review Comment: https://git.openjdk.org/jfx/pull/1713#discussion_r1970231968
PR Review Comment: https://git.openjdk.org/jfx/pull/1713#discussion_r1970236555
PR Review Comment: https://git.openjdk.org/jfx/pull/1713#discussion_r1970267812
PR Review Comment: https://git.openjdk.org/jfx/pull/1713#discussion_r1970271244
PR Review Comment: https://git.openjdk.org/jfx/pull/1713#discussion_r1970276883
PR Review Comment: https://git.openjdk.org/jfx/pull/1713#discussion_r1970277986
PR Review Comment: https://git.openjdk.org/jfx/pull/1713#discussion_r1970288827
PR Review Comment: https://git.openjdk.org/jfx/pull/1713#discussion_r1970292234