unit tests ensure broken comparators don't put null to the start of t…

…he set and than an empty set is never sorted

src/META-INF/test-interfaces.devellibdepends

@@ -1,2 +1,3 @@
 junit-4.10.jar
+mockito-all-1.9.5.jar
 

src/test/org/openscience/cdk/interfaces/AbstractAtomContainerSetTest.java

@@ -25,8 +25,12 @@
 
 import org.junit.Assert;
 import org.junit.Test;
+import org.mockito.Mockito;
 import org.openscience.cdk.tools.manipulator.AtomContainerComparator;
 
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.verify;
+
 /**
  * Checks the functionality of {@link IAtomContainerSet} implementations.
  *
@@ -59,6 +63,76 @@ public void testSortAtomContainers_Comparator_Null() {
         Assert.assertEquals(2, som.getAtomContainer(1).getAtomCount());
     }
 
+    /**
+     * Ensures that sort method of the AtomContainerSet does not include nulls
+     * in the comparator. This is tested using a comparator which sorts null
+     * values as low and thus to the start of an array. By adding two (non-null)
+     * values and sorting we should see that the first two values are not null
+     * despite giving a comparator which sorts null as low.
+     *
+     * @cdk.bug 1291
+     */
+    @Test public void testSort_BrokenComparator() {
+
+        IAtomContainerSet set = (IAtomContainerSet) newChemObject();
+
+        IChemObjectBuilder builder = set.getBuilder();
+
+        IAtomContainer a = builder.newInstance(IAtomContainer.class);
+        IAtomContainer b = builder.newInstance(IAtomContainer.class);
+
+        a.addAtom(builder.newInstance(IAtom.class, "C"));
+        a.addAtom(builder.newInstance(IAtom.class, "C"));
+
+        b.addAtom(builder.newInstance(IAtom.class, "C"));
+
+        set.addAtomContainer(a);
+        set.addAtomContainer(b);
+
+        // this comparator is deliberately broken but serves for the test
+        //  - null should be a high value (Interger.MAX)
+        //  - also, avoid boxed primitives in comparators
+        set.sortAtomContainers(new Comparator<IAtomContainer>() {
+
+            @Override public int compare(IAtomContainer o1, IAtomContainer o2) {
+                return size(o1).compareTo(size(o2));
+            }
+
+            public Integer size(IAtomContainer container) {
+                return container == null ? Integer.MIN_VALUE
+                                         : container.getAtomCount();
+            }
+
+        });
+
+        // despite null being low, the two atom containers should
+        // still be in the first slot
+        Assert.assertNotNull(set.getAtomContainer(0));
+        Assert.assertNotNull(set.getAtomContainer(1));
+        Assert.assertNull(set.getAtomContainer(2));
+
+    }
+
+    /**
+     * Ensure that sort is not called on an empty set. We mock the comparator
+     * and verify the compare method is never called
+     */
+    @Test public void testSort_empty() {
+
+        IAtomContainerSet set = (IAtomContainerSet) newChemObject();
+
+        @SuppressWarnings("unchecked")
+        Comparator<IAtomContainer> comparator = Mockito.mock(Comparator.class);
+
+        set.sortAtomContainers(comparator);
+
+        // verify the comparator was called 0 times
+        verify(comparator,
+               Mockito.times(0)).compare(any(IAtomContainer.class),
+                                         any(IAtomContainer.class));
+
+    }
+
     @Test public void testGetAtomContainerCount() {
         IAtomContainerSet som = (IAtomContainerSet)newChemObject();
         som.addAtomContainer(som.getBuilder().newInstance(IAtomContainer.class));