@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)
 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/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;
@@ -121,7 +122,10 @@ 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);
+    Comparator<Node> comparator = Systematiser.getComparator();
+    if (comparator != null) {
+      Collections.sort(nodes2, comparator);
+    }
 
     //
     // Sort
@@ -160,7 +164,10 @@ private void initializeIndependentNodes() {
       // 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);
+      Comparator<Node> comparator = Systematiser.getComparator();
+      if (comparator != null) {
+        Collections.sort(list, comparator);
+      }
       m_independentNodes = Maps.newLinkedHashMap();
       for (Node<T> node : list) {
         m_independentNodes.put(node.getObject(), node);
@@ -255,7 +262,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 +342,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());
-    }
   }
 }

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

@@ -0,0 +1,86 @@
+package org.testng.internal;
+
+import org.testng.ITestNGMethod;
+
+import java.util.Comparator;
+
+public class Systematiser {
+
+    private Systematiser() {
+        //Utility class. Defeat instantiation.
+    }
+
+    public static Comparator<Graph.Node> getComparator() {
+        Comparator<Graph.Node> comparator = null;
+        String text = System.getProperty("testng.order", Order.INSTANCES.getValue());
+
+        Order order = Order.parse(text);
+        switch (order) {
+            case INSTANCES:
+                comparator = new Comparator<Graph.Node>() {
+                    @Override
+                    public int compare(Graph.Node o1, Graph.Node o2) {
+                        return o1.getObject().toString().compareTo(o2.getObject().toString());
+                    }
+
+                    @Override
+                    public String toString() {
+                        return "Instance_Names";
+                    }
+                };
+                break;
+
+            case METHOD_NAMES:
+                comparator = new Comparator<Graph.Node>() {
+                    @Override
+                    public int compare(Graph.Node o1, Graph.Node o2) {
+                        if (o1.getObject() instanceof ITestNGMethod && o2.getObject() instanceof ITestNGMethod) {
+                            String n1 = ((ITestNGMethod) o1.getObject()).getMethodName();
+                            String n2 = ((ITestNGMethod) o1.getObject()).getMethodName();
+                            return n1.compareTo(n2);
+                        }
+                        return o1.getObject().getClass().getName().compareTo(o2.getObject().getClass().getName());
+                    }
+
+                    @Override
+                    public String toString() {
+                        return "Method_Names";
+                    }
+                };
+                break;
+
+            case NONE:
+            default:
+        }
+
+        return comparator;
+    }
+
+    enum Order {
+        METHOD_NAMES("methods"),
+        INSTANCES("instances"),
+        NONE("none");
+
+        Order(String value) {
+            this.value = value;
+        }
+
+        private String value;
+
+        public String getValue() {
+            return value;
+        }
+
+        public static Order parse(String value) {
+            if (value == null || value.trim().isEmpty()) {
+                return INSTANCES;
+            }
+            for (Order each : values()) {
+                if (each.getValue().equalsIgnoreCase(value)) {
+                    return each;
+                }
+            }
+            return INSTANCES;
+        }
+    }
+}

src/test/java/test/github799/EnsureInstancesAreOrderedViaFactories.java

@@ -0,0 +1,66 @@
+package test.github799;
+
+import org.testng.Assert;
+import org.testng.IInvokedMethod;
+import org.testng.IInvokedMethodListener;
+import org.testng.ITestNGListener;
+import org.testng.ITestResult;
+import org.testng.Reporter;
+import org.testng.TestNG;
+import org.testng.annotations.Test;
+import org.testng.collections.Lists;
+import test.SimpleBaseTest;
+
+import java.util.List;
+
+public class EnsureInstancesAreOrderedViaFactories extends SimpleBaseTest {
+
+    @Test
+    public void testMethod() {
+        System.setProperty("testng.order", "none");
+        runTest(TestSample.class, "1", "2", "3", "4");
+    }
+
+    @Test
+    public void randomOrderTestMethod() {
+        System.setProperty("testng.order", "none");
+        runTest(ReverseOrderTestSample.class, "4", "1", "3", "2");
+    }
+
+    @Test
+    public void methodsOrderTest() {
+        System.setProperty("testng.order", "methods");
+        runTest(MethodsTestSample.class, "android", "angry", "birds");
+    }
+
+    @Test
+    public void testInstancesOrder() {
+        System.setProperty("testng.order", "instances");
+        runTest(InstanceTestSample.class, "Master Oogway:90", "Master Shifu:50");
+    }
+
+    private void runTest(Class<?> clazz, String... expected) {
+        TestNG tng = create(clazz);
+        OrderEavesdropper listener = new OrderEavesdropper();
+        tng.addListener((ITestNGListener) listener);
+        tng.run();
+
+        for (int i = 0; i < expected.length; i++) {
+            String actual = listener.messages.get(i);
+            Assert.assertEquals(actual, expected[i]);
+        }
+    }
+
+    public static class OrderEavesdropper implements IInvokedMethodListener {
+        List<String> messages = Lists.newArrayList();
+
+        @Override
+        public void beforeInvocation(IInvokedMethod method, ITestResult testResult) {
+        }
+
+        @Override
+        public void afterInvocation(IInvokedMethod method, ITestResult testResult) {
+            messages.addAll(Reporter.getOutput(testResult));
+        }
+    }
+}

src/test/java/test/github799/InstanceTestSample.java

@@ -0,0 +1,37 @@
+package test.github799;
+
+import org.testng.Assert;
+import org.testng.Reporter;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Factory;
+import org.testng.annotations.Test;
+
+public class InstanceTestSample {
+    private String name;
+    private int age;
+
+    @Factory(dataProvider = "dp")
+    public InstanceTestSample(String name, int age) {
+        this.name = name;
+        this.age = age;
+    }
+
+    @DataProvider(name = "dp")
+    public static Object[][] getData() {
+        return new Object[][]{
+                {"Master Shifu", 50},
+                {"Master Oogway", 90}
+        };
+    }
+
+    @Test
+    public void testMethod() {
+        Reporter.log(toString());
+        Assert.assertNotNull(this.name);
+    }
+
+    @Override
+    public String toString() {
+        return name + ":" + age;
+    }
+}

src/test/java/test/github799/MethodsTestSample.java

@@ -0,0 +1,28 @@
+package test.github799;
+
+import org.testng.Assert;
+import org.testng.Reporter;
+import org.testng.annotations.Test;
+
+public class MethodsTestSample {
+    @Test
+    public void angry() {
+        String methodName = Reporter.getCurrentTestResult().getMethod().getMethodName();
+        Reporter.log(methodName);
+        Assert.assertNotNull(methodName);
+    }
+
+    @Test
+    public void birds() {
+        String methodName = Reporter.getCurrentTestResult().getMethod().getMethodName();
+        Reporter.log(methodName);
+        Assert.assertNotNull(methodName);
+    }
+
+    @Test
+    public void android() {
+        String methodName = Reporter.getCurrentTestResult().getMethod().getMethodName();
+        Reporter.log(methodName);
+        Assert.assertNotNull(methodName);
+    }
+}

src/test/java/test/github799/ReverseOrderTestSample.java

@@ -0,0 +1,27 @@
+package test.github799;
+
+import org.testng.Assert;
+import org.testng.Reporter;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Factory;
+import org.testng.annotations.Test;
+
+public class ReverseOrderTestSample {
+    int num;
+
+    @Factory(dataProvider = "data")
+    public ReverseOrderTestSample(int n) {
+        num = n;
+    }
+
+    @DataProvider
+    public static Object[][] data() {
+        return new Object[][]{{4}, {1}, {3}, {2}};
+    }
+
+    @Test
+    public void test() {
+        Reporter.log(Integer.toString(num));
+        Assert.assertTrue(num > 0);
+    }
+}

src/test/java/test/github799/TestSample.java

@@ -0,0 +1,27 @@
+package test.github799;
+
+import org.testng.Assert;
+import org.testng.Reporter;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Factory;
+import org.testng.annotations.Test;
+
+public class TestSample {
+    int num;
+
+    @Factory(dataProvider = "data")
+    public TestSample(int n) {
+        num = n;
+    }
+
+    @DataProvider
+    public static Object[][] data() {
+        return new Object[][]{{1}, {2}, {3}, {4}};
+    }
+
+    @Test
+    public void test() {
+        Reporter.log(Integer.toString(num));
+        Assert.assertTrue(num > 0);
+    }
+}

src/test/resources/testng.xml

@@ -815,12 +815,19 @@
       <class name="test.thread.parallelization.ParallelByMethodsTestCase8Scenario1"/>
     </classes>
   </test>
+
   <!--This test depends on System properties. So its intentionally being run at the end
   because the toggling of the system property will affect other tests -->
   <test name = "RunAtLast">
     <classes>
       <class name="test.dataprovider.issue128.DataProviderParametersMismatchTest"/>
     </classes>
   </test>
+
+  <test name="RunOrderingTest">
+    <classes>
+      <class name="test.github799.EnsureInstancesAreOrderedViaFactories"/>
+    </classes>
+  </test>
 </suite>