On Tue, 5 Jan 2021 16:03:42 GMT, Jeanette Winzenburg <[email protected]> wrote:
>> Ambarish Rapte has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Review update > > modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java > line 275: > >> 273: assertEquals(3, tabPane.getChildrenUnmodifiable().size()); >> 274: } >> 275: > > hmm .. looks a bit unusual (could be me, though :) > > - why showControl? just install/replace the skin should be enough? > - the outcome of testing an illegal state (after instantiating the skin and > before really setting it as the control's skin at which time we have a skin : > control relation of 2:1) is unspecified. F.i. there are skins that replace > all children in the constructor (which may or may not be a good idea) that > would fail a test similar to this. > > How about > > TabPane tabPane .. > installDefaultSkin(tabPane); > int children = tabPane.getxxChildren().size(); > replaceSkin(tabPane); > assertEquals(children, tabPane.getxxChildren().size()); > > If we decide to go without showing, the other tests should be changed as well > :) 1. `showControl` is not required. It is now changed to `installDefaultSkin` in all tests. 2. Test did verify an illegal state, Intention was to keep a note of that illegal state. I have changed it as you suggested. ------------- PR: https://git.openjdk.java.net/jfx/pull/318
