From ea5398228050efb23b103cf896c7df6feab4232b Mon Sep 17 00:00:00 2001 From: Krishnan Mahadevan Date: Tue, 19 Sep 2023 09:37:38 +0530 Subject: [PATCH] Minor Code tweaks * Made some minor refactoring to graph implementation for better readability of code. * Leveraged streams wherever possible in graphs * Avoid code duplication --- .../org/testng/internal/BaseTestMethod.java | 20 +++++------ .../main/java/org/testng/internal/Graph.java | 32 ++++++----------- .../testng/internal/MethodGroupsHelper.java | 2 +- .../main/java/org/testng/internal/Tarjan.java | 35 ++++++++++--------- 4 files changed, 39 insertions(+), 50 deletions(-) diff --git a/testng-core/src/main/java/org/testng/internal/BaseTestMethod.java b/testng-core/src/main/java/org/testng/internal/BaseTestMethod.java index 567a0f8f0f..976f8b855c 100644 --- a/testng-core/src/main/java/org/testng/internal/BaseTestMethod.java +++ b/testng-core/src/main/java/org/testng/internal/BaseTestMethod.java @@ -190,25 +190,23 @@ public Set upstreamDependencies() { } public void setDownstreamDependencies(Set methods) { - if (!downstreamDependencies.isEmpty()) { - downstreamDependencies.clear(); - } - Set toAdd = methods; - if (RuntimeBehavior.isMemoryFriendlyMode()) { - toAdd = methods.stream().map(LiteWeightTestNGMethod::new).collect(Collectors.toSet()); - } - downstreamDependencies.addAll(toAdd); + setupDependencies(downstreamDependencies, methods); } public void setUpstreamDependencies(Set methods) { - if (!upstreamDependencies.isEmpty()) { - upstreamDependencies.clear(); + setupDependencies(upstreamDependencies, methods); + } + + private static void setupDependencies( + Set dependencies, Set methods) { + if (!dependencies.isEmpty()) { + dependencies.clear(); } Set toAdd = methods; if (RuntimeBehavior.isMemoryFriendlyMode()) { toAdd = methods.stream().map(LiteWeightTestNGMethod::new).collect(Collectors.toSet()); } - upstreamDependencies.addAll(toAdd); + dependencies.addAll(toAdd); } /** {@inheritDoc} */ diff --git a/testng-core/src/main/java/org/testng/internal/Graph.java b/testng-core/src/main/java/org/testng/internal/Graph.java index 59dca9aa8b..7be39f3dc7 100644 --- a/testng-core/src/main/java/org/testng/internal/Graph.java +++ b/testng-core/src/main/java/org/testng/internal/Graph.java @@ -7,6 +7,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.Function; import java.util.function.Supplier; import java.util.stream.Collectors; import org.testng.TestNGException; @@ -113,6 +114,7 @@ public void topologicalSort() { // Node node = findNodeWithNoPredecessors(nodes2); if (null == node) { + // We found a cycle. Time to use the Tarjan's algorithm to get the cycle. List cycle = new Tarjan<>(this, nodes2.get(0).getObject()).getCycle(); StringBuilder sb = new StringBuilder(); sb.append("The following methods have cyclic dependencies:\n"); @@ -132,16 +134,12 @@ public void topologicalSort() { private void initializeIndependentNodes() { if (null == m_independentNodes) { - List> list = Lists.newArrayList(m_nodes.values()); - // Ideally, we should not have to sort this. However, due to a bug where it treats all the - // methods as - // dependent nodes. - list.sort(comparator); - - m_independentNodes = Maps.newLinkedHashMap(); - for (Node node : list) { - m_independentNodes.put(node.getObject(), node); - } + m_independentNodes = + Lists.newArrayList(m_nodes.values()).stream() + .sorted(comparator) + .collect( + Collectors.toMap( + Node::getObject, Function.identity(), (a, b) -> a, Maps::newLinkedHashMap)); } } @@ -159,9 +157,7 @@ private void dumpSortedNodes() { */ private void removeFromNodes(List> nodes, Node node) { nodes.remove(node); - for (Node n : nodes) { - n.removePredecessor(node.getObject()); - } + nodes.parallelStream().forEach(it -> it.removePredecessor(node.getObject())); } private static void log(String s) { @@ -173,13 +169,7 @@ private static void log(Supplier s) { } private Node findNodeWithNoPredecessors(List> nodes) { - for (Node n : nodes) { - if (!n.hasPredecessors()) { - return n; - } - } - - return null; + return nodes.parallelStream().filter(it -> !it.hasPredecessors()).findFirst().orElse(null); } /** @@ -295,7 +285,7 @@ public void addPredecessor(T tm) { } public boolean hasPredecessors() { - return m_predecessors.size() > 0; + return !m_predecessors.isEmpty(); } } } diff --git a/testng-core/src/main/java/org/testng/internal/MethodGroupsHelper.java b/testng-core/src/main/java/org/testng/internal/MethodGroupsHelper.java index 81d0a68e2e..6a0db4ea3a 100644 --- a/testng-core/src/main/java/org/testng/internal/MethodGroupsHelper.java +++ b/testng-core/src/main/java/org/testng/internal/MethodGroupsHelper.java @@ -205,7 +205,7 @@ protected static void findGroupTransitiveClosure( // // Only keep going if new methods have been added // - keepGoing = newMethods.size() > 0; + keepGoing = !newMethods.isEmpty(); includedMethods = Lists.newArrayList(); includedMethods.addAll(newMethods.keySet()); newMethods = Maps.newHashMap(); diff --git a/testng-core/src/main/java/org/testng/internal/Tarjan.java b/testng-core/src/main/java/org/testng/internal/Tarjan.java index c5e029123d..2880746f50 100644 --- a/testng-core/src/main/java/org/testng/internal/Tarjan.java +++ b/testng-core/src/main/java/org/testng/internal/Tarjan.java @@ -14,39 +14,40 @@ */ public class Tarjan { int m_index = 0; - private final Stack m_s; - Map m_indices = Maps.newHashMap(); + private final Stack stack; + Map visitedNodes = Maps.newHashMap(); Map m_lowlinks = Maps.newHashMap(); private List m_cycle; public Tarjan(Graph graph, T start) { - m_s = new Stack<>(); + stack = new Stack<>(); run(graph, start); } - private void run(Graph graph, T v) { - m_indices.put(v, m_index); - m_lowlinks.put(v, m_index); + private void run(Graph graph, T start) { + visitedNodes.put(start, m_index); + m_lowlinks.put(start, m_index); m_index++; - m_s.push(v); + stack.push(start); - for (T vprime : graph.getPredecessors(v)) { - if (!m_indices.containsKey(vprime)) { - run(graph, vprime); - int min = Math.min(m_lowlinks.get(v), m_lowlinks.get(vprime)); - m_lowlinks.put(v, min); - } else if (m_s.contains(vprime)) { - m_lowlinks.put(v, Math.min(m_lowlinks.get(v), m_indices.get(vprime))); + for (T predecessor : graph.getPredecessors(start)) { + if (!visitedNodes.containsKey(predecessor)) { + run(graph, predecessor); + int min = Math.min(m_lowlinks.get(start), m_lowlinks.get(predecessor)); + m_lowlinks.put(start, min); + } else if (stack.contains(predecessor)) { + int min = Math.min(m_lowlinks.get(start), visitedNodes.get(predecessor)); + m_lowlinks.put(start, min); } } - if (Objects.equals(m_lowlinks.get(v), m_indices.get(v))) { + if (Objects.equals(m_lowlinks.get(start), visitedNodes.get(start))) { m_cycle = Lists.newArrayList(); T n; do { - n = m_s.pop(); + n = stack.pop(); m_cycle.add(n); - } while (!n.equals(v)); + } while (!n.equals(start)); } }