Fix #1480: Priority/parallel=methods issue.

CHANGES.txt

@@ -1,4 +1,5 @@
 Current
+Fixed: GITHUB-1480: Parallel=methods not working when tests have different priorities set (Micah Lapping-Carr)
 Fixed: GITHUB-1041: Factory data-provider parameters not displayed in test-result (Krishnan Mahadevan)
 Fixed: GITHUB-1901: The overall reported Test time for suite containing parallel tests should be max(tests_times) (Krishnan Mahadevan)
 Fixed: GITHUB-1893: Streamline invocation of "init" method within TestResult to be private (Krishnan Mahadevan)

src/main/java/org/testng/ITestNGMethod.java

@@ -191,6 +191,10 @@ public interface ITestNGMethod extends Cloneable {
 
   void setPriority(int priority);
 
+  int getInterceptedPriority();
+
+  void setInterceptedPriority(int priority);
+
   /** @return the XmlTest this method belongs to. */
   XmlTest getXmlTest();
 

src/main/java/org/testng/TestNG.java

@@ -1092,7 +1092,8 @@ public List<ISuite> runSuitesLocally() {
             m_suiteThreadPoolSize,
             Integer.MAX_VALUE,
             TimeUnit.MILLISECONDS,
-            new LinkedBlockingQueue<>());
+            new LinkedBlockingQueue<>(),
+            null);
 
     Utils.log("TestNG", 2, "Starting executor for all suites");
     // Run all suites in parallel

src/main/java/org/testng/TestRunner.java

@@ -12,6 +12,8 @@
 import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.concurrent.LinkedBlockingQueue;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.PriorityBlockingQueue;
+import java.util.concurrent.BlockingQueue;
 
 import org.testng.collections.ListMultiMap;
 import org.testng.collections.Lists;
@@ -32,6 +34,7 @@
 import org.testng.internal.ITestResultNotifier;
 import org.testng.internal.InvokedMethod;
 import org.testng.internal.Invoker;
+import org.testng.internal.TestMethodComparator;
 import org.testng.internal.MethodGroupsHelper;
 import org.testng.internal.MethodHelper;
 import org.testng.internal.ResultMap;
@@ -693,11 +696,20 @@ private void privateRun(XmlTest xmlTest) {
       // Make sure we create a graph based on the intercepted methods, otherwise an interceptor
       // removing methods would cause the graph never to terminate (because it would expect
       // termination from methods that never get invoked).
+      ITestNGMethod[] interceptedOrder = intercept(m_allTestMethods);
       DynamicGraph<ITestNGMethod> graph =
-          DynamicGraphHelper.createDynamicGraph(intercept(m_allTestMethods), getCurrentXmlTest());
+          DynamicGraphHelper.createDynamicGraph(interceptedOrder, getCurrentXmlTest());
+      // In some cases, additional sorting is needed to make sure tests run in the appropriate order.
+      // If the user specified a method interceptor, or if we have any methods that have a non-default
+      // priority on them, we need to sort.
+      boolean needPrioritySort = m_methodInterceptors.size() > 1 || 
+          Arrays.stream(interceptedOrder).anyMatch(m -> m.getPriority() != 0);
+      Comparator<ITestNGMethod> methodComparator = needPrioritySort ? new TestMethodComparator() : null;
       graph.setVisualisers(this.visualisers);
       if (parallel) {
         if (graph.getNodeCount() > 0) {
+          // If any of the test methods specify a priority other than the default, we'll need to be able to sort them.
+          BlockingQueue<Runnable> queue = needPrioritySort ? new PriorityBlockingQueue<Runnable>() : new LinkedBlockingQueue<Runnable>();
           GraphThreadPoolExecutor<ITestNGMethod> executor =
               new GraphThreadPoolExecutor<>(
                   "test=" + xmlTest.getName(),
@@ -707,7 +719,8 @@ private void privateRun(XmlTest xmlTest) {
                   threadCount,
                   0,
                   TimeUnit.MILLISECONDS,
-                  new LinkedBlockingQueue<>());
+                  queue,
+                  methodComparator);
           executor.run();
           try {
             long timeOut = m_xmlTest.getTimeOut(XmlTest.DEFAULT_TIMEOUT_MS);
@@ -734,6 +747,12 @@ private void privateRun(XmlTest xmlTest) {
         }
 
         while (!freeNodes.isEmpty()) {
+          if (needPrioritySort) {
+            Collections.sort(freeNodes, methodComparator);
+            // Since this is sequential, let's run one at a time and fetch/sort freeNodes after each method.
+            // Future task: To optimize this, we can only update freeNodes after running a test that another test is dependent upon.
+            freeNodes = freeNodes.subList(0, 1);
+          }
           List<IWorker<ITestNGMethod>> runnables = createWorkers(freeNodes);
           for (IWorker<ITestNGMethod> r : runnables) {
             r.run();
@@ -776,6 +795,16 @@ private ITestNGMethod[] intercept(ITestNGMethod[] methods) {
               m_groupMethods.getBeforeGroupsMethods(),
               m_groupMethods.getAfterGroupsMethods());
     }
+
+    // If the user specified a method interceptor, whatever that returns is the order we're going
+    // to run things in. Set the intercepted priority for that case.
+    // There's a built-in interceptor, so look for more than one.
+    if (m_methodInterceptors.size() > 1) {
+      for (int i = 0; i < resultArray.length; ++i) {
+        resultArray[i].setInterceptedPriority(i);
+      }
+    }
+
     return resultArray;
   }
 

src/main/java/org/testng/internal/BaseTestMethod.java

@@ -67,6 +67,7 @@ public abstract class BaseTestMethod implements ITestNGMethod {
 
   private boolean m_ignoreMissingDependencies;
   private int m_priority;
+  private int m_interceptedPriority;
 
   private XmlTest m_xmlTest;
   private Object m_instance;
@@ -687,6 +688,16 @@ public void setPriority(int priority) {
     m_priority = priority;
   }
 
+  @Override
+  public int getInterceptedPriority() {
+    return m_interceptedPriority;
+  }
+
+  @Override
+  public void setInterceptedPriority(int priority) {
+    m_interceptedPriority = priority;
+  }
+
   @Override
   public XmlTest getXmlTest() {
     return m_xmlTest;

src/main/java/org/testng/internal/ClonedMethod.java

@@ -333,6 +333,16 @@ public void setPriority(int priority) {
     // ignored
   }
 
+  @Override
+  public int getInterceptedPriority() {
+    return m_method.getInterceptedPriority();
+  }
+
+  @Override
+  public void setInterceptedPriority(int priority) {
+    // ignored
+  }
+
   @Override
   public XmlTest getXmlTest() {
     return m_method.getXmlTest();

src/main/java/org/testng/internal/DynamicGraphHelper.java

@@ -27,23 +27,6 @@ public static DynamicGraph<ITestNGMethod> createDynamicGraph(
       ITestNGMethod[] methods, XmlTest xmlTest) {
     DynamicGraph<ITestNGMethod> result = new DynamicGraph<>();
 
-    ListMultiMap<Integer, ITestNGMethod> methodsByPriority = Maps.newListMultiMap();
-    for (ITestNGMethod method : methods) {
-      methodsByPriority.put(method.getPriority(), method);
-    }
-    List<Integer> availablePriorities = Lists.newArrayList(methodsByPriority.keySet());
-    Collections.sort(availablePriorities);
-    Integer previousPriority = methods.length > 0 ? availablePriorities.get(0) : 0;
-    for (int i = 1; i < availablePriorities.size(); i++) {
-      Integer currentPriority = availablePriorities.get(i);
-      for (ITestNGMethod p0Method : methodsByPriority.get(previousPriority)) {
-        for (ITestNGMethod p1Method : methodsByPriority.get(currentPriority)) {
-          result.addEdge(TestRunner.PriorityWeight.priority.ordinal(), p1Method, p0Method);
-        }
-      }
-      previousPriority = currentPriority;
-    }
-
     DependencyMap dependencyMap = new DependencyMap(methods);
 
     // Keep track of whether we have group dependencies. If we do, preserve-order needs

src/main/java/org/testng/internal/TestMethodComparator.java

@@ -0,0 +1,27 @@
+package org.testng.internal;
+
+import java.util.Comparator;
+
+import org.testng.ITestNGMethod;
+
+public class TestMethodComparator implements Comparator<ITestNGMethod> {
+
+  @Override
+  public int compare(ITestNGMethod o1, ITestNGMethod o2) {
+    return compareStatic(o1, o2);
+  }
+
+  public static int compareStatic(ITestNGMethod o1, ITestNGMethod o2) {
+    int prePriDiff = o1.getInterceptedPriority() - o2.getInterceptedPriority();
+    if (prePriDiff != 0) {
+      return prePriDiff;
+    }
+
+    int priDiff = o1.getPriority() - o2.getPriority();
+    if (priDiff != 0) {
+      return priDiff;
+    }
+
+    return o1.getMethodName().compareTo(o2.getMethodName());
+  }
+}

src/main/java/org/testng/internal/TestMethodWorker.java

@@ -245,7 +245,14 @@ public List<ITestNGMethod> getTasks() {
 
   @Override
   public int compareTo(@Nonnull IWorker<ITestNGMethod> other) {
-    return getPriority() - other.getPriority();
+    if (m_methodInstances.isEmpty()) {
+        return 0;
+    }
+    List<ITestNGMethod> otherTasks = other.getTasks();
+    if (otherTasks.isEmpty()) {
+        return 0;
+    }
+    return TestMethodComparator.compareStatic(m_methodInstances.get(0).getMethod(), otherTasks.get(0));
   }
 
   /** The priority of a worker is the priority of the first method it's going to run. */

src/main/java/org/testng/internal/WrappedTestNGMethod.java

@@ -329,6 +329,16 @@ public void setPriority(int priority) {
     testNGMethod.setPriority(priority);
   }
 
+  @Override
+  public int getInterceptedPriority() {
+    return testNGMethod.getInterceptedPriority();
+  }
+
+  @Override
+  public void setInterceptedPriority(int priority) {
+    testNGMethod.setInterceptedPriority(priority);
+  }
+
   @Override
   public XmlTest getXmlTest() {
     return testNGMethod.getXmlTest();

src/main/java/org/testng/internal/thread/graph/GraphThreadPoolExecutor.java

@@ -9,6 +9,9 @@
 import org.testng.log4testng.Logger;
 
 import javax.annotation.Nonnull;
+
+import java.util.Collections;
+import java.util.Comparator;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.BlockingQueue;
@@ -26,6 +29,7 @@ public class GraphThreadPoolExecutor<T> extends ThreadPoolExecutor {
   private final IThreadWorkerFactory<T> m_factory;
   private final Map<T, IWorker<T>> mapping = Maps.newConcurrentMap();
   private final Map<T, T> upstream = Maps.newConcurrentMap();
+  private final Comparator<T> m_comparator;
 
   public GraphThreadPoolExecutor(
       String name,
@@ -35,7 +39,8 @@ public GraphThreadPoolExecutor(
       int maximumPoolSize,
       long keepAliveTime,
       TimeUnit unit,
-      BlockingQueue<Runnable> workQueue) {
+      BlockingQueue<Runnable> workQueue,
+      Comparator<T> comparator) {
     super(
         corePoolSize,
         maximumPoolSize,
@@ -45,6 +50,7 @@ public GraphThreadPoolExecutor(
         new TestNGThreadFactory(name));
     m_graph = graph;
     m_factory = factory;
+    m_comparator = comparator;
 
     if (m_graph.getFreeNodes().isEmpty()) {
       throw new TestNGException("The graph of methods contains a cycle:" + graph);
@@ -54,6 +60,9 @@ public GraphThreadPoolExecutor(
   public void run() {
     synchronized (m_graph) {
       List<T> freeNodes = m_graph.getFreeNodes();
+      if (m_comparator != null) {
+          Collections.sort(freeNodes, m_comparator);
+      }
       runNodes(freeNodes);
     }
   }
@@ -84,6 +93,9 @@ public void afterExecute(Runnable r, Throwable t) {
         shutdown();
       } else {
         List<T> freeNodes = m_graph.getFreeNodes();
+        if (m_comparator != null) {
+            Collections.sort(freeNodes, m_comparator);
+        }
         handleThreadAffinity(freeNodes);
         runNodes(freeNodes);
       }

src/test/java/org/testng/internal/DynamicGraphHelperTest.java

@@ -55,15 +55,31 @@ public void testCreateDynamicGraphWithDependency(Class<?> clazz) {
     assertThat(edges).isEmpty();
   }
 
+  @Test(dataProvider = "getSoftDependencyData")
+  public void testCreateDynamicGraphWithSoftDependency(Class<?> clazz) {
+    DynamicGraph<ITestNGMethod> graph = newGraph(clazz);
+    assertThat(graph.getFreeNodes()).hasSize(2);
+    Map<ITestNGMethod, Integer> edges = searchForMethod("b", graph);
+    assertThat(edges).isEmpty();
+    edges = searchForMethod("a", graph);
+    assertThat(edges).isEmpty();
+  }
+
   @DataProvider(name = "getDependencyData")
   public Object[][] getDependencyData() {
     return new Object[][] {
-      {SoftDependencyTestClassSample.class},
       {HardDependencyTestClassSample.class},
       {HardDependencyViaGroupsTestClassSample.class}
     };
   }
 
+  @DataProvider(name = "getSoftDependencyData")
+  public Object[][] getSoftDependencyData() {
+    return new Object[][] {
+      {SoftDependencyTestClassSample.class}
+    };
+  }
+
   @Test
   public void testCreateDynamicGraphWithPreserveOrderAndNoParallelism() {
     Class<?>[] classes = new Class<?>[] {SequentialClassA.class, SequentialClassB.class};

src/test/java/org/testng/internal/MethodInstanceTest.java

@@ -509,6 +509,14 @@ public int getPriority() {
     @Override
     public void setPriority(int priority) {}
 
+    @Override
+    public int getInterceptedPriority() {
+      return 0;
+    }
+
+    @Override
+    public void setInterceptedPriority(int priority) {}
+
     @Override
     public XmlTest getXmlTest() {
       return null;

src/test/java/org/testng/internal/paramhandler/FakeTestNGMethod.java

@@ -307,6 +307,14 @@ public int getPriority() {
 
   @Override
   public void setPriority(int priority) {}
+
+  @Override
+  public int getInterceptedPriority() {
+    return 0;
+  }
+
+  @Override
+  public void setInterceptedPriority(int priority) {}
 
   @Override
   public XmlTest getXmlTest() {

src/test/java/test/dependent/BaseOrderMethodTest.java

@@ -22,7 +22,7 @@ public void a_second0() {
 
   @Test(
       groups = {"3"},
-      dependsOnGroups = {"2.0"})
+      dependsOnGroups = {"2.0", "2.1"})
   public void third0() {
     verifyGroup(3, m_group2);
     m_group3[0] = true;

src/test/java/test/dependent/DependentTest.java

@@ -159,7 +159,7 @@ public static Object[][] dp1380() {
       {GitHub1380Sample.class, new String[] {"testMethodA", "testMethodB", "testMethodC"}, true},
       {GitHub1380Sample2.class, new String[] {"testMethodC", "testMethodB", "testMethodA"}, true},
       {GitHub1380Sample3.class, new String[] {"testMethodA", "testMethodB", "testMethodC"}, true},
-      {GitHub1380Sample4.class, new String[] {"testMethodB", "testMethodA", "testMethodC"}, true}
+      {GitHub1380Sample4.class, new String[] {"testMethodB", "testMethodC", "testMethodA"}, true}
     };
   }
 
@@ -175,7 +175,15 @@ public void simpleCyclingDependencyShouldWork(
     tng.addListener(listener);
 
     tng.run();
-
-    assertThat(listener.getSucceedMethodNames()).containsExactly(runMethods);
+
+    if (!isParallel) {
+        // When not running parallel, invoke order and succeed order are the same.
+        assertThat(listener.getInvokedMethodNames()).containsExactly(runMethods);
+        assertThat(listener.getSucceedMethodNames()).containsExactly(runMethods);
+    } else {
+        // When running parallel, invoke order is consistent, but succeed order isn't.
+        assertThat(listener.getInvokedMethodNames()).containsExactly(runMethods);
+        assertThat(listener.getSucceedMethodNames()).containsExactlyInAnyOrder(runMethods);
+    }
   }
 }