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

[Jeanette Winzenburg]

On Sun, 11 Jul 2021 18:28:24 GMT, Marius Hanl <mhanl at openjdk.org> wrote:

>>> 
>>> 
>>> Hmm, but leaving a test without an assert is also bad. You have any suggestions?
>> 
>> 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 :) 
>> 
>>          @Test 
>>          ...
>>          // configure: just as you do
>>          comboBox.setEditable(true)
>>          ...
>>          // the test: just as you do - switch to false
>>          comboBox.setEditable(false)
>>          // safe-guard against future implementation changes: switch back to true
>>          comboBox.setEditable(true)
>>          // end of test
>
> There is no rule, but in my opinion it is also not a good practise. 
> I agree it's also not a big problem, but in general a test should check something. 
> And I think it's not a problem to check, that we still have no value inside ComboBox, even though is not really related. 
> 
> In JUnit 5, there would be `assertDoesNotThrow​()` for that usecase. But I may found a solution for our version, there is  `@Test(expected = Test.None.class)`, which basically tells us explicitly, that we don't expect any exception (and so, the code under test did throw an exception one time in the past).

agree to all that makes the test code easier to read/understand, the JUnit 5 looks good :) Your solution for JU4 isn't really easier to understand .. but could be only me, or simple enough to get used to.

Just: adding an assert that basically asserts nothing or the wrong (because unspecified) thing is .. worse than a test that's hard to read, IMO

-------------

PR: https://git.openjdk.java.net/jfx/pull/557