On Sun, 23 May 2021 15:07:04 GMT, Michael Strauß <mstra...@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
