On Tue, 5 Jan 2021 16:19:47 GMT, Jeanette Winzenburg <[email protected]> wrote:
> Fix looks good, verified tests failing (except one, commented) before and > passing after the fix. Thanks for the review. I have updated the PR according to all comments. Please take a look. :) > modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java > line 289: > >> 287: assertNull(tabPane.getSkin()); >> 288: assertEquals(0, tabPane.getChildrenUnmodifiable().size()); >> 289: } > > Same as above: what's the reason to actually show? > > And just curious: why is this passing already before the fix? Corrected to use installDefaultSkin(). It works without this fix is because the children are cleared([here](https://github.com/openjdk/jfx/blob/e74f679fda6f03ee8449836147815fdaafb5d626/modules/javafx.controls/src/main/java/javafx/scene/control/Control.java#L300)) in Control class when the skin is set to null. > modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java > line 302: > >> 300: @Test >> 301: public void testNPEWhenRemTabAfterSkinIsReplaced() { >> 302: TabPane tabPane = new TabPane(); > > Personally I don't like abbreviation in method names, not even test methods. > `testNPEWhenRemoveTabAfterSkinIsReplaced` isn't that much longer :) Corrected. > modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java > line 311: > >> 309: @Test >> 310: public void testAddRemTabsAfterSkinIsReplaced() { >> 311: TabPane tabPane = new TabPane(); > > see above Corrected ------------- PR: https://git.openjdk.java.net/jfx/pull/318
