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
ObservableListWrapper.
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