Refactor unit tests and helper methods

- Refactors the helper functions for bidirectionalizing and reversing
graphs, so that they do not mutate the input graphs but rather return
new graphs. (Mutating function arguments is a bad smell in general, and
should be avoided when not necessary.)

- Create a new helper base test class with assertions for graph
isomorphism and that a graph is isomorphic to a provided iterator of
edges.

- Write more extensive tests for the bidirectional and reverse
functions.

Test plan: `python3 infra/graph_generators_test.py`

infra/graph_generators.py

@@ -5,28 +5,35 @@
 import networkx as nx
 
 
-def make_bidir(graph):
-    """this function adds an edge with opposite direction for each edge present"""
-    edges = graph.copy().edges
-    for e in edges:
-        graph.add_edge(e[1], e[0])
-
-    return graph
+def bidirectional(graph):
+    """
+    Returns a bidirectional copy of the input graph.
 
+    For every edge in the input graph, the bidirectional graph
+    has a corresponding reversed edge.
 
-# this function adds an edge with opposite direction for each edge present
-# and removed the original edge
-def reverse_dir(graph):
+    Does not mutate the input graph.
     """
-    this function adds an edge with opposite direction for each edge present
-    and removed the original edge
+    output = nx.MultiDiGraph()
+    for e in graph.edges:
+        output.add_edge(e[0], e[1])
+        output.add_edge(e[1], e[0])
+    return output
+
+
+def reverse(graph):
     """
-    edges = graph.copy().edges
-    for e in edges:
-        graph.add_edge(e[1], e[0])
-        graph.remove_edge(e[0], e[1])
+    Returns a reversed direction copy of the input graph.
 
-    return graph
+    For every edge in the input graph, there will be a reversed edge in the
+    resultant graph.
+
+    Does not mutate the input graph.
+    """
+    output = nx.MultiDiGraph()
+    for e in graph.edges:
+        output.add_edge(e[1], e[0])
+    return output
 
 
 def set_types(graph, node_types="vanilla", edge_types="vanilla"):
@@ -87,7 +94,7 @@ def line_graph_gen(
 
     graph = nx.path_graph(num_nodes, create_using=nx.MultiDiGraph)
     if bidir:
-        graph = make_bidir(graph)
+        graph = bidirectional(graph)
 
     graph = set_types(graph, edge_types=node_type_name, node_types=edge_type_name)
 
@@ -138,7 +145,7 @@ def circle_graph_gen(
     graph = nx.cycle_graph(num_nodes, create_using=nx.MultiDiGraph)
 
     if bidir:
-        graph = make_bidir(graph)
+        graph = bidirectional(graph)
 
     graph = set_types(graph, edge_types=node_type_name, node_types=edge_type_name)
 
@@ -166,9 +173,9 @@ def tree_graph_gen(
     graph = nx.balanced_tree(rate, height, create_using=nx.MultiDiGraph)
 
     if kind == "sink":
-        graph = reverse_dir(graph)
+        graph = reverse(graph)
     elif kind == "bidir":
-        graph = make_bidir(graph)
+        graph = bidirectional(graph)
 
     graph = set_types(graph, edge_types=node_type_name, node_types=edge_type_name)
 

infra/graph_generators_test.py

@@ -10,88 +10,162 @@
     circle_graph_gen,
     star_graph_gen,
     tree_graph_gen,
-    make_bidir,
-    reverse_dir,
+    bidirectional,
+    reverse,
     set_types,
 )
 
 
-class Graph_Generators_Line_Graph(unittest.TestCase):
+class GraphTest(unittest.TestCase):
+    def assertIsopmorphic(self, g1, g2):
+        self.assertTrue(nx.is_isomorphic(g1, g2))
+
+    def assertIsomorphicEdges(self, g1, edges):
+        g2 = nx.MultiDiGraph()
+        g2.add_edges_from(edges)
+        self.assertIsopmorphic(g1, g2)
+
+
+class BidirectionalTest(GraphTest):
+    def test_empty_graph(self):
+        g = nx.MultiDiGraph()
+        bidir = bidirectional(g)
+        self.assertIsopmorphic(g, bidir)
+
+    def test_simple_graph(self):
+        g = nx.MultiDiGraph()
+        g.add_edge(1, 2)
+        bidir_expected = nx.MultiDiGraph()
+        bidir_expected.add_edge(1, 2)
+        bidir_expected.add_edge(2, 1)
+        bidir_actual = bidirectional(g)
+        self.assertIsopmorphic(bidir_expected, bidir_actual)
+
+    def test_bidirectional_graph(self):
+        g = nx.MultiDiGraph()
+        g.add_edge(1, 2)
+        g.add_edge(2, 1)
+        # Since it's a multidigraph, every edge gets doubled
+        expected_bidir = nx.MultiDiGraph()
+        expected_bidir.add_edges_from([(1, 2), (2, 1), (2, 1), (1, 2)])
+        self.assertIsopmorphic(expected_bidir, bidirectional(g))
+
+    def test_non_mutating(self):
+        g = nx.MultiDiGraph()
+        g.add_edge(1, 2)
+        g.add_edge(1, 3)
+        g_copy = g.copy()
+        bidirectional(g)
+        self.assertIsopmorphic(g, g_copy)
+
+    def test_path(self):
+        g = bidirectional(nx.path_graph(3, create_using=nx.MultiDiGraph))
+        self.assertIsomorphicEdges(g, [(0, 1), (1, 0), (1, 2), (2, 1)])
+
+
+class ReversedTest(GraphTest):
+    def test_empty_graph(self):
+        g = nx.MultiDiGraph()
+        r = reverse(g)
+        self.assertIsopmorphic(g, r)
+
+    def test_simple_graph(self):
+        g = nx.MultiDiGraph()
+        # Putting in 3 nodes so the reversed graph and the input aren't isomorphic
+        g.add_edge(1, 2)
+        g.add_edge(1, 3)
+        reversed_expected = nx.MultiDiGraph()
+        reversed_expected.add_edge(2, 1)
+        reversed_expected.add_edge(3, 1)
+        reversed_actual = reverse(g)
+        self.assertIsopmorphic(reversed_expected, reversed_actual)
+
+    def test_path(self):
+        g = nx.path_graph(4, create_using=nx.MultiDiGraph)
+        g = reverse(g)
+        self.assertIsomorphicEdges(g, [(1, 0), (2, 1), (3, 2)])
+
+    def test_bidirectional_graph(self):
+        g = nx.MultiDiGraph()
+        g.add_edge(1, 2)
+        g.add_edge(2, 1)
+        self.assertIsopmorphic(g, reverse(g))
+
+    def test_non_mutating(self):
+        g = nx.MultiDiGraph()
+        g.add_edge(1, 2)
+        g.add_edge(1, 3)
+        g_copy = g.copy()
+        reverse(g)
+        self.assertIsopmorphic(g, g_copy)
+
+
+class Graph_Generators_Line_Graph(GraphTest):
     def test_line_graph_gen_sanity(self):
         g = line_graph_gen(4)
-        self.assertEqual(list(g.edges()), [(0, 1), (1, 2), (2, 3)])
+        self.assertIsomorphicEdges(g, [(0, 1), (1, 2), (2, 3)])
 
     def test_line_graph_gen_bidir(self):
         g = line_graph_gen(4, bidir=True)
-        # (node, neighbor)
-        self.assertEqual(
-            list(g.edges()), [(0, 1), (1, 2), (1, 0), (2, 3), (2, 1), (3, 2)]
-        )
+        self.assertIsomorphicEdges(g, [(0, 1), (1, 2), (1, 0), (2, 3), (2, 1), (3, 2)])
 
     def test_line_graph_gen_bogus_bidir(self):
         with self.assertRaises(ValueError):
             line_graph_gen(4, bidir="troll")
 
 
-class Graph_Generators_Circle_Graph(unittest.TestCase):
+class Graph_Generators_Circle_Graph(GraphTest):
     def test_circle_graph_gen_sanity(self):
         g = circle_graph_gen(4)
-        self.assertEqual(list(g.edges()), [(0, 1), (1, 2), (2, 3), (3, 0)])
+        self.assertIsomorphicEdges(g, [(0, 1), (1, 2), (2, 3), (3, 0)])
 
     def test_circle_graph_gen_bidir(self):
         g = circle_graph_gen(4, bidir=True)
         # (node, neighbor)
-        self.assertEqual(
-            list(g.edges()),
-            [(0, 1), (0, 3), (1, 2), (1, 0), (2, 3), (2, 1), (3, 0), (3, 2)],
+        self.assertIsomorphicEdges(
+            g, [(0, 1), (0, 3), (1, 2), (1, 0), (2, 3), (2, 1), (3, 0), (3, 2)]
         )
 
     def test_circle_graph_gen_bogus_bidir(self):
         with self.assertRaises(ValueError):
             circle_graph_gen(4, bidir="troll")
 
 
-class Graph_Generators_Star_Graph(unittest.TestCase):
+class Graph_Generators_Star_Graph(GraphTest):
 
     # how to validate that 'kind' is either 'source', 'sink', or 'bidir' ??
 
     def test_star_graph_sink(self):
         g = star_graph_gen(4)
-        self.assertEqual(list(g.edges()), [(1, 0), (2, 0), (3, 0)])
+        self.assertIsomorphicEdges(g, [(1, 0), (2, 0), (3, 0)])
 
     def test_star_graph_source(self):
         g = star_graph_gen(4, kind="source")
-        self.assertEqual(list(g.edges()), [(0, 1), (0, 2), (0, 3)])
+        self.assertIsomorphicEdges(g, [(0, 1), (0, 2), (0, 3)])
 
     def test_star_graph_bidir(self):
         g = star_graph_gen(4, kind="bidir")
-        self.assertEqual(
-            list(g.edges()), [(0, 1), (0, 2), (0, 3), (1, 0), (2, 0), (3, 0)]
-        )
+        self.assertIsomorphicEdges(g, [(0, 1), (0, 2), (0, 3), (1, 0), (2, 0), (3, 0)])
 
     def test_star_graph_gen_bogus(self):
         with self.assertRaises(ValueError):
             star_graph_gen(4, kind="troll")
 
 
-class Graph_Generators_Tree_Graph(unittest.TestCase):
+class Graph_Generators_Tree_Graph(GraphTest):
     # note: our default is diff from networkx's default in this case (sink vs source)
     def test_tree_graph_source_sink(self):
         g = tree_graph_gen(2, 2)
-        self.assertEqual(
-            list(g.edges()), [(1, 0), (2, 0), (3, 1), (4, 1), (5, 2), (6, 2)]
-        )
+        self.assertIsomorphicEdges(g, [(1, 0), (2, 0), (3, 1), (4, 1), (5, 2), (6, 2)])
 
     def test_treeGraph_source(self):
         g = tree_graph_gen(2, 2, kind="source")
-        self.assertEqual(
-            list(g.edges()), [(0, 1), (0, 2), (1, 3), (1, 4), (2, 5), (2, 6)]
-        )
+        self.assertIsomorphicEdges(g, ((0, 1), (0, 2), (1, 3), (1, 4), (2, 5), (2, 6)))
 
     def test_tree_graph_bidir(self):
         g = tree_graph_gen(2, 2, kind="bidir")
-        self.assertEqual(
-            list(g.edges()),
+        self.assertIsomorphicEdges(
+            g,
             [
                 (0, 1),
                 (0, 2),
@@ -113,7 +187,7 @@ def test_tree_graph_gen_bogus(self):
             tree_graph_gen(2, 2, kind="troll")
 
 
-class Graph_Generators_Set_Types(unittest.TestCase):
+class Graph_Generators_Set_Types(GraphTest):
     def test_set_types_str(self):
         g = nx.path_graph(3, create_using=nx.MultiDiGraph)
 
@@ -160,23 +234,5 @@ def test_set_types_bogus_edge_type(self):
             set_types(g, node_types=node_dict, edge_types=edge_input)
 
 
-class Graph_Generators_Reverse_Dir(unittest.TestCase):
-    def test_reverse_dir(self):
-        g = nx.path_graph(4, create_using=nx.MultiDiGraph)
-
-        g = reverse_dir(g)
-
-        self.assertEqual(list(g.edges()), [(1, 0), (2, 1), (3, 2)])
-
-
-class Graph_Generators_Make_Bidir(unittest.TestCase):
-    def test_make_bidir(self):
-        g = nx.path_graph(3, create_using=nx.MultiDiGraph)
-
-        g = make_bidir(g)
-
-        self.assertEqual(list(g.edges()), [(0, 1), (1, 2), (1, 0), (2, 1)])
-
-
 if __name__ == "__main__":
     unittest.main()