On Fri, 20 Mar 2026 11:35:35 GMT, Marius Hanl <[email protected]> wrote:
> This PR fixes the bug where list changes could be wrong (incorrect index).
> Nearly everything was already in place, but one usecase that had a minor flaw.
>
> Fix is simple, the index we use for iterating must be adjusted in case we
> remove a change from `addRemoveChanges`.
>
> All tests are green and I also added the Tests that were in the ticket.
> Adjusted to JUnit 5 and with a bit of explaination which helped me a lot to
> better understand the issue and fix it.
All the new tests fail in master and pass with the fix.
Noticed a few minor things in the test, otherwise looks good!
modules/javafx.base/src/test/java/test/javafx/collections/ListChangeBuilderTest.java
line 642:
> 640: list.addListener((ListChangeListener.Change<? extends Integer>
> change) -> {
> 641: change.next();
> 642: assertEquals(1, change.getRemovedSize());
I wonder if instead of simply checking the size of the sub-arrays we could
verify the exact content, which will test for size AND the order.
Something like
assertList(change.getRemoved(), 2);
what do you think?
modules/javafx.base/src/test/java/test/javafx/collections/ListChangeBuilderTest.java
line 665:
> 663: list.remove(1);
> 664:
> 665: list.doEndChange();
... and, just for fun, perhaps check that the right content remains in the list
after the change?
modules/javafx.base/src/test/java/test/javafx/collections/ListChangeBuilderTest.java
line 710:
> 708: list.doEndChange();
> 709: }
> 710:
Question: do the new 4 tests provide a complete coverage as far as the bug is
concerned?
For example, do we need to cover the following scenarios: `add-remove-add`, and
`remove-add-remove`?
modules/javafx.base/src/test/java/test/javafx/collections/ListChangeBuilderTest.java
line 722:
> 720: }
> 721: }
> 722:
extra newline?
-------------
PR Review: https://git.openjdk.org/jfx/pull/2117#pullrequestreview-3982965585
PR Review Comment: https://git.openjdk.org/jfx/pull/2117#discussion_r2967046106
PR Review Comment: https://git.openjdk.org/jfx/pull/2117#discussion_r2967100822
PR Review Comment: https://git.openjdk.org/jfx/pull/2117#discussion_r2967135777
PR Review Comment: https://git.openjdk.org/jfx/pull/2117#discussion_r2967028794
