On Thu, 8 Jul 2021 10:52:06 GMT, Jeanette Winzenburg <faste...@openjdk.org> wrote:
>> Marius Hanl has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Fixed NPE for setEditable() and layoutChildren() >> - removed unneeded button cell in test > > modules/javafx.controls/src/test/java/test/javafx/scene/control/ComboBoxTest.java > line 324: > >> 322: >> 323: comboBox.layout(); >> 324: > > KISS again :) > > - buttonCell is unrelated > - setting the value is a not immediately obvious trigger of the NPE > - the test relies on the fact that the combo's skin is installed in setup - > when installing it later, we'd run into a NPE at a different location in code > > but then, this might be something to remember for a future bug/fix, if we > agree on not including the list/combo sync into this bug ButtonCell strikes again. 😄 I will remove it. Hm, this sounds like a follow-up then. Should I test this and create one? > modules/javafx.controls/src/test/java/test/javafx/scene/control/ComboBoxTest.java > line 338: > >> 336: assertNull(comboBox.getValue()); >> 337: } >> 338: > > - ideally, tests should not cement implementation details (particularly not > if these are potential bugs, like forcing a null value :) In other words: > testing the value doesn't belong here (it's null before and null after > toggling editable) > - test should cover both directions (from editable -> !editable as well as > from editable -> !editable): it's only accidental that the value is forced in > one direction, otherwise we might need to start over if the implementation > should change in future > - personally, I prefer one tests to be focused on exactly one change (here: > one per direction to not rely on intermediate state, but that's a matter of > taste/convention Hmm, but leaving a test without an assert is also bad. You have any suggestions? I may can add another editable test, which will pass before and after. ------------- PR: https://git.openjdk.java.net/jfx/pull/557
