RFR: 8196065: ListChangeListener getRemoved() returns items that were not removed. [v6]

[Jeanette Winzenburg]

On Sun, 23 May 2021 15:07:04 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/SelectedItemsReadOnlyObservableListTest.java line 120:
>> 
>>> 118:         assertEquals(change(replaced(0, range("foo"), range("bar"))), changes.get(0));
>>> 119:     }
>>> 120: 
>> 
>> Another thingy I don't quite understand, sry ;) 
>> 
>> What do you want to test here? We have:
>> 
>> selectedItems before: [foo, bar]
>> selectedItems after: [bar, foo]
>> 
>> so we need a change (the "should be" snippet in the description of this method)
>> 
>> replaced [foo, bar] by [bar, foo] at 0  
>> 
>> we get: 
>> 
>> replaced [foo] by [bar] at 0
>> 
>> which is an incorrect notification (because it's incomplete, doesn't notify at all about the replace at 1) covered by a passing test .. feels fishy. The other way round: if selectedItems cannot react to such a use-case with a correct notification, it might be broken?
>
> Yes, `selectedItems` is broken. I agree that having an objectively incorrect test pass is fishy, but the alternative would be to not have the test at all, and lose the "free" heads-up if at some point in the future the underlying issue is fixed. Do you think it's better to have the correct test, but ignore it?

well, false greens are the worst we can have, IMO :). I think the test should concentrate on the isolated selectedItems behavior which __must__ propagate all notifications that it receives (from indices) in terms of items. If it fails to do so, it's a failure of its own implementation and should produce a failing test. 

Consider two scenarios: 

A: indices firing 2 separate changes

    selectedIndices.replaceAll(i -> i == 0 ? 1 : 0);
    assertEquals(2, changes.size());
    assertEquals(change(replaced(0, range("foo"), range("bar"))), changes.get(0));
    assertEquals(change(replaced(1, range("bar"), range("foo"))), changes.get(1));

B: indices firing a composed change:

    selectedIndices._beginChange();
    selectedIndices.replaceAll(i -> i == 0 ? 1 : 0);
    selectedIndices._endChange();
    assertEquals(indicesChanges.size(), changes.size());
    assertEquals(1, changes.size());
    assertEquals(change(replaced(0, range("foo", "bar"), range("bar", "foo"))), changes.get(0));

B passes both before and after the fix, A fails both before and after the fix. Whether or not that can be helped currently, is a different story. If can't be solved right now, I would suggest to keep it failing, file an issue about it and ignore the test with the issue id.

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

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