[COLLECTIONS-796] SetUniqueList.createSetBasedOnList doesn't add list…

… elements to return value

- Request in Comment: https://issues.apache.org/jira/browse/COLLECTIONS-796?focusedCommentId=17419058&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17419058
- Adressed typo in method name.
- Method is public therefore it must be backwards compatible.
- The method 'umodifiableListIterator' was not removed but annotated with @deprecated and calls the actual method 'unmodifiableListIterator'.
- Test was simplified, with appropriate assert-method-call

src/main/java/org/apache/commons/collections4/IteratorUtils.java

@@ -469,7 +469,7 @@ public static <E> Iterator<E> unmodifiableIterator(final Iterator<E> iterator) {
      * @return an immutable version of the iterator
      */
     public static <E> ListIterator<E> unmodifiableListIterator(final ListIterator<E> listIterator) {
-        return UnmodifiableListIterator.umodifiableListIterator(listIterator);
+        return UnmodifiableListIterator.unmodifiableListIterator(listIterator);
     }
 
     /**

src/main/java/org/apache/commons/collections4/iterators/UnmodifiableListIterator.java

@@ -43,7 +43,7 @@
      * @return a new unmodifiable list iterator
      * @throws NullPointerException if the iterator is null
      */
-    public static <E> ListIterator<E> umodifiableListIterator(final ListIterator<? extends E> iterator) {
+    public static <E> ListIterator<E> unmodifiableListIterator(final ListIterator<? extends E> iterator) {
         Objects.requireNonNull(iterator, "iterator");
         if (iterator instanceof Unmodifiable) {
             @SuppressWarnings("unchecked") // safe to upcast
@@ -53,6 +53,14 @@ public static <E> ListIterator<E> umodifiableListIterator(final ListIterator<? e
         return new UnmodifiableListIterator<>(iterator);
     }
 
+    /**
+     * @deprecated method name has typo in it. Use {@link org.apache.commons.collections4.iterators.UnmodifiableListIterator#unmodifiableListIterator(ListIterator)} instead.
+     */
+    @Deprecated
+    public static <E> ListIterator<E> umodifiableListIterator(final ListIterator<? extends E> iterator) {
+        return unmodifiableListIterator(iterator);
+    }
+
     /**
      * Constructor.
      *

src/main/java/org/apache/commons/collections4/list/SetUniqueList.java

@@ -352,6 +352,7 @@ protected Set<E> createSetBasedOnList(final Set<E> set, final List<E> list) {
                 subSet = new HashSet<>();
             }
         }
+        subSet.addAll(list);
         return subSet;
     }
 

src/main/java/org/apache/commons/collections4/list/UnmodifiableList.java

@@ -118,12 +118,12 @@ public boolean retainAll(final Collection<?> coll) {
 
     @Override
     public ListIterator<E> listIterator() {
-        return UnmodifiableListIterator.umodifiableListIterator(decorated().listIterator());
+        return UnmodifiableListIterator.unmodifiableListIterator(decorated().listIterator());
     }
 
     @Override
     public ListIterator<E> listIterator(final int index) {
-        return UnmodifiableListIterator.umodifiableListIterator(decorated().listIterator(index));
+        return UnmodifiableListIterator.unmodifiableListIterator(decorated().listIterator(index));
     }
 
     @Override

src/main/java/org/apache/commons/collections4/map/LinkedMap.java

@@ -307,12 +307,12 @@ public Iterator<K> iterator() {
 
         @Override
         public ListIterator<K> listIterator() {
-            return UnmodifiableListIterator.umodifiableListIterator(super.listIterator());
+            return UnmodifiableListIterator.unmodifiableListIterator(super.listIterator());
         }
 
         @Override
         public ListIterator<K> listIterator(final int fromIndex) {
-            return UnmodifiableListIterator.umodifiableListIterator(super.listIterator(fromIndex));
+            return UnmodifiableListIterator.unmodifiableListIterator(super.listIterator(fromIndex));
         }
 
         @Override

src/test/java/org/apache/commons/collections4/iterators/UnmodifiableListIteratorTest.java

@@ -24,13 +24,14 @@
 
 import org.apache.commons.collections4.Unmodifiable;
 
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
 /**
  * Tests the UnmodifiableListIterator.
- *
  */
 public class UnmodifiableListIteratorTest<E> extends AbstractListIteratorTest<E> {
 
-    protected String[] testArray = { "One", "Two", "Three" };
+    protected String[] testArray = {"One", "Two", "Three"};
     protected List<E> testList;
 
     public UnmodifiableListIteratorTest(final String testName) {
@@ -49,12 +50,12 @@ protected void setUp() throws Exception {
 
     @Override
     public ListIterator<E> makeEmptyIterator() {
-        return UnmodifiableListIterator.umodifiableListIterator(Collections.<E>emptyList().listIterator());
+        return UnmodifiableListIterator.unmodifiableListIterator(Collections.<E>emptyList().listIterator());
     }
 
     @Override
     public ListIterator<E> makeObject() {
-        return UnmodifiableListIterator.umodifiableListIterator(testList.listIterator());
+        return UnmodifiableListIterator.unmodifiableListIterator(testList.listIterator());
     }
 
     @Override
@@ -78,15 +79,12 @@ public void testListIterator() {
 
     public void testDecorateFactory() {
         ListIterator<E> it = makeObject();
-        assertSame(it, UnmodifiableListIterator.umodifiableListIterator(it));
+        assertSame(it, UnmodifiableListIterator.unmodifiableListIterator(it));
 
         it = testList.listIterator();
-        assertTrue(it != UnmodifiableListIterator.umodifiableListIterator(it));
+        assertNotSame(it, UnmodifiableListIterator.unmodifiableListIterator(it));
 
-        try {
-            UnmodifiableListIterator.umodifiableListIterator(null);
-            fail();
-        } catch (final NullPointerException ex) {}
+        assertThrows(NullPointerException.class, () -> UnmodifiableListIterator.unmodifiableListIterator(null));
     }
 
 }

src/test/java/org/apache/commons/collections4/list/SetUniqueListTest.java

@@ -16,14 +16,13 @@
  */
 package org.apache.commons.collections4.list;
 
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.HashSet;
-import java.util.LinkedList;
-import java.util.List;
-import java.util.ListIterator;
-import java.util.Set;
+import org.apache.commons.collections4.set.UnmodifiableSet;
+import org.junit.Assert;
+import org.junit.jupiter.api.Assertions;
+
+import java.util.*;
+
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 /**
  * JUnit tests.
@@ -194,7 +193,7 @@ public void testCollections307() {
         // repeat the test with a different class than HashSet;
         // which means subclassing SetUniqueList below
         list = new ArrayList<>();
-        uniqueList = new SetUniqueList307(list, new java.util.TreeSet<E>());
+        uniqueList = new SetUniqueList307(list, new TreeSet<E>());
 
         uniqueList.add((E) hello);
         uniqueList.add((E) world);
@@ -558,12 +557,8 @@ public void testSetUpwardsInList() {
     public void testSubListIsUnmodifiable() {
         resetFull();
         final List<E> subList = getCollection().subList(1, 3);
-        try {
-            subList.remove(0);
-            fail("subList should be unmodifiable");
-        } catch (final UnsupportedOperationException e) {
-            // expected
-        }
+        assertEquals(2, subList.size());
+        assertThrows(UnsupportedOperationException.class, () -> subList.remove(0));
     }
 
     @SuppressWarnings("unchecked")
@@ -616,11 +611,31 @@ public void verify() {
         }
     }
 
-//    public void testCreate() throws Exception {
-//        resetEmpty();
-//        writeExternalFormToDisk((java.io.Serializable) getCollection(), "src/test/resources/data/test/SetUniqueList.emptyCollection.version4.obj");
-//        resetFull();
-//        writeExternalFormToDisk((java.io.Serializable) getCollection(), "src/test/resources/data/test/SetUniqueList.fullCollection.version4.obj");
-//    }
+    @SuppressWarnings("unchecked")
+    public void testCreateSetBasedOnList() {
+        final List<String> list = new ArrayList<>();
+        list.add("One");
+        list.add("Two");
+        @SuppressWarnings("rawtypes") final SetUniqueList setUniqueList = (SetUniqueList) makeObject();
+
+        // Standard case with HashSet
+        final Set<String> setBasedOnList = setUniqueList.createSetBasedOnList(new HashSet<>(), list);
+        assertEquals(list.size(), setBasedOnList.size());
+        list.forEach(item -> assertTrue(setBasedOnList.contains(item)));
+
+        // Use different Set than HashSet
+        final Set<String> setBasedOnList1 = setUniqueList.createSetBasedOnList(new TreeSet<>(), list);
+        assertEquals(list.size(), setBasedOnList1.size());
+        list.forEach(item -> assertTrue(setBasedOnList1.contains(item)));
+
+        // throws internally NoSuchMethodException --> results in HashSet
+        final Set<String> setBasedOnList2 = setUniqueList.createSetBasedOnList(UnmodifiableSet.unmodifiableSet(new HashSet<>()), list);
+        assertEquals(list.size(), setBasedOnList2.size());
+        list.forEach(item -> assertTrue(setBasedOnList2.contains(item)));
+
+        // provide null values as Parameter
+        assertThrows(NullPointerException.class, () -> setUniqueList.createSetBasedOnList(null, list));
+        assertThrows(NullPointerException.class, () -> setUniqueList.createSetBasedOnList(new HashSet<>(), null));
 
+    }
 }