Overhaul how group edges are added during sorting

The new logic is conceptually simpler, with fewer special cases to
handle. Unlike the old implementation, the new approach avoids cycles.

It basically does depth-first searches through the group graph, adding
edges from each group's plugins to the plugins in the group's
successors.

This involved reversing the direction of edges in the group graph, as
this switches the logic from trying to find predecessors to trying to
successors: the new direction matches that of plugin graph edges, so
it's less confusing.

I can't think of a situation in which the iteration order of plugins
within a group matters. I tested it manually with my test load order of
~1600 plugins, and saw no difference when the order was randomly
shuffled. I've added a test case but it's a relatively simple scenario
and there may be a more complex scenario where it would matter that I
haven't thought of.

A buffer is used to hold the plugins in the previous groups in the
current path, because that's noticeably faster than just recording the
path and looking up the plugins for each group in the path. It does
duplicate the group vectors, but that's an insignificant amount of
memory used.

The new approach has a negative performance impact, with sorting now 15%
slower than before.

A more efficient solution could be to implement a custom DFS algorithm
that doesn't stop when it reaches a vertex it's already visited (which
would be fine since the graph has already been validated to be acyclic),
as then only the root vertices would need to be searched from.

docs/api/sorting.rst

@@ -90,18 +90,31 @@ plugins and the rest of the plugins in the graph.
 Group edges
 -----------
 
-For each plugin, the plugins that are members of groups that the current
-plugin's group loads after are iterated over and individually checked to see if
-adding an edge from the other group's plugin to the current plugin would cause a
-cycle. If not, the edge is queued for addition. If it would cause a cycle and
-one of the plugins is in the default group and the other group's plugin is
-master-flagged or the current plugin is not master flagged, then the plugin in
-the default group is recorded as one to skip adding edges to or from when the
-prospective edge involves any of the groups in the path from the other plugin
-to the current plugin.
+First a graph of groups is created to represent which groups must load after
+which other groups, with groups being added in lexicographical order and
+masterlist groups before userlist groups. Once all the groups have been added,
+a depth-first search is performed starting from each group in the order they
+were added.
 
-Once all the plugins have been iterated over, all the queued edges are added,
-skipping those edges identified in the earlier loop.
+At the start of each search, the starting group is used as the first element in
+a stack that will represent the current path through the graph. On each new edge
+encountered, the target group is appended to the stack and edges are
+added going from the plugins in the edge's source group to the plugins in the
+edge's target group, unless the edge to be added would cause a cycle, or unless
+the source group is the ``default`` group, in which case its plugins are
+ignored. The same is done for all the groups currently recorded in the stack.
+Once all a group's out-edges (going to groups that load directly after it) have
+been processed, the group is removed from the stack.
+
+Once all the groups have been iterated over, one final depth-first search is
+performed, this time starting from the ``default`` group and *not* skipping
+edges from its plugins.
+
+In this way all plugins have edges added from them to all the plugins in the
+groups that load after their group, unless the edge would cause a cycle. The
+order in which groups are defined can affect which edges are skipped, but the
+order of groups' "load after" metadata does not, and neither does the order in
+which plugins in each group are looped over.
 
 At this point the plugin graph is checked for cycles, and an error is thrown if
 any are encountered, so that metadata (or indeed plugin data) that cause them

src/api/sorting/group_sort.cpp

@@ -38,46 +38,6 @@ typedef boost::graph_traits<GroupGraph>::vertex_descriptor vertex_t;
 typedef boost::graph_traits<GroupGraph>::edge_descriptor edge_t;
 typedef boost::associative_property_map<std::map<edge_t, int>> edge_map_t;
 
-// This visitor is responsible for recording a vertex's successor vertices,
-// and whether they are reachable without user metadata or not. However, a
-// vertex is only recorded the first time it's discovered, vertices and
-// edges are iterated over in their insertion order, and masterlist metadata
-// is inserted first, so it depends on the structure and sources of the data
-// which path is encountered first.
-// E.g. for vertices A, B and C, if C -> B and C -> A are masterlist edges
-// added in that order and B -> A is a userlist edge then C -> B -> A will
-// be visited first and A will then have been finished so C -> A won't be
-// recorded, so it'll look the relationship between C and A involves user
-// metadata when it isn't necessary.
-class PredecessorGroupsVisitor : public boost::dfs_visitor<> {
-public:
-  PredecessorGroupsVisitor(std::vector<PredecessorGroup>& visitedGroups) :
-      visitedGroups_(visitedGroups) {}
-
-  void discover_vertex(vertex_t vertex, const GroupGraph& graph) {
-    pathStack_.push_back(vertex);
-
-    if (pathStack_.size() > 1) {
-      bool pathInvolvesUserMetadata = false;
-      for (size_t i = 0; i < pathStack_.size() - 1; i += 1) {
-        const auto edge = boost::edge(pathStack_[i], pathStack_[i + 1], graph);
-
-        pathInvolvesUserMetadata |=
-            graph[edge.first] == EdgeType::userLoadAfter;
-      }
-
-      visitedGroups_.push_back(
-          PredecessorGroup{graph[vertex], pathInvolvesUserMetadata});
-    }
-  }
-
-  void finish_vertex(vertex_t, const GroupGraph&) { pathStack_.pop_back(); }
-
-private:
-  std::vector<PredecessorGroup>& visitedGroups_;
-  std::vector<vertex_t> pathStack_;
-};
-
 class CycleDetector : public boost::dfs_visitor<> {
 public:
   void tree_edge(edge_t edge, const GroupGraph& graph) {
@@ -120,16 +80,6 @@ private:
   std::vector<Vertex> trail;
 };
 
-std::string joinVector(const std::vector<PredecessorGroup>& container) {
-  std::string output;
-  for (const auto& element : container) {
-    const auto via = element.pathInvolvesUserMetadata ? "user" : "masterlist";
-    output += element.name + " (via " + via + " metadata), ";
-  }
-
-  return output.substr(0, output.length() - 2);
-}
-
 std::vector<Group> SortByName(const std::vector<Group>& groups) {
   auto copy = groups;
   std::sort(copy.begin(), copy.end(), [](const auto& lhs, const auto& rhs) {
@@ -184,7 +134,7 @@ GroupGraph BuildGroupGraph(const std::vector<Group>& masterlistGroups,
           throw UndefinedGroupError(otherGroupName);
         }
 
-        boost::add_edge(vertex, otherVertex->second, edgeType, graph);
+        boost::add_edge(otherVertex->second, vertex, edgeType, graph);
       }
     }
   };
@@ -221,40 +171,6 @@ GroupGraph BuildGroupGraph(const std::vector<Group>& masterlistGroups,
   return graph;
 }
 
-std::unordered_map<std::string, std::vector<PredecessorGroup>>
-GetPredecessorGroups(const GroupGraph& graph) {
-  auto logger = getLogger();
-  if (logger) {
-    logger->trace("Sorting groups according to their load after data");
-  }
-
-  std::unordered_map<std::string, std::vector<PredecessorGroup>>
-      transitiveAfterGroups;
-  for (const vertex_t& vertex :
-       boost::make_iterator_range(boost::vertices(graph))) {
-    std::vector<PredecessorGroup> visitedGroups;
-    const PredecessorGroupsVisitor afterGroupsVisitor(visitedGroups);
-
-    // Create a color map.
-    std::vector<boost::default_color_type> colorVec(boost::num_vertices(graph));
-    const auto colorMap = boost::make_iterator_property_map(
-        colorVec.begin(),
-        boost::get(boost::vertex_index, graph),
-        colorVec.at(0));
-
-    boost::depth_first_visit(graph, vertex, afterGroupsVisitor, colorMap);
-    transitiveAfterGroups[graph[vertex]] = visitedGroups;
-
-    if (logger) {
-      logger->debug("Group \"{}\" transitively loads after groups \"{}\"",
-                    graph[vertex],
-                    joinVector(visitedGroups));
-    }
-  }
-
-  return transitiveAfterGroups;
-}
-
 vertex_t GetVertexByName(const GroupGraph& graph, const std::string& name) {
   for (const auto& vertex :
        boost::make_iterator_range(boost::vertices(graph))) {
@@ -263,7 +179,7 @@ vertex_t GetVertexByName(const GroupGraph& graph, const std::string& name) {
     }
   }
 
-  auto logger = getLogger();
+  const auto logger = getLogger();
   if (logger) {
     logger->error("Can't find group with name \"{}\"", name);
   }
@@ -293,7 +209,7 @@ std::vector<Vertex> GetGroupsPath(const GroupGraph& graph,
   std::vector<vertex_t> predecessors(boost::num_vertices(graph));
   std::vector<int> distance(predecessors.size(),
                             (std::numeric_limits<int>::max)());
-  distance.at(toVertex) = 0;
+  distance.at(fromVertex) = 0;
 
   bellman_ford_shortest_paths(
       graph,
@@ -301,13 +217,13 @@ std::vector<Vertex> GetGroupsPath(const GroupGraph& graph,
           .predecessor_map(boost::make_iterator_property_map(
               predecessors.begin(), get(boost::vertex_index, graph)))
           .distance_map(distance.data())
-          .root_vertex(toVertex));
+          .root_vertex(fromVertex));
 
-  std::vector<Vertex> path;
-  vertex_t currentVertex = fromVertex;
-  while (currentVertex != toVertex) {
-    auto nextVertex = predecessors.at(currentVertex);
-    if (nextVertex == currentVertex) {
+  std::vector<Vertex> path{Vertex(graph[toVertex])};
+  vertex_t currentVertex = toVertex;
+  while (currentVertex != fromVertex) {
+    const auto precedingVertex = predecessors.at(currentVertex);
+    if (precedingVertex == currentVertex) {
       if (logger) {
         logger->error(
             "Unreachable vertex {} encountered while looking for vertex {}",
@@ -317,18 +233,19 @@ std::vector<Vertex> GetGroupsPath(const GroupGraph& graph,
       return std::vector<Vertex>();
     }
 
-    const auto pair = boost::edge(nextVertex, currentVertex, graph);
+    const auto pair = boost::edge(precedingVertex, currentVertex, graph);
     if (!pair.second) {
       throw std::runtime_error("Unexpectedly couldn't find edge between \"" +
-                               graph[currentVertex] + "\" and \"" +
-                               graph[nextVertex] + "\"");
+                               graph[precedingVertex] + "\" and \"" +
+                               graph[currentVertex] + "\"");
     }
-    auto vertex = Vertex(graph[currentVertex], graph[pair.first]);
+    const auto vertex = Vertex(graph[precedingVertex], graph[pair.first]);
     path.push_back(vertex);
 
-    currentVertex = nextVertex;
+    currentVertex = precedingVertex;
   }
-  path.push_back(Vertex(graph[currentVertex]));
+
+  std::reverse(path.begin(), path.end());
 
   return path;
 }

src/api/sorting/group_sort.h

@@ -36,23 +36,14 @@
 namespace loot {
 typedef boost::adjacency_list<boost::vecS,
                               boost::vecS,
-                              boost::directedS,
+                              boost::bidirectionalS,
                               std::string,
                               EdgeType>
     GroupGraph;
 
-struct PredecessorGroup {
-  std::string name;
-  bool pathInvolvesUserMetadata{false};
-};
-
 GroupGraph BuildGroupGraph(const std::vector<Group>& masterlistGroups,
                            const std::vector<Group>& userGroups);
 
-// Map entries are a group name and names of transitive load after groups.
-std::unordered_map<std::string, std::vector<PredecessorGroup>>
-GetPredecessorGroups(const GroupGraph& groupGraph);
-
 std::vector<Vertex> GetGroupsPath(const GroupGraph& groupGraph,
                                   const std::string& fromGroupName,
                                   const std::string& toGroupName);

src/api/sorting/plugin_sort.cpp

@@ -29,6 +29,7 @@
 #include "api/helpers/logging.h"
 #include "api/sorting/group_sort.h"
 #include "api/sorting/plugin_graph.h"
+#include "loot/exception/undefined_group_error.h"
 
 namespace loot {
 std::vector<PluginSortingData> GetPluginsSortingData(
@@ -77,6 +78,23 @@ std::vector<PluginSortingData> GetPluginsSortingData(
   return pluginsSortingData;
 }
 
+void ValidatePluginGroups(const std::vector<PluginSortingData>& plugins,
+                          const GroupGraph& graph) {
+  std::unordered_set<std::string> groupNames;
+
+  for (const vertex_t& vertex :
+       boost::make_iterator_range(boost::vertices(graph))) {
+    groupNames.insert(graph[vertex]);
+  }
+
+  for (const auto& plugin : plugins) {
+    const auto pluginGroup = plugin.GetGroup();
+    if (groupNames.count(pluginGroup) == 0) {
+      throw UndefinedGroupError(pluginGroup);
+    }
+  }
+}
+
 bool IsInRange(const std::vector<PluginSortingData>::const_iterator& begin,
                const std::vector<PluginSortingData>::const_iterator& end,
                const std::string& name) {
@@ -263,14 +281,13 @@ std::vector<std::string> SortPlugins(
   graph.AddSpecificEdges();
   graph.AddHardcodedPluginEdges(hardcodedPlugins);
 
-  graph.AddGroupEdges(groupGraph);
-
   // Check for cycles now because from this point on edges are only added if
-  // they don't cause cycles, and adding tie-break edges is by far the slowest
-  // part of the process, so if there is a cycle checking now will provide
-  // quicker feedback than checking later.
+  // they don't cause cycles, and adding overlap and tie-break edges is
+  // relatively slow, so checking now provides quicker feedback if there is an
+  // issue.
   graph.CheckForCycles();
 
+  graph.AddGroupEdges(groupGraph);
   graph.AddOverlapEdges();
   graph.AddTieBreakEdges();
 
@@ -318,6 +335,7 @@ std::vector<std::string> SortPlugins(
             });
 
   const auto groupGraph = BuildGroupGraph(masterlistGroups, userGroups);
+  ValidatePluginGroups(pluginsSortingData, groupGraph);
 
   // Some parts of sorting are O(N^2) for N plugins, and master flags cause
   // O(M*N) edges to be added for M masters and N non-masters, which can be

src/tests/api/internals/sorting/group_sort_test.h

@@ -32,11 +32,6 @@ along with LOOT.  If not, see
 #include "loot/exception/undefined_group_error.h"
 
 namespace loot {
-bool operator==(const PredecessorGroup& lhs, const PredecessorGroup& rhs) {
-  return lhs.pathInvolvesUserMetadata == rhs.pathInvolvesUserMetadata &&
-         lhs.name == rhs.name;
-}
-
 namespace test {
 TEST(BuildGroupGraph, shouldThrowIfAnAfterGroupDoesNotExist) {
   std::vector<Group> groups({Group("b", {"a"})});
@@ -65,15 +60,15 @@ TEST(BuildGroupGraph, shouldThrowIfAfterGroupsAreCyclic) {
     ASSERT_EQ(3, e.GetCycle().size());
 
     EXPECT_EQ("a", e.GetCycle()[0].GetName());
-    EXPECT_EQ(EdgeType::userLoadAfter,
+    EXPECT_EQ(EdgeType::masterlistLoadAfter,
               e.GetCycle()[0].GetTypeOfEdgeToNextVertex());
 
-    EXPECT_EQ("c", e.GetCycle()[1].GetName());
+    EXPECT_EQ("b", e.GetCycle()[1].GetName());
     EXPECT_EQ(EdgeType::userLoadAfter,
               e.GetCycle()[1].GetTypeOfEdgeToNextVertex());
 
-    EXPECT_EQ("b", e.GetCycle()[2].GetName());
-    EXPECT_EQ(EdgeType::masterlistLoadAfter,
+    EXPECT_EQ("c", e.GetCycle()[2].GetName());
+    EXPECT_EQ(EdgeType::userLoadAfter,
               e.GetCycle()[2].GetTypeOfEdgeToNextVertex());
   }
 }
@@ -120,83 +115,6 @@ TEST(BuildGroupGraph,
   }
 }
 
-TEST(GetPredecessorGroups, shouldMapGroupsToTheirPredecessorGroups) {
-  std::vector<Group> groups({Group("a"), Group("b", {"a"}), Group("c", {"b"})});
-
-  const auto groupGraph = BuildGroupGraph(groups, {});
-  auto predecessors = GetPredecessorGroups(groupGraph);
-
-  EXPECT_TRUE(predecessors["a"].empty());
-  EXPECT_EQ(std::vector<PredecessorGroup>({{"a"}}), predecessors["b"]);
-  EXPECT_EQ(std::vector<PredecessorGroup>({{"b"}, {"a"}}), predecessors["c"]);
-}
-
-TEST(GetPredecessorGroups,
-     shouldRecordIfADirectSuccessorIsDefinedInUserMetadata) {
-  std::vector<Group> masterlistGroups({Group("a")});
-  std::vector<Group> userlistGroups({Group("b", {"a"})});
-
-  const auto groupGraph = BuildGroupGraph(masterlistGroups, userlistGroups);
-  auto predecessors = GetPredecessorGroups(groupGraph);
-
-  EXPECT_EQ(std::vector<PredecessorGroup>({{"a", true}}), predecessors["b"]);
-}
-
-TEST(GetPredecessorGroups,
-     shouldRecordIfADirectPredecessorIsLinkedDueToUserMetadata) {
-  std::vector<Group> masterlistGroups({Group("a"), Group("b")});
-  std::vector<Group> userlistGroups({Group("b", {"a"})});
-
-  const auto groupGraph = BuildGroupGraph(masterlistGroups, userlistGroups);
-  auto predecessors = GetPredecessorGroups(groupGraph);
-
-  EXPECT_EQ(std::vector<PredecessorGroup>({{"a", true}}), predecessors["b"]);
-}
-
-TEST(GetPredecessorGroups,
-     shouldRecordIfAnIndirectSuccessorIsDefinedInUserMetadata) {
-  std::vector<Group> masterlistGroups({Group("a"), Group("b", {"a"})});
-  std::vector<Group> userlistGroups({Group("c", {"b"})});
-
-  const auto groupGraph = BuildGroupGraph(masterlistGroups, userlistGroups);
-  auto predecessors = GetPredecessorGroups(groupGraph);
-
-  EXPECT_EQ(std::vector<PredecessorGroup>({{"a"}}), predecessors["b"]);
-  EXPECT_EQ(std::vector<PredecessorGroup>({{"b", true}, {"a", true}}),
-            predecessors["c"]);
-}
-
-TEST(GetPredecessorGroups,
-     shouldRecordIfAnIndirectPredecessorIsLinkedDueToUserMetadata) {
-  std::vector<Group> masterlistGroups(
-      {Group("a"), Group("b"), Group("c", {"b"})});
-  std::vector<Group> userlistGroups({Group("b", {"a"})});
-
-  const auto groupGraph = BuildGroupGraph(masterlistGroups, userlistGroups);
-  auto predecessors = GetPredecessorGroups(groupGraph);
-
-  EXPECT_EQ(std::vector<PredecessorGroup>({{"a", true}}), predecessors["b"]);
-  EXPECT_EQ(std::vector<PredecessorGroup>({{"b"}, {"a", true}}),
-            predecessors["c"]);
-}
-
-TEST(GetPredecessorGroups,
-     shouldNotLeakUserMetadataInvolvementToSeparatePaths) {
-  // This arrangement of groups ensures that a masterlist-sourced edge is
-  // followed after a userlist-sourced edge along a different path, to check
-  // encountering a userlist-sourced edge along one path does not poison
-  // discovery of other paths.
-  std::vector<Group> masterlistGroups(
-      {Group("a"), Group("b"), Group("c"), Group("d", {"b", "c"})});
-  std::vector<Group> userlistGroups({Group("b", {"a"})});
-
-  const auto groupGraph = BuildGroupGraph(masterlistGroups, userlistGroups);
-  auto predecessors = GetPredecessorGroups(groupGraph);
-
-  EXPECT_EQ(std::vector<PredecessorGroup>({{"b"}, {"a", true}, {"c"}}),
-            predecessors["d"]);
-}
-
 TEST(GetGroupsPath, shouldThrowIfTheFromGroupDoesNotExist) {
   std::vector<Group> groups({Group("a"), Group("b", {"a"})});
   std::vector<Group> userGroups({Group("a", {"c"}), Group("c")});

src/tests/api/internals/sorting/plugin_graph_test.h

@@ -581,7 +581,7 @@ TEST_F(PluginGraphTest, addGroupEdgesShouldSkipAnEdgeThatWouldCauseACycle) {
 
 TEST_F(
     PluginGraphTest,
-    addGroupEdgesDoesNotSkipAnEdgeThatCausesACycleInvolvingOtherNonDefaultGroups) {
+    addGroupEdgesShouldSkipAnEdgeThatWouldCauseACycleInvolvingOtherNonDefaultGroups) {
   PluginGraph graph;
 
   const auto a = graph.AddVertex(CreatePluginSortingData("A.esp", "A"));
@@ -596,20 +596,7 @@ TEST_F(
   EXPECT_TRUE(graph.EdgeExists(c, a));
   EXPECT_TRUE(graph.EdgeExists(a, b));
 
-  // FIXME: This should not cause a cycle.
-  try {
-    graph.CheckForCycles();
-    FAIL();
-  } catch (CyclicInteractionError& e) {
-    ASSERT_EQ(3, e.GetCycle().size());
-    EXPECT_EQ(graph.GetPlugin(a).GetName(), e.GetCycle()[0].GetName());
-    EXPECT_EQ(EdgeType::masterlistGroup,
-              e.GetCycle()[0].GetTypeOfEdgeToNextVertex());
-    EXPECT_EQ(graph.GetPlugin(b).GetName(), e.GetCycle()[1].GetName());
-    EXPECT_EQ(EdgeType::userGroup, e.GetCycle()[1].GetTypeOfEdgeToNextVertex());
-    EXPECT_EQ(graph.GetPlugin(c).GetName(), e.GetCycle()[2].GetName());
-    EXPECT_EQ(EdgeType::master, e.GetCycle()[2].GetTypeOfEdgeToNextVertex());
-  }
+  EXPECT_NO_THROW(graph.CheckForCycles());
 }
 
 TEST_F(
@@ -662,10 +649,7 @@ TEST_F(
   EXPECT_TRUE(graph.EdgeExists(c, d3));
 
   EXPECT_TRUE(graph.EdgeExists(b, d3));
-
-  // FIXME: This edge should be added but isn't, it's a limitation of the
-  // current implementation.
-  EXPECT_FALSE(graph.EdgeExists(c, d1));
+  EXPECT_TRUE(graph.EdgeExists(c, d1));
 
   EXPECT_NO_THROW(graph.CheckForCycles());
 }
@@ -729,11 +713,9 @@ TEST_F(
   EXPECT_TRUE(graph.EdgeExists(b1, c2));
   EXPECT_TRUE(graph.EdgeExists(b2, c2));
   EXPECT_TRUE(graph.EdgeExists(a2, c1));
+  EXPECT_FALSE(graph.EdgeExists(b2, c1));
 
-  // FIXME: This edge is unwanted and causes a cycle.
-  EXPECT_TRUE(graph.EdgeExists(b2, c1));
-
-  EXPECT_THROW(graph.CheckForCycles(), CyclicInteractionError);
+  EXPECT_NO_THROW(graph.CheckForCycles());
 }
 
 TEST_F(
@@ -800,13 +782,11 @@ TEST_F(
   EXPECT_TRUE(graph.EdgeExists(a2, c1));
   EXPECT_TRUE(graph.EdgeExists(a2, d1));
   EXPECT_TRUE(graph.EdgeExists(a2, d2));
+  EXPECT_FALSE(graph.EdgeExists(b2, c1));
   EXPECT_FALSE(graph.EdgeExists(d1, d2));
   EXPECT_FALSE(graph.EdgeExists(d2, d1));
 
-  // FIXME: This edge is unwanted and causes a cycle.
-  EXPECT_TRUE(graph.EdgeExists(b2, c1));
-
-  EXPECT_THROW(graph.CheckForCycles(), CyclicInteractionError);
+  EXPECT_NO_THROW(graph.CheckForCycles());
 }
 
 TEST_F(
@@ -825,6 +805,7 @@ TEST_F(
   // Should be D.esp -> B.esp -> C.esp
   EXPECT_TRUE(graph.EdgeExists(b, c));
   EXPECT_TRUE(graph.EdgeExists(d, b));
+  EXPECT_FALSE(graph.EdgeExists(c, d));
 
   EXPECT_NO_THROW(graph.CheckForCycles());
 }
@@ -915,11 +896,9 @@ TEST_F(
   EXPECT_TRUE(graph.EdgeExists(d, b));
   EXPECT_TRUE(graph.EdgeExists(b, c));
   EXPECT_TRUE(graph.EdgeExists(c, e));
+  EXPECT_FALSE(graph.EdgeExists(e, f));
 
-  // FIXME: This edge is unwanted and causes a cycle.
-  EXPECT_TRUE(graph.EdgeExists(e, f));
-
-  EXPECT_THROW(graph.CheckForCycles(), CyclicInteractionError);
+  EXPECT_NO_THROW(graph.CheckForCycles());
 }
 
 TEST_F(
@@ -1362,11 +1341,9 @@ TEST_F(
     EXPECT_TRUE(graph.EdgeExists(b, d));
     EXPECT_FALSE(graph.EdgeExists(a, b));
     EXPECT_FALSE(graph.EdgeExists(b, a));
+    EXPECT_FALSE(graph.EdgeExists(c, d));
 
-    // FIXME: This edge is unwanted and causes a cycle.
-    EXPECT_TRUE(graph.EdgeExists(c, d));
-
-    EXPECT_THROW(graph.CheckForCycles(), CyclicInteractionError);
+    EXPECT_NO_THROW(graph.CheckForCycles());
   }
 }
 
@@ -1414,13 +1391,11 @@ TEST_F(PluginGraphTest,
 
     EXPECT_FALSE(graph.EdgeExists(b, c));
     EXPECT_FALSE(graph.EdgeExists(c, b));
+    EXPECT_FALSE(graph.EdgeExists(c, d));
     EXPECT_FALSE(graph.EdgeExists(c, e));
     EXPECT_FALSE(graph.EdgeExists(d, c));
 
-    // FIXME: This edge is unwanted and causes a cycle.
-    EXPECT_TRUE(graph.EdgeExists(c, d));
-
-    EXPECT_THROW(graph.CheckForCycles(), CyclicInteractionError);
+    EXPECT_NO_THROW(graph.CheckForCycles());
   }
 }
 
@@ -1477,11 +1452,9 @@ TEST_F(PluginGraphTest, addGroupEdgesShouldNotDependOnPluginGraphVertexOrder) {
     ASSERT_TRUE(graph.EdgeExists(a2, c));
     ASSERT_FALSE(graph.EdgeExists(a1, a2));
     ASSERT_FALSE(graph.EdgeExists(a2, a1));
+    ASSERT_FALSE(graph.EdgeExists(b, c));
 
-    // FIXME: This edge is unwanted and causes a cycle.
-    ASSERT_TRUE(graph.EdgeExists(b, c));
-
-    EXPECT_THROW(graph.CheckForCycles(), CyclicInteractionError);
+    ASSERT_NO_THROW(graph.CheckForCycles());
   }
 }