revise object differ and serialization tests

The problem with the differ causing the test to fail was identified.
It does not check whether the outermost objects to be compared are Collections or Maps.

src/test/java/org/opentripplanner/common/diff/GenericObjectDifferTest.java

@@ -2,10 +2,13 @@
 
 import com.google.common.collect.Sets;
 import org.junit.Test;
+import org.opentripplanner.ConstantsForTests;
 import org.opentripplanner.common.TurnRestriction;
 import org.opentripplanner.routing.graph.Graph;
 import org.opentripplanner.routing.graph.Vertex;
 import org.opentripplanner.routing.services.notes.StaticStreetNotesSource;
+import org.opentripplanner.routing.vertextype.IntersectionVertex;
+import org.opentripplanner.routing.vertextype.StreetVertex;
 
 import java.util.HashMap;
 import java.util.List;
@@ -25,32 +28,56 @@ public class GenericObjectDifferTest {
     private GenericObjectDiffer genericObjectDiffer = new GenericObjectDiffer();
 
     private GenericDiffConfig genericDiffConfig = GenericDiffConfig.builder()
-            .ignoreFields(Sets.newHashSet("graphBuilderAnnotations", "streetNotesService", "vertexById", "buildTime"))
+            .ignoreFields(Sets.newHashSet("graphBuilderAnnotations", "streetNotesService", "vertexById", "buildTime", "modCount"))
             .identifiers(Sets.newHashSet("id", "index"))
             .useEqualsBuilder(Sets.newHashSet(TurnRestriction.class, StaticStreetNotesSource.class, Vertex.class))
             .build();
 
     private DiffPrinter diffPrinter = new DiffPrinter();
 
+    // TODO split into several tests
     @Test
     public void testDiff() throws IllegalAccessException {
-        Graph graph = new Graph();
-        Graph graph2 = new Graph();
-        List<Difference> differences = genericObjectDiffer.compareObjects(graph, graph2, genericDiffConfig);
-        assertTrue(differences.isEmpty());
-
-        // Create two semantically equal objects that are composed of completely separate instances.
-        // No differences shoudld be found between the two.
-        Map<String, Map<int[], String>> o1 = makeNestedObject();
-        Map<String, Map<int[], String>> o2 = makeNestedObject();
-        differences = genericObjectDiffer.compareObjects(o1, o2, genericDiffConfig);
-        assertTrue(differences.isEmpty());
-
-        // Now make one of the two nested objects different. Differences should be found.
-        o2.get("50").clear();
-        differences = genericObjectDiffer.compareObjects(o1, o2, genericDiffConfig);
-        // This assertion fails - the differ gives a false negative when the map values are different.
-        // assertFalse(differences.isEmpty());
+        {
+            // Compare two separate essentially empty graphs.
+            Graph graph1 = new Graph();
+            Graph graph2 = new Graph();
+            List<Difference> differences = genericObjectDiffer.compareObjects(graph1, graph2, genericDiffConfig);
+            assertTrue(differences.isEmpty());
+        }
+
+        // Ideally we'd compare two separate but identical complex graphs.
+        // A test that builds the same graph twice will fail for the following reasons:
+        // There is global state in Vertex.index and the feeds IDs that mean if you build the same graph twice
+        // the feed IDs and vertex index numbers will all be unique. This is however evidence that the diff
+        // tool is detecting changes in the graph contents.
+        // On the other hand deserializing the same graph twice or doing a round trip through serialization and
+        // deserialization should produce identical graphs.
+
+        {
+            // Create two semantically equal objects that are composed of completely separate instances.
+            // No differences should be found between the two.
+            Wrapper o1 = new Wrapper();
+            Wrapper o2 = new Wrapper();
+            List<Difference> differences = genericObjectDiffer.compareObjects(o1, o2, genericDiffConfig);
+            assertTrue(differences.isEmpty());
+
+            // Now make one of the two nested objects different. Differences should be found.
+            o2.wrappedItem.get("50").clear();
+            differences = genericObjectDiffer.compareObjects(o1, o2, genericDiffConfig);
+            // This assertion fails - the differ gives a false negative when the map values are different.
+            assertFalse(differences.isEmpty());
+        }
+
+    }
+
+    /**
+     * The diff code currently assumes the outermost object should be compared field-by-field (is not a Collection or
+     * Map and does not define semantic equals() method). Therefore we compare two appropriate objects wrapping complex
+     * nested collections.
+     */
+    private static class Wrapper {
+        Map<String, Map<int[], String>> wrappedItem = makeNestedObject();
     }
 
     public static Map<String, Map<int[], String>> makeNestedObject() {

src/test/java/org/opentripplanner/routing/graph/GraphSerializationTest.java

@@ -42,6 +42,8 @@ public class GraphSerializationTest {
     @Test
     public void testRoundTrip () throws Exception {
 
+        // This graph does not make an ideal test because it doesn't have any street data.
+        // TODO switch to another graph that has both GTFS and OSM data
         Graph originalGraph = ConstantsForTests.getInstance().getPortlandGraph();
         originalGraph.index(new DefaultStreetVertexIndexFactory());