Re: RFR: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel [v4]
On Wed, 7 Jul 2021 19:37:23 GMT, Marius Hanl 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
Re: RFR: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel [v4]
On Thu, 8 Jul 2021 21:05:51 GMT, Marius Hanl wrote: >> 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? > > EDIT: I think it's not wrong to check the button cell. But I guess you may > want to see another check, but then again, I don't know what should I check > for. > We should expect that the value is still shown in the button cell after an > editable change (and ofc selectionModel null) the buttonCell state is simply irrelevant here, what's striking - you - again is a test without final assert :) Same as toggling editability. - PR: https://git.openjdk.java.net/jfx/pull/557
Re: RFR: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel [v4]
On Sun, 11 Jul 2021 18:28:24 GMT, Marius Hanl 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
Re: RFR: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel [v4]
On Fri, 9 Jul 2021 09:58:38 GMT, Jeanette Winzenburg wrote: >> 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. > >> >> >> 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). - PR: https://git.openjdk.java.net/jfx/pull/557
Re: RFR: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel [v4]
On Thu, 8 Jul 2021 21:04:44 GMT, Marius Hanl 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 - PR: https://git.openjdk.java.net/jfx/pull/557
Re: RFR: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel [v4]
On Wed, 7 Jul 2021 19:37:23 GMT, Marius Hanl 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 okay, you two convinced me - include all NPEs in the combo/skin :) - Changes requested by fastegal (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/557
Re: RFR: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel [v4]
On Thu, 8 Jul 2021 10:52:06 GMT, Jeanette Winzenburg 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
Re: RFR: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel [v4]
On Wed, 7 Jul 2021 19:37:23 GMT, Marius Hanl 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 - fix for value/editable change looks fine, - suggest to not include the (yet incomplete) fix for sync of combo/list selection in skin left some inline comments to the new tests 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 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 - Changes requested by fastegal (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/557
Re: RFR: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel [v4]
On Wed, 7 Jul 2021 19:37:23 GMT, Marius Hanl 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 Marked as reviewed by aghaisas (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/557
Re: RFR: 8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel [v4]
> This PR fixes 2 NPEs in Choice-and ComboBox, when the selection model is null. > > ChoiceBox: > - Null check in **valueProperty()** listener > > ComboBox: > - Null check in **valueProperty()** listener > - Null check in **ComboBoxListViewSkin#updateValue()** > > The tests checks, that no NPE is printed to the console. They also 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 - Changes: - all: https://git.openjdk.java.net/jfx/pull/557/files - new: https://git.openjdk.java.net/jfx/pull/557/files/0b631ed4..b064f61e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=557=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=557=02-03 Stats: 37 lines in 3 files changed: 31 ins; 1 del; 5 mod Patch: https://git.openjdk.java.net/jfx/pull/557.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/557/head:pull/557 PR: https://git.openjdk.java.net/jfx/pull/557
