On Wed, 7 Jul 2021 19:37:23 GMT, Marius Hanl <mh...@openjdk.org> wrote:
>> This PR fixes multiple NPEs in Choice-and ComboBox, when the selection model >> is null. >> >> ChoiceBox: >> - Null check in **valueProperty()** listener >> >> ComboBox: >> - Null check in **editableProperty()* listener* >> - Null check in **valueProperty()** listener >> - Null check in **ComboBoxListViewSkin#updateValue()** >> - Null check in **ComboBoxListViewSkin#layoutChildren()** >> >> ~~The tests checks, that no NPE is printed to the console.~ >> Some checks, that the set value is still displayed (either in the ComboBox >> button cell or the ChoiceBox display label)~~ > > 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 > Not aware of such a rule - if we fix code throwing an exception there is not > much to assert, except that it fails before and passes after. And paddling > back a bit, I think a separate test for the back switch would be overdoing it > :) There is no rule, but in my opinion it's simply better and more meaningful with an assert (maybe this also come's a bit from Sonar 😄 ). But you are right, the checks are not really needed and thinking about it they can be confusing as well. So I will update the PR. But I'm also interested in other opinions. It would be perfect for me if we have a nice way to indicate, that a Unit test expects no exception. (I now wrote a comment on every method which triggers the NPE) ------------- PR: https://git.openjdk.java.net/jfx/pull/557
