From e03c2216d99349af7d60bcb6b8c586ebd2e33448 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Fri, 2 Nov 2018 22:57:13 +0800 Subject: [PATCH] Renaming identifiers, documentation. #2643. --- .../common/diff/Difference.java | 3 ++ ...ricObjectDiffer.java => ObjectDiffer.java} | 32 ++++++++------ ...tDifferTest.java => ObjectDifferTest.java} | 32 ++++++-------- .../routing/graph/GraphSerializationTest.java | 43 ++++++++----------- 4 files changed, 52 insertions(+), 58 deletions(-) rename src/main/java/org/opentripplanner/common/diff/{GenericObjectDiffer.java => ObjectDiffer.java} (92%) rename src/test/java/org/opentripplanner/common/diff/{GenericObjectDifferTest.java => ObjectDifferTest.java} (71%) diff --git a/src/main/java/org/opentripplanner/common/diff/Difference.java b/src/main/java/org/opentripplanner/common/diff/Difference.java index da16f0347fa..f147d69de9c 100644 --- a/src/main/java/org/opentripplanner/common/diff/Difference.java +++ b/src/main/java/org/opentripplanner/common/diff/Difference.java @@ -15,6 +15,9 @@ package org.opentripplanner.common.diff; +/** + * This represents a single difference found between two objects by the ObjectDiffer. + */ public class Difference { Object a; diff --git a/src/main/java/org/opentripplanner/common/diff/GenericObjectDiffer.java b/src/main/java/org/opentripplanner/common/diff/ObjectDiffer.java similarity index 92% rename from src/main/java/org/opentripplanner/common/diff/GenericObjectDiffer.java rename to src/main/java/org/opentripplanner/common/diff/ObjectDiffer.java index 4e44d5ceae5..9181ba78dda 100644 --- a/src/main/java/org/opentripplanner/common/diff/GenericObjectDiffer.java +++ b/src/main/java/org/opentripplanner/common/diff/ObjectDiffer.java @@ -12,15 +12,15 @@ /** * Perform a recursive deep comparison of objects. - * This is intended only for testing that graph building is reproducible and serialization restores an identical graph. - * It should be kept simple. + * This is intended for testing that graph building is reproducible and serialization restores an identical graph. + * It should be kept relatively simple. * - * This class is not threadsafe. It holds configuration and state internally, and should therefore be used for one - * comparison only and thrown away. + * This class is not threadsafe. It holds configuration and state internally. Each instance should therefore be used + * for one comparison only and thrown away. */ -public class GenericObjectDiffer { +public class ObjectDiffer { - private static final Logger LOG = LoggerFactory.getLogger(GenericObjectDiffer.class); + private static final Logger LOG = LoggerFactory.getLogger(ObjectDiffer.class); /** * The maximum recursion depth during the comparison. @@ -53,13 +53,13 @@ public class GenericObjectDiffer { private int nObjectsCompared = 0; - /** - * If true, execute the whole comparison procedure, even for two identical object references. - * Normally these comparisons would be optimized away since they are by definition identical trees of objects. - * However this is useful in tests of the comparison system itself - differences should be impossible. - */ private boolean compareIdenticalObjects = false; + /** + * Tell the Differ to execute the whole comparison procedure even when comparing two identical object references. + * Normally such comparisons would be optimized away since they are by definition identical trees of objects. + * However this is useful in tests of the comparison system itself - there are known to be no differences. + */ public void enableComparingIdenticalObjects() { compareIdenticalObjects = true; } @@ -88,6 +88,10 @@ public void useEquals(Class... classes) { useEquals.addAll(Arrays.asList(classes)); } + /** + * This is the main entry point to compare two objects. It's also called recursively for many other + * complex objects in the tree. + */ public void compareTwoObjects (Object a, Object b) { nObjectsCompared += 1; if (compareIdenticalObjects) { @@ -149,8 +153,8 @@ else if (a instanceof Multimap) { compareMaps(new MultimapWrapper((Multimap) a), new MultimapWrapper((Multimap) b)); } else if (a instanceof Collection) { - compareCollections((Collection) a, (Collection) b); // TODO maybe even generalize this to iterable? + compareCollections((Collection) a, (Collection) b); } else if (classToCompare.isArray()){ compareArrays(a, b); @@ -162,8 +166,8 @@ else if (classToCompare.isArray()){ } /** - * The fallback comparison method - compare every field of the two objects. - * Note that the two objects passed in must already be known to be of the same class. + * The fallback comparison method - compare every field of two objects of the same class. + * Note that the two objects passed in must already be known to be of the same class by the caller. */ private void compareFieldByField (Object a, Object b) { Class classToCompare = a.getClass(); diff --git a/src/test/java/org/opentripplanner/common/diff/GenericObjectDifferTest.java b/src/test/java/org/opentripplanner/common/diff/ObjectDifferTest.java similarity index 71% rename from src/test/java/org/opentripplanner/common/diff/GenericObjectDifferTest.java rename to src/test/java/org/opentripplanner/common/diff/ObjectDifferTest.java index ac2f45de396..599accdf9c2 100644 --- a/src/test/java/org/opentripplanner/common/diff/GenericObjectDifferTest.java +++ b/src/test/java/org/opentripplanner/common/diff/ObjectDifferTest.java @@ -1,17 +1,9 @@ package org.opentripplanner.common.diff; -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; import java.util.Map; import java.util.stream.IntStream; @@ -20,20 +12,20 @@ import static org.junit.Assert.assertTrue; /** - * This test verifies that the GenericObjectDiffer can find differences in nested objects. + * This test verifies that the ObjectDiffer can find differences in nested objects. * It is expected to automatically perform a recursive, semantic comparison of the two supplied objects. */ -public class GenericObjectDifferTest { +public class ObjectDifferTest { @Test public void testDiff() throws IllegalAccessException { { // Compare two separate essentially empty graphs. - GenericObjectDiffer genericObjectDiffer = new GenericObjectDiffer(); - genericObjectDiffer.ignoreFields("graphBuilderAnnotations", "streetNotesService", "vertexById", "buildTime", "modCount"); + ObjectDiffer objectDiffer = new ObjectDiffer(); + objectDiffer.ignoreFields("graphBuilderAnnotations", "streetNotesService", "vertexById", "buildTime", "modCount"); Graph graph1 = new Graph(); Graph graph2 = new Graph(); - genericObjectDiffer.compareTwoObjects(graph1, graph2); + objectDiffer.compareTwoObjects(graph1, graph2); } // Ideally we'd compare two separate but identical complex graphs. @@ -49,17 +41,17 @@ public void testDiff() throws IllegalAccessException { // No differences should be found between the two. Map o1 = makeFieldsMap(); Map o2 = makeFieldsMap(); - GenericObjectDiffer genericObjectDiffer = new GenericObjectDiffer(); - genericObjectDiffer.compareTwoObjects(o1, o2); - assertFalse(genericObjectDiffer.hasDifferences()); + ObjectDiffer objectDiffer = new ObjectDiffer(); + objectDiffer.compareTwoObjects(o1, o2); + assertFalse(objectDiffer.hasDifferences()); // Now make one of the two nested objects different. Differences should be found. o2.get(10).nestedMap.get("50").clear(); // This should fail - genericObjectDiffer = new GenericObjectDiffer(); // TODO add a reset function - genericObjectDiffer.compareTwoObjects(o1, o2); - assertTrue(genericObjectDiffer.hasDifferences()); - genericObjectDiffer.printDifferences(); + objectDiffer = new ObjectDiffer(); // TODO add a reset function + objectDiffer.compareTwoObjects(o1, o2); + assertTrue(objectDiffer.hasDifferences()); + objectDiffer.printDifferences(); } } diff --git a/src/test/java/org/opentripplanner/routing/graph/GraphSerializationTest.java b/src/test/java/org/opentripplanner/routing/graph/GraphSerializationTest.java index 3a868a1b8c5..607747a9111 100644 --- a/src/test/java/org/opentripplanner/routing/graph/GraphSerializationTest.java +++ b/src/test/java/org/opentripplanner/routing/graph/GraphSerializationTest.java @@ -6,7 +6,7 @@ import org.jets3t.service.io.TempFile; import org.junit.Test; import org.opentripplanner.ConstantsForTests; -import org.opentripplanner.common.diff.GenericObjectDiffer; +import org.opentripplanner.common.diff.ObjectDiffer; import org.opentripplanner.common.geometry.HashGridSpatialIndex; import org.opentripplanner.routing.impl.DefaultStreetVertexIndexFactory; import org.opentripplanner.routing.trippattern.Deduplicator; @@ -26,16 +26,11 @@ * Tests that saving a graph and reloading it (round trip through serialization and deserialization) does not corrupt * the graph, and yields exactly the same data. * - * Candidates for performing the comparison included: - * DeepEquals from https://github.com/jdereg/java-util - * isEqualToComparingFieldByFieldRecursively() from http://joel-costigliola.github.io/assertj/ + * We tried several existing libraries to perform the comparison but nothing did exactly what we needed in a way that + * we could control precisely. * - * We need to use a system that can detect cycles, and one that can display or record any differences encountered. - * DeepEquals cannot record what differences it encounters. isEqualToComparingFieldByFieldRecursively finds - * differences but there are so many it runs out of memory. - * - * So currently I'm using the object differ supplied by csolem via the Entur OTP branch at - * https://github.com/entur/OpenTripPlanner/tree/protostuff_poc + * The object differ we're using started out as a copy of the one supplied by csolem via the Entur OTP branch at + * https://github.com/entur/OpenTripPlanner/tree/protostuff_poc but has been mostly rewritten at this point. * * Created by abyrd on 2018-10-26 */ @@ -57,13 +52,13 @@ public void testRoundTrip () throws Exception { // We can exclude relatively few classes here, the two trees are of course perfectly identical. // We do skip edge lists - otherwise this does a depth-first search of the graph causing a stack overflow. // And also some deeply buried weak-value hash maps, which refuse to tell you what their keys are. - GenericObjectDiffer genericObjectDiffer = new GenericObjectDiffer(); - genericObjectDiffer.ignoreFields("incoming", "outgoing"); - genericObjectDiffer.ignoreClasses(WeakValueHashMap.class); - genericObjectDiffer.enableComparingIdenticalObjects(); - genericObjectDiffer.compareTwoObjects(originalGraph, originalGraph); - genericObjectDiffer.printDifferences(); - assertFalse(genericObjectDiffer.hasDifferences()); + ObjectDiffer objectDiffer = new ObjectDiffer(); + objectDiffer.ignoreFields("incoming", "outgoing"); + objectDiffer.ignoreClasses(WeakValueHashMap.class); + objectDiffer.enableComparingIdenticalObjects(); + objectDiffer.compareTwoObjects(originalGraph, originalGraph); + objectDiffer.printDifferences(); + assertFalse(objectDiffer.hasDifferences()); // Now round-trip the graph through serialization. File tempFile = TempFile.createTempFile("graph", "pdx"); @@ -72,19 +67,19 @@ public void testRoundTrip () throws Exception { // This time we need to make some exclusions because some classes are inherently transient or contain // unordered lists we can't yet compare. - genericObjectDiffer = new GenericObjectDiffer(); + objectDiffer = new ObjectDiffer(); // Skip incoming and outgoing edge lists. These are unordered lists which will not compare properly. // The edges themselves will be compared via another field, and the edge lists are reconstructed after deserialization. - genericObjectDiffer.ignoreFields("incoming", "outgoing"); - genericObjectDiffer.useEquals(BitSet.class, LineString.class, Polygon.class); + objectDiffer.ignoreFields("incoming", "outgoing"); + objectDiffer.useEquals(BitSet.class, LineString.class, Polygon.class); // HashGridSpatialIndex contains unordered lists in its bins. This is rebuilt after deserialization anyway. // The deduplicator in the loaded graph will be empty, because it is transient and only fills up when items // are deduplicated. - genericObjectDiffer.ignoreClasses(HashGridSpatialIndex.class, ThreadPoolExecutor.class, Deduplicator.class); + objectDiffer.ignoreClasses(HashGridSpatialIndex.class, ThreadPoolExecutor.class, Deduplicator.class); - genericObjectDiffer.compareTwoObjects(originalGraph, copiedGraph); - assertFalse(genericObjectDiffer.hasDifferences()); - genericObjectDiffer.printDifferences(); + objectDiffer.compareTwoObjects(originalGraph, copiedGraph); + assertFalse(objectDiffer.hasDifferences()); + objectDiffer.printDifferences(); } }