RFR: 8253086: Optimization of removeAll and retainAll of ObservableListWrapper

yosbits github.com+7517141+yososs at openjdk.java.net
Tue Sep 22 02:19:09 UTC 2020

On Mon, 21 Sep 2020 11:19:30 GMT, yosbits <github.com+7517141+yososs at openjdk.org> wrote:

>> modules/javafx.base/src/main/java/com/sun/javafx/collections/ObservableListWrapper.java line 191:
>>> 189:         if (this.isEmpty() || c.isEmpty()) {
>>> 190:             return false;
>>> 191:         }
>> logic seems wrong: if c is empty this must be cleared
>> The error shows up in a failing test (but only when running all base.collections tests in Eclipse, not with gradle on
>> the commandline)
>> the test failure is in ObservableListWithExtractor.testUpdate_retainAll() which - besides exposing the error above -
>> tells us two issues with the tests
>> a) the normal (that is no extractor) test is incomplete (in not testing retainAll with empty array/collection)
>> b) the test is not run with gradle because it doesn't adhere to naming conventions ;)
> I realized I was wrong. Thank you. I would probably need to add a test for this case.
> I plan to push new changes around tomorrow.

I found that I needed to add at least a retainAll test, but I also found that there was no test class to directly test

Should I add a test for this non-public API class?
When I add it, I'm not going to write test code for methods other than removeAll and retainAll.


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

More information about the openjfx-dev mailing list