Specifying dataProvider & successPercentage causes test to always pass

Closes testng-team#3170

CHANGES.txt

@@ -1,4 +1,5 @@
 Current (7.11.0)
+Fixed: GITHUB-3170: Specifying dataProvider and successPercentage causes test to always pass (Krishnan Mahadevan)
 Fixed: GITHUB-3028: Execution stalls when using "use-global-thread-pool" (Krishnan Mahadevan)
 Fixed: GITHUB-3122: Update JCommander to 1.83 (Antoine Dessaigne)
 Fixed: GITHUB-3135: assertEquals on arrays - Failure message is missing information about the array index when an array element is unexpectedly null or non-null (Albert Choi)

testng-core/src/main/java/org/testng/internal/invokers/BaseInvoker.java

@@ -110,8 +110,8 @@ protected void handleException(
     if (throwable != null && testResult.getThrowable() == null) {
       testResult.setThrowable(throwable);
     }
-    int successPercentage = testMethod.getSuccessPercentage();
     int invocationCount = testMethod.getInvocationCount();
+    int successPercentage = testMethod.getSuccessPercentage();
     float numberOfTestsThatCanFail = ((100 - successPercentage) * invocationCount) / 100f;
 
     if (failureCount < numberOfTestsThatCanFail) {

testng-core/src/main/java/org/testng/internal/invokers/ITestInvoker.java

@@ -1,6 +1,8 @@
 package org.testng.internal.invokers;
 
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
 import org.testng.IInvokedMethod;
@@ -19,6 +21,7 @@ class FailureContext {
     AtomicInteger count = new AtomicInteger(0);
     List<Object> instances = Lists.newArrayList();
     AtomicBoolean representsRetriedMethod = new AtomicBoolean(false);
+    final Map<String, AtomicInteger> counter = new HashMap<>();
   }
 
   List<ITestResult> invokeTestMethods(

testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java

@@ -557,7 +557,8 @@ private void handleInvocationResult(
       if (holder.status == ITestResult.FAILURE && !holder.handled) {
         int count = failure.count.getAndIncrement();
         if (testMethod.isDataDriven()) {
-          count = 0;
+          String key = Arrays.toString(testResult.getParameters());
+          count = failure.counter.computeIfAbsent(key, k -> new AtomicInteger()).incrementAndGet();
         }
         handleException(testResult.getThrowable(), testMethod, testResult, count);
       }

testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java

@@ -1,10 +1,20 @@
 package test.invocationcount;
 
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.concurrent.atomic.AtomicInteger;
 import org.testng.Assert;
+import org.testng.IInvokedMethod;
+import org.testng.IInvokedMethodListener;
+import org.testng.ITestResult;
 import org.testng.TestListenerAdapter;
 import org.testng.TestNG;
+import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 import test.SimpleBaseTest;
+import test.invocationcount.issue1719.IssueTest;
+import test.invocationcount.issue3170.DataDrivenWithSuccessPercentageAndInvocationCountDefinedSample;
+import test.invocationcount.issue3170.DataDrivenWithSuccessPercentageDefinedSample;
 
 public class FailedInvocationCountTest extends SimpleBaseTest {
 
@@ -41,4 +51,50 @@ public void verifyAttributeShouldStop() {
     Assert.assertEquals(tla.getFailedTests().size(), 7);
     Assert.assertEquals(tla.getSkippedTests().size(), 5);
   }
+
+  @Test(dataProvider = "dp")
+  public void ensureSuccessPercentageWorksFineWith(Class<?> clazz, IssueTest.Expected expected) {
+    TestNG testng = create(clazz);
+    AtomicInteger failed = new AtomicInteger(0);
+    AtomicInteger passed = new AtomicInteger(0);
+    AtomicInteger failedWithInSuccessPercentage = new AtomicInteger(0);
+    testng.addListener(
+        new IInvokedMethodListener() {
+          @Override
+          public void afterInvocation(IInvokedMethod method, ITestResult testResult) {
+
+            switch (testResult.getStatus()) {
+              case ITestResult.SUCCESS:
+                passed.incrementAndGet();
+                break;
+              case ITestResult.FAILURE:
+                failed.incrementAndGet();
+                break;
+              case ITestResult.SUCCESS_PERCENTAGE_FAILURE:
+                failedWithInSuccessPercentage.incrementAndGet();
+                break;
+              default:
+            }
+          }
+        });
+    testng.run();
+    assertThat(passed.get()).isEqualTo(expected.success());
+    assertThat(failed.get()).isEqualTo(expected.failures());
+    assertThat(failedWithInSuccessPercentage.get())
+        .isEqualTo(expected.failedWithinSuccessPercentage());
+  }
+
+  @DataProvider(name = "dp")
+  public Object[][] dp() {
+    return new Object[][] {
+      {
+        DataDrivenWithSuccessPercentageAndInvocationCountDefinedSample.class,
+        new IssueTest.Expected().failures(10)
+      },
+      {
+        DataDrivenWithSuccessPercentageDefinedSample.class,
+        new IssueTest.Expected().failures(3).success(1)
+      }
+    };
+  }
 }

testng-core/src/test/java/test/invocationcount/issue1719/IssueTest.java

@@ -1,22 +1,128 @@
 package test.invocationcount.issue1719;
 
-import static org.assertj.core.api.Assertions.assertThat;
-
+import java.util.Set;
+import org.assertj.core.api.SoftAssertions;
+import org.testng.ITestResult;
 import org.testng.TestNG;
+import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 import test.SimpleBaseTest;
 
 public class IssueTest extends SimpleBaseTest {
 
-  @Test
-  public void testSuccessPercentageCalculation() {
-    TestNG testng = create(TestclassSample.class);
+  @Test(dataProvider = "dp")
+  public void testSuccessPercentageCalculation(Class<?> clazz, Expected expected) {
+    TestNG testng = create(clazz);
     DummyReporter listener = new DummyReporter();
     testng.addListener(listener);
     testng.run();
-    assertThat(listener.getFailures()).isEmpty();
-    assertThat(listener.getSkip()).isEmpty();
-    assertThat(listener.getSuccess()).isEmpty();
-    assertThat(listener.getFailedWithinSuccessPercentage()).hasSize(5);
+    SoftAssertions assertions = new SoftAssertions();
+    String msg =
+        pretty(
+            "[failedWithinSuccessPercentage]",
+            expected.failedWithinSuccessPercentage,
+            listener.getFailedWithinSuccessPercentage());
+    assertions
+        .assertThat(listener.getFailedWithinSuccessPercentage())
+        .withFailMessage(msg)
+        .hasSize(expected.failedWithinSuccessPercentage);
+
+    msg = pretty("[skip]", expected.skip, listener.getSkip());
+    assertions.assertThat(listener.getSkip()).withFailMessage(msg).hasSize(expected.skip);
+
+    msg = pretty("[success]", expected.success, listener.getSuccess());
+    assertions.assertThat(listener.getSuccess()).withFailMessage(msg).hasSize(expected.success);
+
+    msg = pretty("[failures]", expected.failures, listener.getFailures());
+    assertions.assertThat(listener.getFailures()).withFailMessage(msg).hasSize(expected.failures);
+
+    assertions.assertAll();
+  }
+
+  private static String pretty(String prefix, int expected, Set<ITestResult> actual) {
+    return prefix + " test. Expected: " + expected + ", Actual: " + actual.size();
+  }
+
+  @DataProvider(name = "dp")
+  public Object[][] dp() {
+    return new Object[][] {
+      {
+        TestclassSample.DataDrivenTestHavingZeroSuccessPercentageAndNoInvocationCount.class,
+        new Expected().failures(2)
+      },
+      {
+        TestclassSample.DataDrivenTestHavingValidSuccessPercentageAndInvocationCount.class,
+        // Parameter - Invocation Count - Expected Test Result
+        // .  1             1               Failed Within success percentage (30% expected)
+        //    1             2               Passed (Remember this is a flaky test simulation)
+        //    2             1               Failed Within success percentage (30% expected)
+        //    2             2               Failed
+        new Expected().failures(1).failedWithinSuccessPercentage(2).success(1)
+      },
+      {
+        TestclassSample.RegularTestWithZeroSuccessPercentage.class,
+        new Expected().failedWithinSuccessPercentage(1)
+      },
+      {
+        TestclassSample.RegularTestWithZeroSuccessPercentageAndInvocationCount.class,
+        new Expected().failedWithinSuccessPercentage(2)
+      },
+    };
+  }
+
+  public static class Expected {
+    private int failures = 0;
+    private int success = 0;
+    private int skip = 0;
+    private int failedWithinSuccessPercentage = 0;
+
+    public int failures() {
+      return failures;
+    }
+
+    public Expected failures(int failures) {
+      this.failures = failures;
+      return this;
+    }
+
+    public int success() {
+      return success;
+    }
+
+    public Expected success(int success) {
+      this.success = success;
+      return this;
+    }
+
+    public int skip() {
+      return skip;
+    }
+
+    public Expected skip(int skip) {
+      this.skip = skip;
+      return this;
+    }
+
+    public int failedWithinSuccessPercentage() {
+      return failedWithinSuccessPercentage;
+    }
+
+    public Expected failedWithinSuccessPercentage(int failedWithinSuccessPercentage) {
+      this.failedWithinSuccessPercentage = failedWithinSuccessPercentage;
+      return this;
+    }
+
+    @Override
+    public String toString() {
+      return "{failures="
+          + failures
+          + ", success="
+          + success
+          + ", skip="
+          + skip
+          + ", failedWithinSuccessPercentage="
+          + failedWithinSuccessPercentage
+          + '}';
+    }
   }
 }

testng-core/src/test/java/test/invocationcount/issue1719/TestclassSample.java

@@ -1,28 +1,60 @@
 package test.invocationcount.issue1719;
 
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
 import org.testng.Assert;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 
 public class TestclassSample {
 
-  @Test(successPercentage = 0, dataProvider = "dp")
-  public void dataDrivenTestMethod(int i) {
-    Assert.fail("Failing iteration:" + i);
-  }
-
   @DataProvider(name = "dp")
-  public Object[][] getData() {
+  public static Object[][] getData() {
     return new Object[][] {{1}, {2}};
   }
 
-  @Test(successPercentage = 0)
-  public void simpleTestMethod() {
-    Assert.fail();
+  public static class DataDrivenTestHavingZeroSuccessPercentageAndNoInvocationCount {
+    @Test(successPercentage = 0, dataProvider = "dp", dataProviderClass = TestclassSample.class)
+    public void dataDrivenTestMethod(int i) {
+      Assert.fail("Failing iteration:" + i);
+    }
+  }
+
+  public static class DataDrivenTestHavingValidSuccessPercentageAndInvocationCount {
+
+    private boolean shouldFail = true;
+    private Map<Integer, AtomicInteger> tracker = new HashMap<>();
+
+    @Test(
+        successPercentage = 30,
+        dataProvider = "dp",
+        invocationCount = 2,
+        dataProviderClass = TestclassSample.class)
+    public void dataDrivenTestMethodWithInvocationCount(int i) {
+      int current = tracker.computeIfAbsent(i, k -> new AtomicInteger()).incrementAndGet();
+      String msg = String.format("Parameter [%d], Invocation [%d]", i, current);
+      if (i != 1) { // If the parameter is NOT 1, then just fail
+        Assert.fail("Failing test " + msg);
+      }
+      if (shouldFail) { // If the parameter is 1, then simulate a flaky test that passes and fails
+        shouldFail = false;
+        Assert.fail("Failing test " + msg);
+      }
+    }
+  }
+
+  public static class RegularTestWithZeroSuccessPercentage {
+    @Test(successPercentage = 0)
+    public void simpleTestMethod() {
+      Assert.fail();
+    }
   }
 
-  @Test(successPercentage = 0, invocationCount = 2)
-  public void testMethodWithMultipleInvocations() {
-    Assert.fail();
+  public static class RegularTestWithZeroSuccessPercentageAndInvocationCount {
+    @Test(successPercentage = 0, invocationCount = 2)
+    public void testMethodWithMultipleInvocations() {
+      Assert.fail();
+    }
   }
 }

testng-core/src/test/java/test/invocationcount/issue3170/DataDrivenWithSuccessPercentageAndInvocationCountDefinedSample.java

@@ -0,0 +1,18 @@
+package test.invocationcount.issue3170;
+
+import static org.testng.Assert.assertEquals;
+
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+public class DataDrivenWithSuccessPercentageAndInvocationCountDefinedSample {
+  @Test(dataProvider = "test", invocationCount = 10, successPercentage = 90)
+  public void sampleTestCase(String string) {
+    assertEquals(string, "1");
+  }
+
+  @DataProvider(name = "test")
+  public Object[][] testProvider() {
+    return new Object[][] {{"2"}};
+  }
+}

testng-core/src/test/java/test/invocationcount/issue3170/DataDrivenWithSuccessPercentageDefinedSample.java

@@ -0,0 +1,18 @@
+package test.invocationcount.issue3170;
+
+import static org.testng.Assert.assertEquals;
+
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+public class DataDrivenWithSuccessPercentageDefinedSample {
+  @Test(dataProvider = "test", successPercentage = 99)
+  public void sampleTestCase(String string) {
+    assertEquals(string, "1");
+  }
+
+  @DataProvider(name = "test")
+  public Object[][] testProvider() {
+    return new Object[][] {{"1"}, {"2"}, {"3"}, {"4"}};
+  }
+}