Streamline invocation numbers in failed xml file #3181
Conversation
WalkthroughThe pull request introduces version 7.11.0 of the software, focusing on bug fixes and minor enhancements. Key changes include improvements to TestNG's data provider functionality, updates to assertion methods for better error reporting, and refinements in test execution logic. Additionally, new classes and test cases are added to enhance testing capabilities, particularly around retries and invocation counts. The changes maintain backward compatibility while improving overall reliability and accuracy. Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
krmahadevan
force-pushed
the
fix_3180
branch
from
bebe004 to
8a568d4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (6)
testng-core/src/test/java/test/invocationcount/issue3180/RetryAnalyzer.java (1)
11-13: Improve or remove the comment above the retry method.The comment "Retry method" above the
retrymethod doesn't provide any additional information beyond what the method name already conveys. Consider either removing this comment or expanding it to provide more meaningful information about the method's behavior or usage.If you decide to keep a comment, consider something more informative, like:
/** * Determines whether a failed test should be retried. * @param result The result of the test execution. * @return true if the test should be retried, false otherwise. */This improved comment provides more context about the method's purpose and parameters.
testng-asserts/src/test/java/org/testng/AssertTest.java (2)
585-591: LGTM! Consider adding a comment explaining the use of LinkedHashSet.The change from
HashSettoLinkedHashSetis a good improvement as it ensures consistent ordering during deep equality checks. This addresses the issue described in GITHUB-3140.Consider adding a brief comment explaining why
LinkedHashSetis used instead ofHashSet. This would help future maintainers understand the reasoning behind this specific implementation. For example:// Using LinkedHashSet to ensure consistent ordering for deep equality checks var expectedSet = new LinkedHashSet<>();
596-602: LGTM! Consider adding a comment for consistency with the previous test.The change to use
LinkedHashSetis consistent with the previous test method and correctly implements the failure case for deep equality checks.For consistency with the previous test method, consider adding a brief comment explaining the use of
LinkedHashSet. This would maintain a uniform style across related test cases. For example:// Using LinkedHashSet to ensure consistent ordering for deep equality checks var expectedSet = new LinkedHashSet<>();CHANGES.txt (1)
Line range hint
9-2802: Review of historical changesAfter scanning through the changelog, here are some significant historical changes and patterns that might still be relevant:
- Continuous improvement in parallel execution and thread management (e.g., GITHUB-1835, GITHUB-772, GITHUB-1173).
- Ongoing enhancements to reporting capabilities (e.g., GITHUB-1402, GITHUB-1213, GITHUB-1231).
- Regular updates to support newer Java versions and deprecate older features (e.g., GITHUB-2637, GITHUB-1456).
- Frequent improvements in handling data providers and factory methods (e.g., GITHUB-1241, GITHUB-1770, GITHUB-2669).
- Continuous refinement of group and method dependencies (e.g., GITHUB-1658, GITHUB-893, GITHUB-165).
These historical changes show a pattern of continuous improvement and adaptation to new Java features and testing paradigms. They also highlight areas that have required ongoing attention, such as concurrency, reporting, and complex test configurations.
Based on these historical changes, here are some architectural considerations for future development:
Concurrency and Parallelism: Given the frequent updates in this area, it might be beneficial to review and possibly refactor the core concurrency model to make it more robust and easier to maintain.
Reporting Framework: Consider developing a more modular and extensible reporting system to accommodate the frequent additions and changes to reporting features.
Dependency Management: The ongoing refinements to group and method dependencies suggest that this area might benefit from a more flexible and powerful dependency resolution system.
Java Version Support: Maintain a clear strategy for supporting new Java versions while gracefully deprecating support for older versions.
Data Provider and Factory Method Handling: These features seem to be common pain points. Consider a comprehensive review and possible redesign of these systems to make them more robust and flexible.
By addressing these areas, TestNG could potentially reduce the frequency of related issues and provide a more stable and powerful testing framework for its users.
testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java (1)
121-122: Clarify boolean variable naming for better readabilityThe boolean variables
noXmlandxmlSeenmay cause confusion due to similar naming and opposite meanings. Consider renaming them to more descriptive names likexmlExpectedFalseandxmlExpectedTrueor combining them into a single variable for clarity.Apply this diff to rename the variables:
- boolean noXml = false; - boolean xmlSeen = true; + boolean xmlExpected = false; + boolean xmlExpected = true;And update their usage accordingly in the test data:
{SampleTestContainer.TestContainsFlakyDataDrivenTest.class, noXml, nothing}, {SampleTestContainer.TestContainsPercentageDrivenTest.class, noXml, nothing}, {SampleTestContainer.TestWithNormalFailingTest.class, xmlSeen, nothing},Becomes:
{SampleTestContainer.TestContainsFlakyDataDrivenTest.class, xmlExpectedFalse, nothing}, {SampleTestContainer.TestContainsPercentageDrivenTest.class, xmlExpectedFalse, nothing}, {SampleTestContainer.TestWithNormalFailingTest.class, xmlExpectedTrue, nothing},testng-core/src/main/java/org/testng/reporters/FailedReporter.java (1)
88-91: Avoid Duplicate File WritesAt lines 89-90, the code writes
failedSuite.toXml()to two directories:Utils.writeUtf8File(outputDir, TESTNG_FAILED_XML, failedSuite.toXml()); Utils.writeUtf8File(suite.getOutputDirectory(), TESTNG_FAILED_XML, failedSuite.toXml());Unless both writes are necessary, consider removing one to prevent redundant operations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- CHANGES.txt (1 hunks)
- testng-asserts/src/test/java/org/testng/AssertTest.java (1 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (1 hunks)
- testng-core/src/main/java/org/testng/reporters/FailedReporter.java (7 hunks)
- testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java (3 hunks)
- testng-core/src/test/java/test/invocationcount/issue3180/RetryAnalyzer.java (1 hunks)
- testng-core/src/test/java/test/invocationcount/issue3180/SampleTestContainer.java (1 hunks)
🔇 Additional comments (8)
testng-core/src/test/java/test/invocationcount/issue3180/RetryAnalyzer.java (1)
1-5: LGTM: Package declaration and imports are correct.The package declaration and imports are appropriate for the implementation of the RetryAnalyzer.
testng-asserts/src/test/java/org/testng/AssertTest.java (1)
585-602: Summary: Improved deep equality checks for setsThe changes in both
testAssertEqualsDeepSetandtestAssertEqualsDeepSetFailmethods address the issue described in GITHUB-3140 by usingLinkedHashSetinstead ofHashSet. This ensures consistent ordering during deep equality checks, improving the reliability of these tests.These changes effectively solve the problem while maintaining the overall structure and purpose of the tests. The use of
LinkedHashSetis an appropriate solution for ensuring consistent behavior in deep equality assertions involving sets.CHANGES.txt (2)
Line range hint
1-2802: Conclusion of CHANGES.txt reviewAfter reviewing the CHANGES.txt file, it's evident that TestNG is a mature and actively maintained project with a strong focus on continuous improvement. The changelog demonstrates:
- Regular releases with bug fixes and feature enhancements
- Responsiveness to user-reported issues (via GitHub)
- Ongoing efforts to improve core functionality such as parallel execution, reporting, and test configuration
- Adaptation to new Java versions and deprecation of older features
- Attention to performance and reliability
The project seems to be in a healthy state, with a good balance between fixing bugs, adding new features, and refining existing functionality. The frequent mentions of GitHub issues indicate an active community and good engagement with users.
However, there are a few areas that might benefit from additional attention:
- Some long-standing issues (like certain aspects of parallel execution and data provider handling) seem to require frequent tweaks. These might benefit from a more comprehensive review or refactoring.
- The frequency of changes related to reporting suggests that this area might need a more extensible architecture to accommodate future enhancements more easily.
- As the project continues to evolve, maintaining backward compatibility while introducing new features appears to be an ongoing challenge that requires careful management.
Overall, the CHANGES.txt file paints a picture of a robust, actively developed testing framework that is responsive to user needs and evolving alongside the Java ecosystem. Future development efforts might focus on addressing recurring themes in the changelog to create a more stable and maintainable codebase.
Line range hint
2-7: Review of changes in version 7.11.0The changes in version 7.11.0 appear to be primarily bug fixes and minor enhancements. Here's a summary of the key points:
- Fixed an issue with 'invocation-numbers' calculation in testng-failed.xml (GITHUB-3180).
- Resolved a problem where specifying dataProvider and successPercentage caused tests to always pass (GITHUB-3170).
- Fixed an execution stall when using "use-global-thread-pool" (GITHUB-3028).
- Updated JCommander to version 1.83 (GITHUB-3122).
- Improved failure messages for array comparisons in assertEquals (GITHUB-3135).
- Fixed a deep comparison issue with Sets in assertEqualsDeep (GITHUB-3140).
These changes seem to address important issues and improve the overall functionality and reliability of TestNG. However, there are a few points that might benefit from further clarification or attention:
- For GITHUB-3180, it would be helpful to have more details on how the 'invocation-numbers' calculation was fixed and what the expected behavior is now.
- The fix for GITHUB-3170 (dataProvider and successPercentage interaction) might need additional testing to ensure it doesn't introduce any new edge cases.
- The execution stall fix (GITHUB-3028) could potentially impact performance in certain scenarios. It might be worth monitoring and gathering feedback from users on this change.
To verify the impact of these changes, especially the fix for the execution stall, we could run a performance test. Here's a script to check for any significant changes in execution time:
This script will help identify any potential performance regressions or improvements resulting from the changes in version 7.11.0.
testng-core/src/test/java/test/invocationcount/issue3180/SampleTestContainer.java (1)
1-106: LGTM!The newly added test classes are well-structured and effectively cover various scenarios involving retries and data providers. This comprehensive test suite aligns with the PR objectives to address the calculation of 'invocation-numbers' in the
testng-failed.xmlfile when using retries and data providers. The implementation is correct and follows TestNG best practices.testng-core/src/main/java/org/testng/reporters/FailedReporter.java (2)
95-97: Ensure Thread Safety with 'keyCache' AccessThe
keyCachemap is a shared resource accessed concurrently (lines 95-97). WhileConcurrentHashMapis thread-safe for individual operations, compound operations may still lead to race conditions. Ensure that all usages ofkeyCache, especiallycomputeIfAbsentin theisFlakyTestmethod, are safe in the context of concurrent execution.
142-146: Verify Logic for Identifying Flaky TestsThe logic at lines 144-145 skips tests that are repetitive or data-driven and have already passed (
isFlakyTest). Ensure that this correctly identifies flaky tests and doesn't inadvertently skip tests that should be rerun.testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (1)
767-769: LGTM: Correctly handle failed invocation numbers when retries are disabledThe added condition ensures that failed invocation numbers are only recorded when the method will not be retried. This change aligns with the PR objective to streamline invocation numbers in the
testng-failed.xmlfile and addresses the issue where incorrectinvocation-numberswere reported for methods using retries and data providers.
testng-core/src/test/java/test/invocationcount/issue3180/RetryAnalyzer.java
Show resolved
Hide resolved
testng-core/src/test/java/test/invocationcount/issue3180/RetryAnalyzer.java
Show resolved
Hide resolved
testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java
Show resolved
Hide resolved
testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java
Show resolved
Hide resolved
testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java
Show resolved
Hide resolved
testng-core/src/main/java/org/testng/reporters/FailedReporter.java
Outdated
Show resolved
Hide resolved
testng-core/src/main/java/org/testng/reporters/FailedReporter.java
Outdated
Show resolved
Hide resolved
testng-core/src/main/java/org/testng/reporters/FailedReporter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java (3)
119-147: LGTM: Comprehensive data provider for invocation count scenarios.The
getTestDatadata provider covers a wide range of scenarios, including flaky tests and various invocation count cases, which aligns well with the PR objectives. This will help ensure that the fix for issue #3180 is robust.Consider adding a brief comment for each test case to explain its purpose, which would improve readability and maintainability. For example:
// Test case 1: Flaky test that eventually passes {SampleTestContainer.TestContainsFlakyDataDrivenTest.class, noXml, nothing}, // Test case 2: Repetitive test with success percentage {SampleTestContainer.TestContainsPercentageDrivenTest.class, noXml, nothing}, // ... (comments for other test cases)
149-181: LGTM: Well-implemented test method for invocation count with retries and data providers.This method effectively tests the invocation count behavior with retries and data providers, addressing the core of issue #3180. The implementation is thorough, checking both the existence of the XML file and its content.
Consider extracting the XML parsing and validation logic into a separate private method to improve readability and reusability. For example:
private void validateXmlContent(Path xml, String[] invocationCountValue) throws Exception { 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++) { // ... (existing validation logic) } softly.assertAll(); }Then, you can call this method from
ensureInvocationCountHonoursRetriesWhenUsingDataProviders:if (isXmlGenerated) { validateXmlContent(xml, invocationCountValue); }This would make the main test method more concise and easier to read.
Line range hint
1-258: Overall, excellent implementation addressing issue #3180.The changes in this file effectively address the problem of invocation count calculation when using retries and data providers. The new test methods and data providers cover a comprehensive range of scenarios, including flaky tests and multiple test tags, which should thoroughly validate the fix for issue #3180.
Key strengths of this implementation:
- Comprehensive test scenarios covering various edge cases.
- Clear separation of concerns between data providers and test methods.
- Effective use of TestNG's XML suite capabilities for testing multiple test tags.
While there are opportunities for refactoring to reduce code duplication (as mentioned in previous comments), the current implementation is functional, clear, and achieves the objectives of the PR.
As the test suite grows, consider implementing a more modular approach to test case generation and validation. This could involve creating separate classes for test data generation, XML validation, and test execution, which would improve maintainability and scalability of the test suite in the long run.
testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (1)
Line range hint
1022-1108: Improved parameter handling and error management in MethodInvocationAgentThe changes in the
MethodInvocationAgentclass, particularly in theinvokemethod, enhance the handling of parameters and error cases. This improvement contributes to more accurate reporting of invocation numbers and aligns with the PR objectives.Key improvements:
- Use of
ParameterHandlerfor creating parameters, which centralizes parameter handling logic.- Explicit handling of errors during parameter creation, including proper status setting and notification.
- Clear separation of parallel and sequential test execution paths.
These changes should help address the issues with 'invocation-numbers' in the
testng-failed.xmlfile, especially for methods using data providers and retries.Consider adding a comment explaining the significance of the
bubbleUpFailuresflag, as it affects how certain failures are propagated:boolean bubbleUpFailures = m_configuration.isPropagateDataProviderFailureAsTestFailure() || bag.isBubbleUpFailures(); +// Determine if data provider failures should be treated as test failures
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- CHANGES.txt (1 hunks)
- testng-asserts/src/test/java/org/testng/AssertTest.java (1 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (1 hunks)
- testng-core/src/main/java/org/testng/reporters/FailedReporter.java (4 hunks)
- testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java (3 hunks)
- testng-core/src/test/java/test/invocationcount/issue3180/RetryAnalyzer.java (1 hunks)
- testng-core/src/test/java/test/invocationcount/issue3180/SampleTestContainer.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- CHANGES.txt
- testng-asserts/src/test/java/org/testng/AssertTest.java
- testng-core/src/main/java/org/testng/reporters/FailedReporter.java
- testng-core/src/test/java/test/invocationcount/issue3180/RetryAnalyzer.java
- testng-core/src/test/java/test/invocationcount/issue3180/SampleTestContainer.java
🧰 Additional context used
📓 Learnings (1)
testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java (1)
Learnt from: krmahadevan PR: testng-team/testng#3181 File: testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java:169-174 Timestamp: 2024-09-30T05:22:19.902Z Learning: When using `SoftAssertions` in tests, apply soft assertions within loops, but use standard assertions outside of loops.
🔇 Additional comments (4)
testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java (2)
5-15: LGTM: New imports are appropriate for the added functionality.The added imports for XML processing, file handling, and assertions are necessary and relevant to the new methods implementing the fix for issue #3180.
Also applies to: 24-30, 35-35
254-258: LGTM: Simple XML parsing method.The
documentmethod effectively parses an XML string into a Document object, which is suitable for the test code context.As per the previous discussion, it was decided that the current implementation is acceptable for test code. However, it's worth noting that in a production environment, we would typically want to implement security measures against XML External Entity (XXE) attacks. If this code ever moves to a non-test context, please remember to update the XML parsing logic accordingly.
testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (2)
Line range hint
1-1108: Summary of changes addressing PR objectivesThe changes in this file effectively address the reported issues with 'invocation-numbers' in the
testng-failed.xmlfile, specifically for methods using data providers and retries. The key improvements are:
- In the
invokeMethodmethod, failed invocation numbers are only added when the method will not be retried, ensuring accurate reflection of 'invocation-numbers' in thetestng-failed.xmlfile.- The
MethodInvocationAgentclass has been enhanced to improve parameter handling and error management, which contributes to more accurate reporting of invocation numbers.These changes align well with the PR objectives and should resolve the issues described in the linked issue #3180.
To ensure the changes work as expected, consider adding or updating unit tests that specifically target the scenario described in the issue, using data providers and retries to verify the correct 'invocation-numbers' are generated in the
testng-failed.xmlfile.
767-769: Improvement in handling failed invocations for data-driven testsThis change addresses the issue with 'invocation-numbers' in the
testng-failed.xmlfile. By only adding the failed invocation number when the method will not be retried, it ensures that thetestng-failed.xmlfile accurately reflects the correct 'invocation-numbers' for methods that utilize data providers and retries.To ensure this change doesn't introduce any regressions, please run the following verification:
testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java
Show resolved
Hide resolved
testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java
Show resolved
Hide resolved
Closes #3180
Fixes #3180 .
Did you remember to?
CHANGES.txt./gradlew autostyleApplyWe encourage pull requests that:
If your pull request involves fixing SonarQube issues then we would suggest that you please discuss this with the
TestNG-dev before you spend time working on it.
Note: For more information on contribution guidelines please make sure you refer our Contributing section for detailed set of steps.
Summary by CodeRabbit
New Features
RetryAnalyzerclass for enhanced retry mechanisms in tests.SampleTestContainerto demonstrate retry and data-driven testing scenarios.Bug Fixes
Enhancements
Documentation