@factory with dataProvider changes order of iterations

Closes #799

CHANGES.txt

@@ -1,4 +1,5 @@
 Current
+Fixed: GITHUB-799: @Factory with dataProvider changes order of iterations (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)
 Fixed: GITHUB-1433: Missing encoding for emailable reports (Shaburov Oleg)

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

@@ -5,7 +5,6 @@
 import org.testng.collections.Maps;
 
 import java.util.Collection;
-import java.util.Collections;
 import java.util.HashSet;
 import java.util.LinkedList;
 import java.util.List;
@@ -117,12 +116,6 @@ 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);
-
     //
     // Sort
     //
@@ -157,10 +150,6 @@ public void topologicalSort() {
   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);
       m_independentNodes = Maps.newLinkedHashMap();
       for (Node<T> node : list) {
         m_independentNodes.put(node.getObject(), node);

src/test/java/test/SimpleBaseTest.java

@@ -35,6 +35,8 @@
 import java.util.UUID;
 import java.util.regex.Pattern;
 
+import static org.assertj.core.api.Assertions.assertThat;
+
 public class SimpleBaseTest {
   // System property specifying where the resources (e.g. xml files) can be found
   private static final String TEST_RESOURCES_DIR = "test.resources.dir";
@@ -242,7 +244,7 @@ protected static void addMethods(XmlClass cls, String... methods) {
   }
 
   public static String getPathToResource(String fileName) {
-    String result = System.getProperty(TEST_RESOURCES_DIR);
+    String result = System.getProperty(TEST_RESOURCES_DIR,"src/test/resources");
     if (result == null) {
       throw new IllegalArgumentException("System property " + TEST_RESOURCES_DIR + " was not defined.");
     }
@@ -340,4 +342,14 @@ protected static List<Integer> grep(File fileName, String regexp, List<String> r
 
   }
 
+  protected static void doAssert(List<String> actual, String... expectedValues) {
+    doAssert(actual, Arrays.asList(expectedValues));
+  }
+
+  protected static void doAssert(List<String> actual, List<String>  expected) {
+    for (String each : actual) {
+      assertThat(each).isIn(expected);
+    }
+  }
+
 }

src/test/java/test/Test1.java

@@ -8,6 +8,8 @@
 import org.testng.xml.XmlTest;
 import test.sample.Sample1;
 
+import java.util.Arrays;
+import java.util.List;
 import java.util.logging.Handler;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -36,7 +38,8 @@ public void includedGroups() {
 
         tng.run();
 
-        assertThat(listener.getSucceedMethodNames()).containsExactly("method1", "method3");
+        List<String> expected = Arrays.asList("method1", "method3");
+        doAssert(listener.getSucceedMethodNames(), expected);
         assertThat(listener.getFailedMethodNames()).isEmpty();
     }
 
@@ -55,7 +58,9 @@ public void groupsOfGroupsSimple() {
 
         tng.run();
 
-        assertThat(listener.getSucceedMethodNames()).containsExactly("method1", "method2", "method3");
+        List<String> expected = Arrays.asList("method1", "method2", "method3");
+        doAssert(listener.getSucceedMethodNames(), expected);
+
         assertThat(listener.getFailedMethodNames()).isEmpty();
     }
 
@@ -73,8 +78,8 @@ public void groupsOfGroupsWithIndirections() {
         tng.addListener((ITestNGListener) listener);
 
         tng.run();
-
-        assertThat(listener.getSucceedMethodNames()).containsExactly("method1", "broken", "method2", "method3");
+        List<String> expected = Arrays.asList("method1", "broken", "method2", "method3");
+        doAssert(listener.getSucceedMethodNames(), expected);
         assertThat(listener.getFailedMethodNames()).isEmpty();
     }
 
@@ -108,15 +113,18 @@ public void excludedGroups() {
         tng.addListener((ITestNGListener) listener);
 
         tng.run();
-
-        assertThat(listener.getSucceedMethodNames()).containsExactly(
+        List<String> expected = Arrays.asList(
                 "broken", "method2",
                 "throwExpectedException1ShouldPass",
                 "throwExpectedException2ShouldPass"
         );
-        assertThat(listener.getFailedMethodNames()).containsExactly(
+        doAssert(listener.getSucceedMethodNames(), expected);
+
+        expected = Arrays.asList(
                 "throwExceptionShouldFail", "verifyLastNameShouldFail"
         );
+
+        doAssert(listener.getFailedMethodNames(), expected);
     }
 
     @Test
@@ -132,7 +140,7 @@ public void regexp() {
 
         tng.run();
 
-        assertThat(listener.getSucceedMethodNames()).containsExactly("method1", "method3");
+        doAssert(listener.getSucceedMethodNames(), "method1", "method3");
         assertThat(listener.getFailedMethodNames()).isEmpty();
     }
 

src/test/java/test/annotationtransformer/AnnotationTransformerTest.java

@@ -1,5 +1,7 @@
 package test.annotationtransformer;
 
+import org.assertj.core.api.Condition;
+import org.assertj.core.api.ListAssert;
 import org.assertj.core.api.iterable.Extractor;
 import org.testng.Assert;
 import org.testng.IAnnotationTransformer;
@@ -42,13 +44,12 @@ public void verifyAnnotationWithoutTransformer() {
 
     tng.run();
 
-    assertThat(tla.getPassedTests()).extracting(NAME_EXTRACTOR)
-        .containsExactly(
-            "five",
-            "four", "four", "four", "four", "four",
-            "three", "three", "three", "three", "three",
-            "two", "two"
-        );
+    ListAssert passedTests = assertThat(tla.getPassedTests()).extracting(NAME_EXTRACTOR);
+    passedTests.areExactly(5, new NameCondition("four"));
+    passedTests.areExactly(1, new NameCondition("five"));
+    passedTests.areExactly(5, new NameCondition("three"));
+    passedTests.areExactly(2, new NameCondition("two"));
+
     assertThat(tla.getFailedTests()).extracting(NAME_EXTRACTOR)
         .containsExactly("verify");
   }
@@ -70,15 +71,14 @@ public void verifyAnnotationTransformerMethod() {
     tng.run();
 
     assertThat(transformer.getMethodNames()).contains("two", "three", "four", "five", "verify");
+    ListAssert passedTests = assertThat(tla.getPassedTests()).extracting(NAME_EXTRACTOR);
+
+    passedTests.areExactly(4, new NameCondition("four"));
+    passedTests.areExactly(5, new NameCondition("five"));
+    passedTests.areExactly(3, new NameCondition("three"));
+    passedTests.areExactly(2, new NameCondition("two"));
+    passedTests.areExactly(1, new NameCondition("verify"));
 
-    assertThat(tla.getPassedTests()).extracting(NAME_EXTRACTOR)
-        .containsExactly(
-            "five", "five", "five", "five", "five",
-            "four", "four", "four", "four",
-            "three", "three", "three",
-            "two", "two",
-            "verify"
-        );
     assertThat(tla.getFailedTests()).isEmpty();
   }
 
@@ -249,4 +249,15 @@ public void annotationTransformerInXmlShouldBeRun() throws Exception {
     Assert.assertEquals(tla.getPassedTests().size(), 1);
 
   }
+
+  class NameCondition extends Condition<String> {
+    private String name;
+    public NameCondition(String name) {
+      this.name = name;
+    }
+    @Override
+    public boolean matches(String value) {
+      return name.equals(value);
+    }
+  }
 }

src/test/java/test/dataprovider/DataProviderTest.java

@@ -10,6 +10,8 @@
 import test.SimpleBaseTest;
 
 import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
 
 import static org.assertj.core.api.Assertions.assertThat;
 
@@ -18,31 +20,36 @@ public class DataProviderTest extends SimpleBaseTest {
   @Test(description = "GITHUB-1139")
   public void oneDimDataProviderShouldWork() {
     InvokedMethodNameListener listener = run(OneDimDataProviderSample.class);
-
-    assertThat(listener.getSucceedMethodNames()).containsExactly(
-        "testArray(foo)", "testArray(bar)",
-        "testIterator(foo)", "testIterator(bar)",
-        "testStaticArray(foo)", "testStaticArray(bar)",
-        "testStaticIterator(foo)", "testStaticIterator(bar)"
+    List<String> expected = Arrays.asList(
+            "testArray(foo)", "testArray(bar)",
+            "testIterator(foo)", "testIterator(bar)",
+            "testStaticArray(foo)", "testStaticArray(bar)",
+            "testStaticIterator(foo)", "testStaticIterator(bar)"
     );
+
+    for (String method : listener.getSucceedMethodNames()) {
+      assertThat(method).isIn(expected);
+    }
   }
 
   @Test
   public void booleanTest() {
     InvokedMethodNameListener listener = run(BooleanDataProviderSample.class);
-
-    assertThat(listener.getSucceedMethodNames()).containsExactly(
-        "doStuff(true)", "doStuff(false)"
-    );
+    List<String> expected = Arrays.asList("doStuff(true)", "doStuff(false)");
+    for (String method : listener.getSucceedMethodNames()) {
+      assertThat(method).isIn(expected);
+    }
   }
 
   @Test
   public void classTest() {
     InvokedMethodNameListener listener = run(ClassDataProviderSample.class);
+    List<String> expected = Arrays.asList("f(a)", "f(b)", "g(a)", "g(b)");
+
+    for (String method : listener.getSucceedMethodNames()) {
+      assertThat(method).isIn(expected);
+    }
 
-    assertThat(listener.getSucceedMethodNames()).containsExactly(
-        "f(a)", "f(b)", "g(a)", "g(b)"
-    );
   }
 
   @Test
@@ -133,12 +140,12 @@ public void methodTest() {
     MethodSample.m_test3 = 0;
 
     InvokedMethodNameListener listener = run(MethodSample.class);
+    List<String> expected = Arrays.asList("test1(Cedric)", "test1(Alois)", "test2(Cedric)", "test3(Cedric)");
+
+    for (String method : listener.getSucceedMethodNames()) {
+      assertThat(method).isIn(expected);
+    }
 
-    assertThat(listener.getSucceedMethodNames()).containsExactly(
-            "test1(Cedric)", "test1(Alois)",
-            "test2(Cedric)",
-            "test3(Cedric)"
-    );
     Assert.assertEquals(MethodSample.m_test2, 1);
     Assert.assertEquals(MethodSample.m_test3, 1);
   }
@@ -237,23 +244,29 @@ public void parallelDataProviderSample() {
   @Test
   public void staticDataProviderTest() {
     InvokedMethodNameListener listener = run(StaticDataProviderSampleSample.class);
-
-    assertThat(listener.getSucceedMethodNames()).containsExactly(
-        "verifyConstructorInjection(Cedric)",
-        "verifyExternal(Cedric)",
-        "verifyFieldInjection(Cedric)",
-        "verifyStatic(Cedric)"
+    List<String> expected = Arrays.asList(
+            "verifyConstructorInjection(Cedric)",
+            "verifyExternal(Cedric)",
+            "verifyFieldInjection(Cedric)",
+            "verifyStatic(Cedric)"
     );
+    for (String method : listener.getSucceedMethodNames()){
+      assertThat(method).isIn(expected);
+    }
+
   }
 
   @Test
   public void staticDataProviderSampleWithoutGuiceTest() {
     InvokedMethodNameListener listener = run(StaticDataProviderSampleWithoutGuiceSample.class);
-
-    assertThat(listener.getSucceedMethodNames()).containsExactly(
-        "verifyExternal(Cedric)",
-        "verifyStatic(Cedric)"
+    List<String> expected = Arrays.asList(
+            "verifyExternal(Cedric)",
+            "verifyStatic(Cedric)"
     );
+
+    for (String method : listener.getSucceedMethodNames()){
+      assertThat(method).isIn(expected);
+    }
   }
 
   @Test
@@ -274,7 +287,10 @@ public void testNG411Test() {
 
     assertThat(listener.getSucceedMethodNames()).hasSize(1)
         .are(new RegexCondition("checkMinTest_injection\\(1,2,org\\.testng\\.TestRunner@\\p{XDigit}+\\)"));
-    assertThat(listener.getFailedBeforeInvocationMethodNames()).containsExactly("checkMaxTest", "checkMinTest");
+    List<String> expected = Arrays.asList("checkMaxTest", "checkMinTest");
+    for (String method : listener.getFailedBeforeInvocationMethodNames()) {
+      assertThat(method).isIn(expected);
+    }
   }
 
   @Test

src/test/java/test/dataprovider/FailingDataProviderTest.java

@@ -4,6 +4,9 @@
 import test.InvokedMethodNameListener;
 import test.SimpleBaseTest;
 
+import java.util.Arrays;
+import java.util.List;
+
 import static org.assertj.core.api.Assertions.assertThat;
 
 public class FailingDataProviderTest extends SimpleBaseTest {
@@ -25,9 +28,12 @@ public void duplicateDataProviders() {
   @Test
   public void failingDataProviderAndInvocationCount() {
     InvokedMethodNameListener listener = run(DataProviderWithErrorSample.class);
-
-    assertThat(listener.getSkippedMethodNames()).containsExactly(
-        "testShouldSkip", "testShouldSkip", "testShouldSkipEvenIfSuccessPercentage", "testShouldSkipEvenIfSuccessPercentage"
+    List<String> expected = Arrays.asList(
+            "testShouldSkip", "testShouldSkip", "testShouldSkipEvenIfSuccessPercentage", "testShouldSkipEvenIfSuccessPercentage"
     );
+
+    for (String method : listener.getSkippedMethodNames()) {
+      assertThat(method).isIn(expected);
+    }
   }
 }