On Fri, 20 Mar 2026 17:38:47 GMT, Andy Goryachev <[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. > > 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? But does it make sense? I mean, we literally `add` and `remove` the items. And want to test the change, so I think this will just make the test bigger. > 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`? I checked the test coverage and the method in question is covered completely. The whole `ListChangeBuilder` is nearly completely covered, so I did think we are good here. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/2117#discussion_r2967253047 PR Review Comment: https://git.openjdk.org/jfx/pull/2117#discussion_r2967259269
