On Sun, 2 Apr 2023 00:31:51 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> Michael Strauß has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> addressed review comments, added tests
>
> modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableListWrapperTest.java
> line 41:
>
>> 39:
>> 40: @Nested
>> 41: class RemoveAllTest {
>
> Empty line after class declaration.
>
> Same for other places.
I see that all over the JavaFX codebase. Do we have any guidance for that?
> modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableListWrapperTest.java
> line 57:
>
>> 55: });
>> 56:
>> 57: list.removeAll(Collections.<String>emptyList());
>
> You can use `List.of()` instead of `Collections.emptyList()` if you want here
> and in other places.
Done.
> modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableMapWrapperTest.java
> line 65:
>
>> 63:
>> 64: @Test
>> 65: public void testValueSetNullArgumentThrowsNPE() {
>
> The values collection is not a `Set`, but the intention is clear. Same in
> other places.
Fixed that.
> modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableMapWrapperTest.java
> line 108:
>
>> 106:
>> 107: var map2 = new ObservableMapWrapper<>(new
>> HashMap<>(Map.of("k0", "v0", "k1", "v1", "k2", "v2")));
>> 108: assertThrows(NullPointerException.class, () ->
>> map2.entrySet().retainAll((Collection<?>) null));
>
> Would advise to rename `map1` to `emptyMap` and `map2` to `notEmptyMap` or
> the like.
>
> Same in other places.
Done.
> modules/javafx.base/src/test/java/test/com/sun/javafx/collections/ObservableSequentialListWrapperTest.java
> line 55:
>
>> 53: };
>> 54:
>> 55: assertDoesNotThrow(() -> list.addAll(Collections.emptyList()));
>
> The `addAll` method here does not belong to
> `ObservableSequentialListWrapper`, but to `ModifiableObservableListBase`,
> which is tested in another file. Not sure if this method test helps.
Good catch, I've removed the method test.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155395211
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155394964
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155395046
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155395091
PR Review Comment: https://git.openjdk.org/jfx/pull/751#discussion_r1155394994
