From defe94f2e3aac293a2c217f1fffc02b5b797e0d2 Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Fri, 17 Jan 2025 21:30:46 +0000 Subject: [PATCH] Check for cycles as part of building the group graph --- src/api/sorting/group_sort.cpp | 11 +- .../api/internals/sorting/group_sort_test.h | 227 +++++++++--------- 2 files changed, 124 insertions(+), 114 deletions(-) diff --git a/src/api/sorting/group_sort.cpp b/src/api/sorting/group_sort.cpp index d40bc789..57e17565 100644 --- a/src/api/sorting/group_sort.cpp +++ b/src/api/sorting/group_sort.cpp @@ -179,6 +179,11 @@ GroupGraph BuildGroupGraph(const std::vector& masterlistGroups, } addGroups(userGroups, EdgeType::userLoadAfter); + if (logger) { + logger->trace("Checking for cycles in the group graph"); + } + boost::depth_first_search(graph, boost::visitor(CycleDetector())); + return graph; } @@ -189,12 +194,6 @@ GetPredecessorGroups(const GroupGraph& graph) { logger->trace("Sorting groups according to their load after data"); } - // Check for cycles. - if (logger) { - logger->trace("Checking for cycles in the group graph"); - } - boost::depth_first_search(graph, boost::visitor(CycleDetector())); - std::unordered_map> transitiveAfterGroups; for (const vertex_t& vertex : diff --git a/src/tests/api/internals/sorting/group_sort_test.h b/src/tests/api/internals/sorting/group_sort_test.h index 70ad3aff..7923a68b 100644 --- a/src/tests/api/internals/sorting/group_sort_test.h +++ b/src/tests/api/internals/sorting/group_sort_test.h @@ -51,8 +51,121 @@ TEST(BuildGroupGraph, shouldThrowIfMasterlistGroupLoadsAfterAUserlistGroup) { Group("e", {"b", "d"})}); std::vector userGroups({Group("d", {"c"})}); - EXPECT_THROW(BuildGroupGraph(groups, userGroups), - UndefinedGroupError); + EXPECT_THROW(BuildGroupGraph(groups, userGroups), UndefinedGroupError); +} + +TEST(BuildGroupGraph, shouldThrowIfAfterGroupsAreCyclic) { + std::vector groups({Group("a"), Group("b", {"a"})}); + std::vector userGroups({Group("a", {"c"}), Group("c", {"b"})}); + + try { + const auto groupGraph = BuildGroupGraph(groups, userGroups); + FAIL(); + } catch (CyclicInteractionError& e) { + ASSERT_EQ(3, e.GetCycle().size()); + + // Vertices can be added in any order, so which group is first is + // undefined. + if (e.GetCycle()[0].GetName() == "a") { + EXPECT_EQ(EdgeType::userLoadAfter, + e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); + + EXPECT_EQ("c", e.GetCycle()[1].GetName()); + EXPECT_EQ(EdgeType::userLoadAfter, + e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); + + EXPECT_EQ("b", e.GetCycle()[2].GetName()); + EXPECT_EQ(EdgeType::masterlistLoadAfter, + e.GetCycle()[2].GetTypeOfEdgeToNextVertex()); + } else if (e.GetCycle()[0].GetName() == "b") { + EXPECT_EQ(EdgeType::masterlistLoadAfter, + e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); + + EXPECT_EQ("a", e.GetCycle()[1].GetName()); + EXPECT_EQ(EdgeType::userLoadAfter, + e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); + + EXPECT_EQ("c", e.GetCycle()[2].GetName()); + EXPECT_EQ(EdgeType::userLoadAfter, + e.GetCycle()[2].GetTypeOfEdgeToNextVertex()); + } else { + EXPECT_EQ("c", e.GetCycle()[0].GetName()); + EXPECT_EQ(EdgeType::userLoadAfter, + e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); + + EXPECT_EQ("b", e.GetCycle()[1].GetName()); + EXPECT_EQ(EdgeType::masterlistLoadAfter, + e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); + + EXPECT_EQ("a", e.GetCycle()[2].GetName()); + EXPECT_EQ(EdgeType::userLoadAfter, + e.GetCycle()[2].GetTypeOfEdgeToNextVertex()); + } + } +} + +TEST(BuildGroupGraph, shouldNotThrowIfThereIsNoCycle) { + std::vector groups({Group("a"), Group("b", {"a"})}); + + EXPECT_NO_THROW(BuildGroupGraph(groups, {})); +} + +TEST(BuildGroupGraph, shouldThrowIfThereIsACycle) { + std::vector groups({Group("a", {"b"}), Group("b", {"a"})}); + + try { + BuildGroupGraph(groups, {}); + FAIL(); + } catch (const CyclicInteractionError& e) { + ASSERT_EQ(2, e.GetCycle().size()); + EXPECT_EQ("a", e.GetCycle()[0].GetName()); + EXPECT_EQ(EdgeType::masterlistLoadAfter, + e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); + EXPECT_EQ("b", e.GetCycle()[1].GetName()); + EXPECT_EQ(EdgeType::masterlistLoadAfter, + e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); + } +} + +TEST(BuildGroupGraph, + exceptionThrownShouldOnlyRecordGroupsThatArePartOfTheCycle) { + std::vector groups( + {Group("a", {"b"}), Group("b", {"a"}), Group("c", {"b"})}); + + try { + BuildGroupGraph(groups, {}); + FAIL(); + } catch (const CyclicInteractionError& e) { + ASSERT_EQ(2, e.GetCycle().size()); + EXPECT_EQ("a", e.GetCycle()[0].GetName()); + EXPECT_EQ(EdgeType::masterlistLoadAfter, + e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); + EXPECT_EQ("b", e.GetCycle()[1].GetName()); + EXPECT_EQ(EdgeType::masterlistLoadAfter, + e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); + } +} + +TEST(GetPredecessorGroups, shouldMapGroupsToTheirPredecessorGroups) { + std::vector 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({{"a"}}), predecessors["b"]); + EXPECT_EQ(std::vector({{"b"}, {"a"}}), predecessors["c"]); +} + +TEST(GetPredecessorGroups, + shouldRecordIfADirectSuccessorIsDefinedInUserMetadata) { + std::vector masterlistGroups({Group("a")}); + std::vector userlistGroups({Group("b", {"a"})}); + + const auto groupGraph = BuildGroupGraph(masterlistGroups, userlistGroups); + auto predecessors = GetPredecessorGroups(groupGraph); + + EXPECT_EQ(std::vector({{"a", true}}), predecessors["b"]); } TEST(GetPredecessorGroups, @@ -110,124 +223,22 @@ TEST(GetPredecessorGroups, predecessors["d"]); } -TEST(GetPredecessorGroups, shouldThrowIfAfterGroupsAreCyclic) { - std::vector groups({Group("a"), Group("b", {"a"})}); - std::vector userGroups({Group("a", {"c"}), Group("c", {"b"})}); - - const auto groupGraph = BuildGroupGraph(groups, userGroups); - - try { - GetPredecessorGroups(groupGraph); - FAIL(); - } catch (CyclicInteractionError& e) { - ASSERT_EQ(3, e.GetCycle().size()); - - // Vertices can be added in any order, so which group is first is - // undefined. - if (e.GetCycle()[0].GetName() == "a") { - EXPECT_EQ(EdgeType::userLoadAfter, - e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); - - EXPECT_EQ("c", e.GetCycle()[1].GetName()); - EXPECT_EQ(EdgeType::userLoadAfter, - e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); - - EXPECT_EQ("b", e.GetCycle()[2].GetName()); - EXPECT_EQ(EdgeType::masterlistLoadAfter, - e.GetCycle()[2].GetTypeOfEdgeToNextVertex()); - } else if (e.GetCycle()[0].GetName() == "b") { - EXPECT_EQ(EdgeType::masterlistLoadAfter, - e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); - - EXPECT_EQ("a", e.GetCycle()[1].GetName()); - EXPECT_EQ(EdgeType::userLoadAfter, - e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); - - EXPECT_EQ("c", e.GetCycle()[2].GetName()); - EXPECT_EQ(EdgeType::userLoadAfter, - e.GetCycle()[2].GetTypeOfEdgeToNextVertex()); - } else { - EXPECT_EQ("c", e.GetCycle()[0].GetName()); - EXPECT_EQ(EdgeType::userLoadAfter, - e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); - - EXPECT_EQ("b", e.GetCycle()[1].GetName()); - EXPECT_EQ(EdgeType::masterlistLoadAfter, - e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); - - EXPECT_EQ("a", e.GetCycle()[2].GetName()); - EXPECT_EQ(EdgeType::userLoadAfter, - e.GetCycle()[2].GetTypeOfEdgeToNextVertex()); - } - } -} - -TEST(GetPredecessorGroups, shouldNotThrowIfThereIsNoCycle) { - std::vector groups({Group("a"), Group("b", {"a"})}); - - const auto groupGraph = BuildGroupGraph(groups, {}); - - EXPECT_NO_THROW(GetPredecessorGroups(groupGraph)); -} - -TEST(GetPredecessorGroups, shouldThrowIfThereIsACycle) { - std::vector groups({Group("a", {"b"}), Group("b", {"a"})}); - - const auto groupGraph = BuildGroupGraph(groups, {}); - - try { - GetPredecessorGroups(groupGraph); - FAIL(); - } catch (const CyclicInteractionError& e) { - ASSERT_EQ(2, e.GetCycle().size()); - EXPECT_EQ("a", e.GetCycle()[0].GetName()); - EXPECT_EQ(EdgeType::masterlistLoadAfter, - e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); - EXPECT_EQ("b", e.GetCycle()[1].GetName()); - EXPECT_EQ(EdgeType::masterlistLoadAfter, - e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); - } -} - -TEST(GetPredecessorGroups, - exceptionThrownShouldOnlyRecordGroupsThatArePartOfTheCycle) { - std::vector groups( - {Group("a", {"b"}), Group("b", {"a"}), Group("c", {"b"})}); - - const auto groupGraph = BuildGroupGraph(groups, {}); - - try { - GetPredecessorGroups(groupGraph); - FAIL(); - } catch (const CyclicInteractionError& e) { - ASSERT_EQ(2, e.GetCycle().size()); - EXPECT_EQ("a", e.GetCycle()[0].GetName()); - EXPECT_EQ(EdgeType::masterlistLoadAfter, - e.GetCycle()[0].GetTypeOfEdgeToNextVertex()); - EXPECT_EQ("b", e.GetCycle()[1].GetName()); - EXPECT_EQ(EdgeType::masterlistLoadAfter, - e.GetCycle()[1].GetTypeOfEdgeToNextVertex()); - } -} - TEST(GetGroupsPath, shouldThrowIfTheFromGroupDoesNotExist) { std::vector groups({Group("a"), Group("b", {"a"})}); - std::vector userGroups({Group("a", {"c"}), Group("c", {"b"})}); + std::vector userGroups({Group("a", {"c"}), Group("c")}); const auto groupGraph = BuildGroupGraph(groups, userGroups); - EXPECT_THROW(GetGroupsPath(groupGraph, "d", "a"), - std::invalid_argument); + EXPECT_THROW(GetGroupsPath(groupGraph, "d", "a"), std::invalid_argument); } TEST(GetGroupsPath, shouldThrowIfTheToGroupDoesNotExist) { std::vector groups({Group("a"), Group("b", {"a"})}); - std::vector userGroups({Group("a", {"c"}), Group("c", {"b"})}); + std::vector userGroups({Group("a", {"c"}), Group("c")}); const auto groupGraph = BuildGroupGraph(groups, userGroups); - EXPECT_THROW(GetGroupsPath(groupGraph, "a", "d"), - std::invalid_argument); + EXPECT_THROW(GetGroupsPath(groupGraph, "a", "d"), std::invalid_argument); } TEST(GetGroupsPath,