RFR: 8273324: IllegalArgumentException: fromIndex(0) > toIndex(-1) after clear and select TableCell

[Kevin Rushforth]

On Sat, 4 Sep 2021 11:20:30 GMT, Marius Hanl <mhanl at openjdk.org> wrote:

>>> Can we get a unit test for this case?
>> 
>> This is a test that fails without the fix, and passes with the fix. However, there are no assertions, which is... strange. Once [JDK-8273336](https://bugs.openjdk.java.net/browse/JDK-8273336) is fixed, the test could validate the selection state.
>> 
>> 
>>     @Test
>>     public void test_jdk_8273324() {
>>         class Person {
>>             final String firstName, lastName;
>>             Person(String firstName, String lastName) {
>>                 this.firstName = firstName;
>>                 this.lastName = lastName;
>>             }
>>             public String getFirstName() { return firstName; }
>>             public String getLastName() { return lastName; }
>>         }
>> 
>>         var tableView = new TableView<Person>();
>>         tableView.getSelectionModel().setCellSelectionEnabled(true);
>>         tableView.getSelectionModel().setSelectionMode(SelectionMode.MULTIPLE);
>> 
>>         var col1 = new TableColumn<Person, String>();
>>         col1.setCellValueFactory(new PropertyValueFactory<>("firstName"));
>>         var col2 = new TableColumn<Person, String>();
>>         col2.setCellValueFactory(new PropertyValueFactory<>("lastName"));
>>         tableView.getColumns().addAll(List.of(col1, col2));
>> 
>>         var ab = new Person("a", "b");
>>         tableView.getItems().add(ab);
>>         var cd = new Person("c", "d");
>>         tableView.getItems().add(cd);
>> 
>>         tableView.getSelectionModel().select(0);
>>         tableView.getSelectionModel().clearAndSelect(0, col1);
>>     }
>
>> > Can we get a unit test for this case?
>> 
>> This is a test that fails without the fix, and passes with the fix. However, there are no assertions, which is... strange. Once [JDK-8273336](https://bugs.openjdk.java.net/browse/JDK-8273336) is fixed, the test could validate the selection state.
>>
> 
> That is still fine. I also don't like to write tests without at least one assertion, but there is not other way in JUnit4. I ended up writing a comment above the problematic call, e.g.: `// Should not throw an NPE.`

Your proposed unit test looks fine to me, and catches the bug. I agree with @Maran23 that as long as you add a comment that it should run without throwing NPE, it's OK to not have any asserts. One final point is that we have moved away from the practice of including the bug ID in the name of the method (or class, which we also used to do), and instead just use a meaningful name. You can add the bug ID in a comment.

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

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