RFR: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel [v4]

[Marius Hanl]

On Thu, 8 Jul 2021 10:52:06 GMT, Jeanette Winzenburg <fastegal at 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