From 27ed972961f54598bf6a92fea65518c67da04efb Mon Sep 17 00:00:00 2001 From: Krishnan Mahadevan Date: Fri, 27 Sep 2024 12:25:28 +0530 Subject: [PATCH] Streamline invocation numbers in failed xml file Closes #3180 --- CHANGES.txt | 1 + .../src/test/java/org/testng/AssertTest.java | 8 +- .../testng/internal/invokers/TestInvoker.java | 4 +- .../org/testng/reporters/FailedReporter.java | 163 +++++++++++------- .../FailedInvocationCountTest.java | 159 +++++++++++++++++ .../issue3180/RetryAnalyzer.java | 17 ++ .../issue3180/SampleTestContainer.java | 106 ++++++++++++ 7 files changed, 392 insertions(+), 66 deletions(-) create mode 100644 testng-core/src/test/java/test/invocationcount/issue3180/RetryAnalyzer.java create mode 100644 testng-core/src/test/java/test/invocationcount/issue3180/SampleTestContainer.java diff --git a/CHANGES.txt b/CHANGES.txt index 4a8bf0245..99f86edc3 100644 --- a/CHANGES.txt +++ b/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) diff --git a/testng-asserts/src/test/java/org/testng/AssertTest.java b/testng-asserts/src/test/java/org/testng/AssertTest.java index ba97f0c67..4b626c3a0 100644 --- a/testng-asserts/src/test/java/org/testng/AssertTest.java +++ b/testng-asserts/src/test/java/org/testng/AssertTest.java @@ -582,10 +582,10 @@ 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); @@ -593,10 +593,10 @@ public void testAssertEqualsDeepSet() { @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); diff --git a/testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java b/testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java index 1269a251a..b48d458ef 100644 --- a/testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java +++ b/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); + } } // diff --git a/testng-core/src/main/java/org/testng/reporters/FailedReporter.java b/testng-core/src/main/java/org/testng/reporters/FailedReporter.java index 068af3808..8358fcec4 100644 --- a/testng-core/src/main/java/org/testng/reporters/FailedReporter.java +++ b/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> keyCache = new ConcurrentHashMap<>(); public FailedReporter() {} @@ -69,11 +72,12 @@ protected void generateFailureSuite(XmlSuite xmlSuite, ISuite suite, String outp ISuiteResult suiteResult = entry.getValue(); ITestContext testContext = suiteResult.getTestContext(); - generateXmlTest( - testContext.getCurrentXmlTest(), - testContext, - testContext.getFailedTests().getAllResults(), - testContext.getSkippedTests().getAllResults()); + boolean shouldWriteIntoFile = generateXmlTest(testContext); + XmlTest current = testContext.getCurrentXmlTest(); + failedSuite + .getTests() + .removeIf(it -> !shouldWriteIntoFile && it.getName().equals(current.getName())); + clearKeyCache(testContext); } if (null != failedSuite.getTests() && !failedSuite.getTests().isEmpty()) { @@ -88,71 +92,108 @@ protected void generateFailureSuite(XmlSuite xmlSuite, ISuite suite, String outp } } - private void generateXmlTest( - XmlTest xmlTest, - ITestContext context, - Set failedTests, - Set skippedTests) { + 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 static Map buildMap(Set passed) { + return passed + .parallelStream() + .map(FailedReporter::key) + .collect( + Collectors.toUnmodifiableMap(Function.identity(), Function.identity(), (s1, s2) -> s1)); + } + + private boolean isFlakyTest(Set 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 failedTests = context.getFailedTests().getAllResults(); + Set 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 methodsToReRun = Sets.newHashSet(); - - // Get the transitive closure of all the failed methods and the methods - // they depend on - Set allTests = Sets.newHashSet(); - allTests.addAll(failedTests); - allTests.addAll(skippedTests); - ITestNGMethod[] allTestMethods = context.getAllTestMethods(); - for (ITestResult failedTest : allTests) { - ITestNGMethod current = failedTest.getMethod(); - if (!current.isTest()) { // Don't count configuration methods - continue; - } - methodsToReRun.add(current); - List methodsDependedUpon = - MethodHelper.getMethodsDependedUpon(current, allTestMethods, MethodSorting.basedOn()); + if (skippedTests.isEmpty() && failedTests.isEmpty()) { + return false; + } + Set methodsToReRun = Sets.newHashSet(); + Set passedTests = context.getPassedTests().getAllResults(); + + // Get the transitive closure of all the failed methods and the methods + // they depend on + Set allTests = Sets.newHashSet(); + allTests.addAll(failedTests); + allTests.addAll(skippedTests); + ITestNGMethod[] allTestMethods = context.getAllTestMethods(); + for (ITestResult failedTest : allTests) { + ITestNGMethod current = failedTest.getMethod(); + 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 methodsDependedUpon = + MethodHelper.getMethodsDependedUpon(current, allTestMethods, MethodSorting.basedOn()); - for (ITestNGMethod m : methodsDependedUpon) { - if (m.isTest()) { - methodsToReRun.add(m); - } + for (ITestNGMethod m : methodsDependedUpon) { + if (m.isTest()) { + methodsToReRun.add(m); } } + } - // - // Now we have all the right methods. Go through the list of - // all the methods that were run and only pick those that are - // in the methodToReRun map. Since the methods are already - // sorted, we don't need to sort them again. - // - List result = Lists.newArrayList(); - Set relevantConfigs = Sets.newHashSet(); - for (ITestNGMethod m : allTestMethods) { - if (RuntimeBehavior.isMemoryFriendlyMode()) { - // We are doing this because the `m` would not be of type - // LiteWeightTestNGMethod and hence the hashCode() and equals() - // computation would be different. - m = new LiteWeightTestNGMethod(m); - } - if (methodsToReRun.contains(m)) { - result.add(m); - getAllApplicableConfigs(relevantConfigs, m.getTestClass()); - getAllGroupApplicableConfigs(context, relevantConfigs, m); - } + // + // Now we have all the right methods. Go through the list of + // all the methods that were run and only pick those that are + // in the methodToReRun map. Since the methods are already + // sorted, we don't need to sort them again. + // + List result = Lists.newArrayList(); + Set relevantConfigs = Sets.newHashSet(); + for (ITestNGMethod m : allTestMethods) { + if (RuntimeBehavior.isMemoryFriendlyMode()) { + // We are doing this because the `m` would not be of type + // LiteWeightTestNGMethod and hence the hashCode() and equals() + // computation would be different. + m = new LiteWeightTestNGMethod(m); + } + if (methodsToReRun.contains(m)) { + result.add(m); + getAllApplicableConfigs(relevantConfigs, m.getTestClass()); + getAllGroupApplicableConfigs(context, relevantConfigs, m); } + } - Set upstreamConfigFailures = - Stream.of( - context.getFailedConfigurations().getAllMethods(), - context.getSkippedConfigurations().getAllMethods()) - .flatMap(Collection::stream) - .filter(FailedReporter::isNotClassLevelConfigurationMethod) - .collect(Collectors.toSet()); - result.addAll(upstreamConfigFailures); - result.addAll(relevantConfigs); - createXmlTest(context, result, xmlTest); + if (methodsToReRun.isEmpty()) { + return false; } + + Set upstreamConfigFailures = + Stream.of( + context.getFailedConfigurations().getAllMethods(), + context.getSkippedConfigurations().getAllMethods()) + .flatMap(Collection::stream) + .filter(FailedReporter::isNotClassLevelConfigurationMethod) + .collect(Collectors.toSet()); + result.addAll(upstreamConfigFailures); + result.addAll(relevantConfigs); + createXmlTest(context, result, xmlTest); + return true; } private static boolean isNotClassLevelConfigurationMethod(ITestNGMethod each) { diff --git a/testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java b/testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java index 53788b724..6181cba65 100644 --- a/testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java +++ b/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,18 @@ import org.testng.TestNG; import org.testng.annotations.DataProvider; import org.testng.annotations.Test; +import org.testng.reporters.FailedReporter; +import org.testng.xml.XmlSuite; +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 +115,145 @@ 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(); + } + + @DataProvider(name = "github-3180-test-tags") + public Object[][] getTestData1() { + 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", "0 2"} + }, + // Flaky test. So ensure only flaky invocations are referenced + { + SampleTestContainer.TestContainsAlwaysFailingDataDrivenTest.class, + xmlSeen, + new String[] {"0", "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", "", "0 2", "0"} + } + }; + } + + @Test(description = "GITHUB-3180", dataProvider = "github-3180-test-tags") + public void ensureInvocationCountHonoursRetriesWhenUsingMultipleTestTags( + Class cls, boolean isXmlGenerated, String[] invocationCountValue) throws Exception { + String reportsDir = createDirInTempDir("3180").getAbsolutePath(); + XmlSuite xmlSuite = createXmlSuite("sample_suite"); + createXmlTest(xmlSuite, "sample_test1", cls); + createXmlTest(xmlSuite, "sample_test2", cls); + TestNG testng = create(Paths.get(reportsDir), xmlSuite); + 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))); + } } diff --git a/testng-core/src/test/java/test/invocationcount/issue3180/RetryAnalyzer.java b/testng-core/src/test/java/test/invocationcount/issue3180/RetryAnalyzer.java new file mode 100644 index 000000000..8feedd5e6 --- /dev/null +++ b/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; + } +} diff --git a/testng-core/src/test/java/test/invocationcount/issue3180/SampleTestContainer.java b/testng-core/src/test/java/test/invocationcount/issue3180/SampleTestContainer.java new file mode 100644 index 000000000..92040dd8c --- /dev/null +++ b/testng-core/src/test/java/test/invocationcount/issue3180/SampleTestContainer.java @@ -0,0 +1,106 @@ +package test.invocationcount.issue3180; + +import static org.testng.Assert.assertEquals; + +import java.util.concurrent.atomic.AtomicInteger; +import org.testng.Assert; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; + +public class SampleTestContainer { + @DataProvider + public static Object[][] atomicNumbers() { + return new Object[][] {{new AtomicInteger(0)}}; + } + + @DataProvider + public static Object[][] numbers() { + return new Object[][] {{1}, {2}, {3}}; + } + + public static class TestContainsFlakyDataDrivenTest { + + @Test( + dataProvider = "atomicNumbers", + dataProviderClass = SampleTestContainer.class, + retryAnalyzer = RetryAnalyzer.class) + public void testDataProvider(AtomicInteger counter) { + assertEquals(counter.incrementAndGet(), 3); + } + } + + public static class TestWithNormalFailingTest { + @Test(retryAnalyzer = RetryAnalyzer.class) + public void alwaysFailingRegularTest() { + Assert.fail(); + } + } + + public static class TestWithSomeFailingIterations { + + @Test( + dataProvider = "numbers", + dataProviderClass = SampleTestContainer.class, + retryAnalyzer = RetryAnalyzer.class) + public void failsForOddNumbersOnly(int i) { + assertEquals(i % 2, 0); + } + } + + public static class TestContainsAlwaysFailingDataDrivenTest { + @Test( + dataProvider = "atomicNumbers", + dataProviderClass = SampleTestContainer.class, + retryAnalyzer = RetryAnalyzer.class) + public void alwaysFailingDataDrivenTest(AtomicInteger ignored) { + Assert.fail(); + } + } + + public static class TestContainsPercentageDrivenTest { + private final AtomicInteger switcher = new AtomicInteger(1); + + @Test(invocationCount = 10, successPercentage = 90, retryAnalyzer = RetryAnalyzer.class) + public void invocationCountTestWhichEventuallyPassesDueToSuccessFactors() { + Assert.assertTrue(switcher.getAndIncrement() < 9); + } + } + + public static class TestContainsAllCombinations { + private final AtomicInteger switcher = new AtomicInteger(1); + + @Test( + dataProvider = "atomicNumbers", + dataProviderClass = SampleTestContainer.class, + retryAnalyzer = RetryAnalyzer.class) + public void testDataProvider(AtomicInteger counter) { + assertEquals(counter.incrementAndGet(), 3); + } + + @Test(retryAnalyzer = RetryAnalyzer.class) + public void alwaysFailingRegularTest() { + Assert.fail(); + } + + @Test( + dataProvider = "numbers", + dataProviderClass = SampleTestContainer.class, + retryAnalyzer = RetryAnalyzer.class) + public void failsForOddNumbersOnly(int i) { + assertEquals(i % 2, 0); + } + + @Test( + dataProvider = "atomicNumbers", + dataProviderClass = SampleTestContainer.class, + retryAnalyzer = RetryAnalyzer.class) + public void alwaysFailingDataDrivenTest(AtomicInteger ignored) { + Assert.fail(); + } + + @Test(invocationCount = 10, successPercentage = 90) + public void invocationCountTestWhichEventuallyPassesDueToSuccessFactors() { + Assert.assertTrue(switcher.getAndIncrement() < 9); + } + } +}