Streamline invocation numbers in failed xml file

Closes #3180

CHANGES.txt

@@ -1,4 +1,5 @@
 Current (7.11.0)
+Fixed: GITHUB-3180: TestNG testng-failed.xml 'invocation-numbers' values are not calculated correctly with retry and dataproviders (Krishnan Mahadevan)
 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)

testng-asserts/src/test/java/org/testng/AssertTest.java

@@ -582,21 +582,21 @@ public void testAssertNotEqualsWithNull() {
 
   @Test(description = "GITHUB-3140")
   public void testAssertEqualsDeepSet() {
-    var expectedSet = new HashSet<>();
+    var expectedSet = new LinkedHashSet<>();
     expectedSet.add(new Contrived(1));
     expectedSet.add(new Contrived[] {new Contrived(1)});
-    var actualSet = new HashSet<>();
+    var actualSet = new LinkedHashSet<>();
     actualSet.add(new Contrived(1));
     actualSet.add(new Contrived[] {new Contrived(1)});
     Assert.assertEqualsDeep(actualSet, expectedSet);
   }
 
   @Test(description = "GITHUB-3140", expectedExceptions = AssertionError.class)
   public void testAssertEqualsDeepSetFail() {
-    var expectedSet = new HashSet<>();
+    var expectedSet = new LinkedHashSet<>();
     expectedSet.add(new Contrived(1));
     expectedSet.add(new Contrived[] {new Contrived(1)});
-    var actualSet = new HashSet<>();
+    var actualSet = new LinkedHashSet<>();
     actualSet.add(new Contrived(1));
     actualSet.add(new Contrived[] {new Contrived(2)});
     Assert.assertEqualsDeep(actualSet, expectedSet);

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

@@ -764,7 +764,9 @@ private ITestResult invokeMethod(
         if (null != testResult.getMethod().getFactoryMethodParamsInfo()) {
           parametersIndex = testResult.getMethod().getFactoryMethodParamsInfo().getIndex();
         }
-        arguments.getTestMethod().addFailedInvocationNumber(parametersIndex);
+        if (!willRetryMethod) {
+          arguments.getTestMethod().addFailedInvocationNumber(parametersIndex);
+        }
       }
 
       //

testng-core/src/main/java/org/testng/reporters/FailedReporter.java

@@ -8,7 +8,9 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.Function;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 import org.testng.IReporter;
@@ -42,6 +44,7 @@ public class FailedReporter implements IReporter {
   public static final String TESTNG_FAILED_XML = "testng-failed.xml";
 
   private XmlSuite m_xmlSuite;
+  private final Map<String, Map<String, String>> keyCache = new ConcurrentHashMap<>();
 
   public FailedReporter() {}
 
@@ -65,15 +68,14 @@ protected void generateFailureSuite(XmlSuite xmlSuite, ISuite suite, String outp
 
     Map<String, ISuiteResult> results = suite.getResults();
 
+    boolean shouldWriteIntoFile = true;
+
     for (Map.Entry<String, ISuiteResult> entry : results.entrySet()) {
       ISuiteResult suiteResult = entry.getValue();
       ITestContext testContext = suiteResult.getTestContext();
 
-      generateXmlTest(
-          testContext.getCurrentXmlTest(),
-          testContext,
-          testContext.getFailedTests().getAllResults(),
-          testContext.getSkippedTests().getAllResults());
+      shouldWriteIntoFile = shouldWriteIntoFile && generateXmlTest(testContext);
+      clearKeyCache(testContext);
     }
 
     if (null != failedSuite.getTests() && !failedSuite.getTests().isEmpty()) {
@@ -83,20 +85,48 @@ protected void generateFailureSuite(XmlSuite xmlSuite, ISuite suite, String outp
             Lists.merge(failedSuite.getListeners(), xmlSuite.getParentSuite().getListeners());
         failedSuite.setListeners(merged);
       }
-      Utils.writeUtf8File(outputDir, TESTNG_FAILED_XML, failedSuite.toXml());
-      Utils.writeUtf8File(suite.getOutputDirectory(), TESTNG_FAILED_XML, failedSuite.toXml());
+      if (shouldWriteIntoFile) {
+        Utils.writeUtf8File(outputDir, TESTNG_FAILED_XML, failedSuite.toXml());
+        Utils.writeUtf8File(suite.getOutputDirectory(), TESTNG_FAILED_XML, failedSuite.toXml());
+      }
+    }
+  }
+
+  private void clearKeyCache(ITestContext ctx) {
+    keyCache.remove(ctx.getName());
+  }
+
+  private static String key(ITestResult it) {
+    String prefix = it.getMethod().getQualifiedName() + it.getInstance().toString();
+    if (it.getParameters().length != 0) {
+      return prefix + Arrays.toString(it.getParameters());
     }
+    return prefix + it.getMethod().getCurrentInvocationCount();
   }
 
-  private void generateXmlTest(
-      XmlTest xmlTest,
-      ITestContext context,
-      Set<ITestResult> failedTests,
-      Set<ITestResult> skippedTests) {
+  private static Map<String, String> buildMap(Set<ITestResult> passed) {
+    return passed
+        .parallelStream()
+        .map(FailedReporter::key)
+        .collect(
+            Collectors.toUnmodifiableMap(Function.identity(), Function.identity(), (s1, s2) -> s1));
+  }
+
+  private boolean isFlakyTest(Set<ITestResult> passed, ITestResult result) {
+    String ctxKey = result.getTestContext().getName();
+    String individualKey = key(result);
+    return keyCache.computeIfAbsent(ctxKey, k -> buildMap(passed)).containsKey(individualKey);
+  }
+
+  private boolean generateXmlTest(ITestContext context) {
+    XmlTest xmlTest = context.getCurrentXmlTest();
+    Set<ITestResult> failedTests = context.getFailedTests().getAllResults();
+    Set<ITestResult> skippedTests = context.getSkippedTests().getAllResults();
     // Note:  we can have skipped tests and no failed tests
     // if a method depends on nonexistent groups
     if (!skippedTests.isEmpty() || !failedTests.isEmpty()) {
       Set<ITestNGMethod> methodsToReRun = Sets.newHashSet();
+      Set<ITestResult> passedTests = context.getPassedTests().getAllResults();
 
       // Get the transitive closure of all the failed methods and the methods
       // they depend on
@@ -109,6 +139,11 @@ private void generateXmlTest(
         if (!current.isTest()) { // Don't count configuration methods
           continue;
         }
+        boolean repetitiveTest = failedTest.getMethod().getInvocationCount() > 0;
+        boolean isDataDriven = failedTest.getMethod().isDataDriven();
+        if ((repetitiveTest || isDataDriven) && isFlakyTest(passedTests, failedTest)) {
+          continue;
+        }
         methodsToReRun.add(current);
         List<ITestNGMethod> methodsDependedUpon =
             MethodHelper.getMethodsDependedUpon(current, allTestMethods, MethodSorting.basedOn());
@@ -142,6 +177,10 @@ private void generateXmlTest(
         }
       }
 
+      if (methodsToReRun.isEmpty()) {
+        return false;
+      }
+
       Set<ITestNGMethod> upstreamConfigFailures =
           Stream.of(
                   context.getFailedConfigurations().getAllMethods(),
@@ -152,7 +191,9 @@ private void generateXmlTest(
       result.addAll(upstreamConfigFailures);
       result.addAll(relevantConfigs);
       createXmlTest(context, result, xmlTest);
+      return true;
     }
+    return false;
   }
 
   private static boolean isNotClassLevelConfigurationMethod(ITestNGMethod each) {

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

@@ -2,7 +2,17 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 
+import java.io.StringReader;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.concurrent.atomic.AtomicInteger;
+import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.xpath.XPath;
+import javax.xml.xpath.XPathConstants;
+import javax.xml.xpath.XPathExpression;
+import javax.xml.xpath.XPathFactory;
+import org.assertj.core.api.SoftAssertions;
 import org.testng.Assert;
 import org.testng.IInvokedMethod;
 import org.testng.IInvokedMethodListener;
@@ -11,10 +21,17 @@
 import org.testng.TestNG;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
+import org.testng.reporters.FailedReporter;
+import org.w3c.dom.Document;
+import org.w3c.dom.Element;
+import org.w3c.dom.Node;
+import org.w3c.dom.NodeList;
+import org.xml.sax.InputSource;
 import test.SimpleBaseTest;
 import test.invocationcount.issue1719.IssueTest;
 import test.invocationcount.issue3170.DataDrivenWithSuccessPercentageAndInvocationCountDefinedSample;
 import test.invocationcount.issue3170.DataDrivenWithSuccessPercentageDefinedSample;
+import test.invocationcount.issue3180.SampleTestContainer;
 
 public class FailedInvocationCountTest extends SimpleBaseTest {
 
@@ -97,4 +114,74 @@ public Object[][] dp() {
       }
     };
   }
+
+  @DataProvider(name = "github-3180")
+  public Object[][] getTestData() {
+    String[] nothing = new String[] {""};
+    boolean noXml = false;
+    boolean xmlSeen = true;
+    return new Object[][] {
+      // Test has flaky iterations which pass eventually. So no failed xml should be seen.
+      {SampleTestContainer.TestContainsFlakyDataDrivenTest.class, noXml, nothing},
+      // Repetitive test that eventually passes. So no failed xml should be seen.
+      {SampleTestContainer.TestContainsPercentageDrivenTest.class, noXml, nothing},
+      // Not a test that repeats. So no invocation count attribute should be seen
+      {SampleTestContainer.TestWithNormalFailingTest.class, xmlSeen, nothing},
+      // Flaky test. So ensure only flaky invocations are referenced
+      {SampleTestContainer.TestWithSomeFailingIterations.class, xmlSeen, new String[] {"0 2"}},
+      // Flaky test. So ensure only flaky invocations are referenced
+      {
+        SampleTestContainer.TestContainsAlwaysFailingDataDrivenTest.class,
+        xmlSeen,
+        new String[] {"0"}
+      },
+      // This is a combination of all the earlier permutations.
+      // So we should see an xml that contains only the true cases from earlier
+      {
+        SampleTestContainer.TestContainsAllCombinations.class,
+        xmlSeen,
+        new String[] {"", "0 2", "0"}
+      }
+    };
+  }
+
+  @Test(description = "GITHUB-3180", dataProvider = "github-3180")
+  public void ensureInvocationCountHonoursRetriesWhenUsingDataProviders(
+      Class<?> cls, boolean isXmlGenerated, String[] invocationCountValue) throws Exception {
+    String reportsDir = createDirInTempDir("3180").getAbsolutePath();
+    TestNG testng = create(Paths.get(reportsDir), cls);
+    testng.setUseDefaultListeners(false);
+    testng.addListener(new FailedReporter());
+    testng.run();
+    Path xml = Paths.get(reportsDir, "testng-failed.xml");
+    assertThat(xml.toFile().exists()).isEqualTo(isXmlGenerated);
+    if (!isXmlGenerated) {
+      // Do not validate anything if the xml is NOT generated
+      return;
+    }
+    String xmlContent = Files.readString(xml);
+    Document document = document(xmlContent);
+    XPathFactory xPathFactory = XPathFactory.newInstance();
+    XPath xPath = xPathFactory.newXPath();
+    XPathExpression xPathExpression = xPath.compile("//methods/include");
+
+    NodeList nodeList = (NodeList) xPathExpression.evaluate(document, XPathConstants.NODESET);
+    assertThat(nodeList.getLength()).isEqualTo(invocationCountValue.length);
+    SoftAssertions softly = new SoftAssertions();
+    for (int i = 0; i < nodeList.getLength(); i++) {
+      Node node = nodeList.item(i);
+      assertThat(node).isNotNull();
+      assertThat(node.getNodeType()).isEqualTo(Node.ELEMENT_NODE);
+      Element element = (Element) node;
+      String value = element.getAttribute("invocation-numbers");
+      softly.assertThat(value).isEqualTo(invocationCountValue[i]);
+    }
+    softly.assertAll();
+  }
+
+  private static Document document(String xmlContent) throws Exception {
+    return DocumentBuilderFactory.newInstance()
+        .newDocumentBuilder()
+        .parse(new InputSource(new StringReader(xmlContent)));
+  }
 }

testng-core/src/test/java/test/invocationcount/issue3180/RetryAnalyzer.java

@@ -0,0 +1,17 @@
+package test.invocationcount.issue3180;
+
+import org.testng.IRetryAnalyzer;
+import org.testng.ITestResult;
+
+public class RetryAnalyzer implements IRetryAnalyzer {
+
+  int counter = 0;
+  int retryLimit = 3;
+
+  /*
+   * Retry method
+   */
+  public boolean retry(ITestResult result) {
+    return counter++ < retryLimit;
+  }
+}

testng-core/src/test/java/test/invocationcount/issue3180/SampleTestCase.java

@@ -0,0 +1,23 @@
+package test.invocationcount.issue3180;
+
+import static org.testng.Assert.assertEquals;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+public class SampleTestCase {
+
+  @Test(dataProvider = "dp", retryAnalyzer = RetryAnalyzer.class)
+  public void testDataProvider(AtomicInteger counter) {
+    assertEquals(counter.incrementAndGet(), 3);
+  }
+
+  @Test
+  public void normalTestMethod() {}
+
+  @DataProvider(name = "dp")
+  public Object[][] createTestData() {
+    return new Object[][] {{new AtomicInteger(0)}};
+  }
+}