@Factory with dataProvider changes order of iterations

CHANGES.txt

@@ -1,4 +1,5 @@
 Current
+Fixed: GITHUB-799: @Factory with dataProvider changes order of iterations (Krishnan Mahadevan & Julien Herr)
 New: Enhance XML Reporter to be able to customize the file name (Krishnan Mahadevan)
 Fixed: GITHUB-1417: Class param injection is not working with @BeforeClass (Krishnan Mahadevan)
 Fixed: GITHUB-1440: Improve error message when wrong params on configuration methods (Krishnan Mahadevan)

src/main/java/org/testng/SuiteRunner.java

@@ -7,6 +7,7 @@
 import org.testng.internal.Attributes;
 import org.testng.internal.IConfiguration;
 import org.testng.internal.IInvoker;
+import org.testng.internal.Systematiser;
 import org.testng.internal.Utils;
 import org.testng.internal.annotations.IAnnotationFinder;
 import org.testng.internal.thread.ThreadUtil;
@@ -78,14 +79,40 @@ public class SuiteRunner implements ISuite, Serializable, IInvokedMethodListener
   private SuiteRunState suiteState = new SuiteRunState();
   private IAttributes attributes = new Attributes();
 
+  public SuiteRunner(IConfiguration configuration, XmlSuite suite, String outputDir,
+      Comparator<ITestNGMethod> comparator) {
+    this(configuration, suite, outputDir, null, comparator);
+  }
+
+  public SuiteRunner(IConfiguration configuration, XmlSuite suite, String outputDir,
+      ITestRunnerFactory runnerFactory, Comparator<ITestNGMethod> comparator) {
+    this(configuration, suite, outputDir, runnerFactory, false, comparator);
+  }
+
+  public SuiteRunner(IConfiguration configuration,
+      XmlSuite suite,
+      String outputDir,
+      ITestRunnerFactory runnerFactory,
+      boolean useDefaultListeners, Comparator<ITestNGMethod> comparator)
+  {
+    this(configuration, suite, outputDir, runnerFactory, useDefaultListeners,
+        new ArrayList<IMethodInterceptor>() /* method interceptor */,
+        null /* invoked method listeners */,
+        null /* test listeners */,
+        null /* class listeners */, comparator);
+  }
+
+  @Deprecated
   public SuiteRunner(IConfiguration configuration, XmlSuite suite, String outputDir) {
-    this(configuration, suite, outputDir, null);
+    this(configuration, suite, outputDir, (ITestRunnerFactory) null);
   }
 
+  @Deprecated
   public SuiteRunner(IConfiguration configuration, XmlSuite suite, String outputDir, ITestRunnerFactory runnerFactory) {
     this(configuration, suite, outputDir, runnerFactory, false);
   }
 
+  @Deprecated
   public SuiteRunner(IConfiguration configuration,
       XmlSuite suite,
       String outputDir,
@@ -96,7 +123,7 @@ public SuiteRunner(IConfiguration configuration,
         new ArrayList<IMethodInterceptor>() /* method interceptor */,
         null /* invoked method listeners */,
         null /* test listeners */,
-        null /* class listeners */);
+        null /* class listeners */, Systematiser.getComparator());
   }
 
   /**
@@ -115,9 +142,12 @@ protected SuiteRunner(IConfiguration configuration,
       List<ITestListener> testListeners,
       List<IClassListener> classListeners)
   {
-    init(configuration, suite, outputDir, runnerFactory, useDefaultListeners, methodInterceptors, invokedMethodListeners, testListeners, classListeners);
+    init(configuration, suite, outputDir, runnerFactory, useDefaultListeners,
+        methodInterceptors, invokedMethodListeners, testListeners, classListeners,
+        Systematiser.getComparator());
   }
 
+  @Deprecated
   protected SuiteRunner(IConfiguration configuration,
       XmlSuite suite,
       String outputDir,
@@ -128,7 +158,21 @@ protected SuiteRunner(IConfiguration configuration,
       Collection<ITestListener> testListeners,
       Collection<IClassListener> classListeners)
   {
-    init(configuration, suite, outputDir, runnerFactory, useDefaultListeners, methodInterceptors, invokedMethodListeners, testListeners, classListeners);
+    init(configuration, suite, outputDir, runnerFactory, useDefaultListeners, methodInterceptors, invokedMethodListeners, testListeners, classListeners, Systematiser.getComparator());
+  }
+
+  protected SuiteRunner(IConfiguration configuration,
+      XmlSuite suite,
+      String outputDir,
+      ITestRunnerFactory runnerFactory,
+      boolean useDefaultListeners,
+      List<IMethodInterceptor> methodInterceptors,
+      Collection<IInvokedMethodListener> invokedMethodListeners,
+      Collection<ITestListener> testListeners,
+      Collection<IClassListener> classListeners, Comparator<ITestNGMethod> comparator)
+  {
+    init(configuration, suite, outputDir, runnerFactory, useDefaultListeners,
+        methodInterceptors, invokedMethodListeners, testListeners, classListeners, comparator);
   }
 
   private void init(IConfiguration configuration,
@@ -139,8 +183,7 @@ private void init(IConfiguration configuration,
     List<IMethodInterceptor> methodInterceptors,
     Collection<IInvokedMethodListener> invokedMethodListener,
     Collection<ITestListener> testListeners,
-    Collection<IClassListener> classListeners)
-  {
+    Collection<IClassListener> classListeners, Comparator<ITestNGMethod> comparator) {
     this.configuration = configuration;
     xmlSuite = suite;
     this.useDefaultListeners = useDefaultListeners;
@@ -170,7 +213,10 @@ private void init(IConfiguration configuration,
         this.classListeners.put(classListener.getClass(), classListener);
       }
     }
-    this.runnerFactory = buildRunnerFactory();
+    if (comparator == null) {
+      throw new IllegalArgumentException("comparator must not be null");
+    }
+    this.runnerFactory = buildRunnerFactory(comparator);
 
     // Order the <test> tags based on their order of appearance in testng.xml
     List<XmlTest> xmlTests = xmlSuite.getTests();
@@ -240,13 +286,13 @@ private void setOutputDir(String outputdir) {
         : null;
   }
 
-  private ITestRunnerFactory buildRunnerFactory() {
+  private ITestRunnerFactory buildRunnerFactory(Comparator<ITestNGMethod> comparator) {
     ITestRunnerFactory factory;
 
     if (null == tmpRunnerFactory) {
       factory = new DefaultTestRunnerFactory(configuration,
           testListeners.toArray(new ITestListener[testListeners.size()]),
-          useDefaultListeners, skipFailedInvocationCounts);
+          useDefaultListeners, skipFailedInvocationCounts, comparator);
     }
     else {
       factory = new ProxyTestRunnerFactory(
@@ -555,16 +601,18 @@ private static class DefaultTestRunnerFactory implements ITestRunnerFactory {
     private boolean useDefaultListeners;
     private boolean skipFailedInvocationCounts;
     private IConfiguration configuration;
+    private final Comparator<ITestNGMethod> comparator;
 
     public DefaultTestRunnerFactory(IConfiguration configuration,
         ITestListener[] failureListeners,
         boolean useDefaultListeners,
-        boolean skipFailedInvocationCounts)
+        boolean skipFailedInvocationCounts, Comparator<ITestNGMethod> comparator)
     {
       this.configuration = configuration;
       failureGenerators = failureListeners;
       this.useDefaultListeners = useDefaultListeners;
       this.skipFailedInvocationCounts = skipFailedInvocationCounts;
+      this.comparator = comparator;
     }
 
     @Override
@@ -576,7 +624,7 @@ public TestRunner newTestRunner(ISuite suite, XmlTest test,
       }
       TestRunner testRunner = new TestRunner(configuration, suite, test,
           suite.getOutputDirectory(), suite.getAnnotationFinder(), skip,
-          listeners, classListeners);
+          listeners, classListeners, comparator);
 
       if (useDefaultListeners) {
         testRunner.addListener(new TestHTMLReporter());

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

@@ -32,6 +32,7 @@
 import org.testng.internal.IResultListener2;
 import org.testng.internal.OverrideProcessor;
 import org.testng.internal.SuiteRunnerMap;
+import org.testng.internal.Systematiser;
 import org.testng.internal.Utils;
 import org.testng.internal.Version;
 import org.testng.internal.annotations.DefaultAnnotationTransformer;
@@ -1393,7 +1394,7 @@ private SuiteRunner createSuiteRunner(XmlSuite xmlSuite) {
         m_methodInterceptors,
         m_invokedMethodListeners.values(),
         m_testListeners.values(),
-        m_classListeners.values());
+        m_classListeners.values(), Systematiser.getComparator());
 
     for (ISuiteListener isl : m_suiteListeners.values()) {
       result.addListener(isl);

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

@@ -36,6 +36,7 @@
 import org.testng.internal.MethodInstance;
 import org.testng.internal.ResultMap;
 import org.testng.internal.RunInfo;
+import org.testng.internal.Systematiser;
 import org.testng.internal.TestMethodWorker;
 import org.testng.internal.TestNGClassFinder;
 import org.testng.internal.TestNGMethodFinder;
@@ -67,6 +68,8 @@ public class TestRunner
 
   /* generated */
   private static final long serialVersionUID = 4247820024988306670L;
+
+  private final Comparator<ITestNGMethod> comparator;
   private ISuite m_suite;
   private XmlTest m_xmlTest;
   private String m_testName;
@@ -151,6 +154,7 @@ private enum PriorityWeight {
     groupByInstance, preserveOrder, priority, dependsOnGroups, dependsOnMethods
   }
 
+  @Deprecated
   protected TestRunner(IConfiguration configuration,
                     ISuite suite,
                     XmlTest test,
@@ -160,14 +164,40 @@ protected TestRunner(IConfiguration configuration,
                     Collection<IInvokedMethodListener> invokedMethodListeners,
                     List<IClassListener> classListeners)
   {
+    this.comparator = Systematiser.getComparator();
     init(configuration, suite, test, outputDirectory, finder, skipFailedInvocationCounts,
         invokedMethodListeners, classListeners);
   }
 
+  @Deprecated
   public TestRunner(IConfiguration configuration, ISuite suite, XmlTest test,
       boolean skipFailedInvocationCounts,
       Collection<IInvokedMethodListener> invokedMethodListeners,
       List<IClassListener> classListeners) {
+    this.comparator = Systematiser.getComparator();
+    init(configuration, suite, test, suite.getOutputDirectory(),
+        suite.getAnnotationFinder(),
+        skipFailedInvocationCounts, invokedMethodListeners, classListeners);
+  }
+
+  protected TestRunner(IConfiguration configuration,
+                    ISuite suite,
+                    XmlTest test,
+                    String outputDirectory,
+                    IAnnotationFinder finder,
+                    boolean skipFailedInvocationCounts,
+                    Collection<IInvokedMethodListener> invokedMethodListeners,
+                    List<IClassListener> classListeners, Comparator<ITestNGMethod> comparator) {
+    this.comparator = comparator;
+    init(configuration, suite, test, outputDirectory, finder, skipFailedInvocationCounts,
+        invokedMethodListeners, classListeners);
+  }
+
+  public TestRunner(IConfiguration configuration, ISuite suite, XmlTest test,
+      boolean skipFailedInvocationCounts,
+      Collection<IInvokedMethodListener> invokedMethodListeners,
+      List<IClassListener> classListeners, Comparator<ITestNGMethod> comparator) {
+    this.comparator = comparator;
     init(configuration, suite, test, suite.getOutputDirectory(),
         suite.getAnnotationFinder(),
         skipFailedInvocationCounts, invokedMethodListeners, classListeners);
@@ -405,7 +435,7 @@ private void initMethods() {
                                              m_configuration,
                                              this);
     ITestMethodFinder testMethodFinder
-      = new TestNGMethodFinder(m_runInfo, m_annotationFinder);
+      = new TestNGMethodFinder(m_runInfo, m_annotationFinder, comparator);
 
     m_runInfo.setTestMethods(testMethods);
 
@@ -462,36 +492,36 @@ private void initMethods() {
                                                               m_runInfo,
                                                               m_annotationFinder,
                                                               true /* unique */,
-                                                              m_excludedMethods);
+                                                              m_excludedMethods, comparator);
 
     m_beforeXmlTestMethods = MethodHelper.collectAndOrderMethods(beforeXmlTestMethods,
                                                               false /* forTests */,
                                                               m_runInfo,
                                                               m_annotationFinder,
                                                               true /* unique (CQ added by me)*/,
-                                                              m_excludedMethods);
+                                                              m_excludedMethods, comparator);
 
     m_allTestMethods = MethodHelper.collectAndOrderMethods(testMethods,
                                                                 true /* forTest? */,
                                                                 m_runInfo,
                                                                 m_annotationFinder,
                                                                 false /* unique */,
-                                                                m_excludedMethods);
+                                                                m_excludedMethods, comparator);
     m_classMethodMap = new ClassMethodMap(testMethods, m_xmlMethodSelector);
 
     m_afterXmlTestMethods = MethodHelper.collectAndOrderMethods(afterXmlTestMethods,
                                                               false /* forTests */,
                                                               m_runInfo,
                                                               m_annotationFinder,
                                                               true /* unique (CQ added by me)*/,
-                                                              m_excludedMethods);
+                                                              m_excludedMethods, comparator);
 
     m_afterSuiteMethods = MethodHelper.collectAndOrderMethods(afterSuiteMethods,
                                                               false /* forTests */,
                                                               m_runInfo,
                                                               m_annotationFinder,
                                                               true /* unique */,
-                                                              m_excludedMethods);
+                                                              m_excludedMethods, comparator);
     // shared group methods
     m_groupMethods = new ConfigurationGroupMethods(m_allTestMethods, beforeGroupMethods, afterGroupMethods);
 

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

@@ -6,6 +6,7 @@
 
 import java.util.Collection;
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.HashSet;
 import java.util.LinkedList;
 import java.util.List;
@@ -21,12 +22,17 @@ public class Graph<T> {
   private static boolean m_verbose = false;
   private Map<T, Node<T>> m_nodes = Maps.newLinkedHashMap();
   private List<T> m_strictlySortedNodes = null;
+  private final Comparator<Node<T>> comparator;
 
   //  A map of nodes that are not the predecessors of any node
   // (not needed for the algorithm but convenient to calculate
   // the parallel/sequential lists in TestNG).
   private Map<T, Node<T>> m_independentNodes = null;
 
+  public Graph(Comparator<Node<T>> comparator) {
+    this.comparator = comparator;
+  }
+
   public void addNode(T tm) {
     ppp("ADDING NODE " + tm + " " + tm.hashCode());
     m_nodes.put(tm, new Node<>(tm));
@@ -121,7 +127,7 @@ public void topologicalSort() {
     // Sort the nodes alphabetically to make sure that methods of the same class
     // get run close to each other as much as possible
     //
-    Collections.sort(nodes2);
+    Collections.sort(nodes2, comparator);
 
     //
     // Sort
@@ -158,9 +164,9 @@ private void initializeIndependentNodes() {
     if (null == m_independentNodes) {
       List<Node<T>> 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. Therefore, all the nodes were mostly sorted alphabetically. So we need to keep
-      // the behavior for now.
-      Collections.sort(list);
+      // dependent nodes.
+      Collections.sort(list, comparator);
+
       m_independentNodes = Maps.newLinkedHashMap();
       for (Node<T> node : list) {
         m_independentNodes.put(node.getObject(), node);
@@ -255,7 +261,7 @@ public String toString() {
   /////
   // class Node
   //
-  public static class Node<T> implements Comparable<Node<T>> {
+  public static class Node<T>  {
     private T m_object = null;
     private Map<T, T> m_predecessors = Maps.newHashMap();
 
@@ -335,9 +341,5 @@ public boolean hasPredecessor(T m) {
       return m_predecessors.containsKey(m);
     }
 
-    @Override
-    public int compareTo(Node<T> o) {
-      return getObject().toString().compareTo(o.getObject().toString());
-    }
   }
 }