Fix for #3189 - fixed ignored testcases count #3190
Conversation
WalkthroughThe pull request introduces various bug fixes and enhancements to the TestNG framework, particularly in version 7.11.0. Key changes include corrections to the calculation of ignored tests, improvements in the reporting of test results, and the addition of a new test class to demonstrate the ignore functionality. The modifications to the Changes
Assessment against linked 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
testng-core/src/test/java/test/reports/XmlReporterTest.java (1)
96-101: Consider splitting the test or updating the method name.The test method name
ensureConfigurationMethodsAreNotCountedAsSkippedInXmlReportsdoesn't fully reflect its current responsibility, as it now also verifies the correct counting of ignored tests with the addition ofSimpleIgnoredSample.class.Consider either:
- Splitting this into two separate test methods to maintain single responsibility, or
- Updating the method name to reflect both concerns being tested
Example of a more descriptive name if keeping as single test:
-public void ensureConfigurationMethodsAreNotCountedAsSkippedInXmlReports() +public void ensureCorrectCountingOfSkippedConfigurationAndIgnoredTestsInXmlReports()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
CHANGES.txt(1 hunks)testng-core/src/main/java/org/testng/reporters/AbstractXmlReporter.java(1 hunks)testng-core/src/test/java/test/reports/SimpleIgnoredSample.java(1 hunks)testng-core/src/test/java/test/reports/XmlReporterTest.java(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- testng-core/src/test/java/test/reports/SimpleIgnoredSample.java
🔇 Additional comments (1)
CHANGES.txt (1)
8-8: LGTM! The changelog entry accurately reflects the PR objectives.
The addition "Fixed: GITHUB-3189: Incorrect number of ignored tests displayed in the XML results" properly documents the fix for issue #3189 which addresses the incorrect display of ignored tests count in XML results.
| assertThat(ignored).isEqualTo(1); | ||
| assertThat(total).isEqualTo(3); |
There was a problem hiding this comment.
💡 Codebase verification
The assertions don't match the actual test implementation
The assertions expect a total of 3 tests with 1 ignored test, but SimpleIgnoredSample.java only contains a single test method that is ignored. This means the assertions assertThat(ignored).isEqualTo(1); assertThat(total).isEqualTo(3); are incorrect as they don't reflect the actual test class implementation.
- SimpleIgnoredSample.java has only one test method
ignored()which is annotated with both@Testand@Ignore - The total count should be 1 (not 3)
- The ignored count is correctly set to 1
🔗 Analysis chain
Verify the SimpleIgnoredSample test class implementation.
The assertions expect exactly one ignored test and a total of 3 tests. Let's verify that SimpleIgnoredSample.class is properly implemented with one @Ignore annotated test method.
Let me gather more information about the SimpleIgnoredSample class implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SimpleIgnoredSample class implementation
# Find and check the content of SimpleIgnoredSample class
echo "Searching for SimpleIgnoredSample class definition..."
rg -l "class SimpleIgnoredSample"
echo "Checking for @Ignore annotation usage..."
ast-grep --pattern 'class SimpleIgnoredSample {
$$$
@Ignore
$$$
}'
Length of output: 382
Script:
#!/bin/bash
# Get the content of SimpleIgnoredSample class to verify test methods
echo "Content of SimpleIgnoredSample class:"
cat testng-core/src/test/java/test/reports/SimpleIgnoredSample.java
# Count total test methods
echo -e "\nCounting test methods..."
ast-grep --pattern 'class SimpleIgnoredSample {
$$$
@Test
$$$
}'
# Count ignored test methods
echo -e "\nCounting ignored test methods..."
ast-grep --pattern 'class SimpleIgnoredSample {
$$$
@Ignore
@Test
$$$
}'
Length of output: 637
| @@ -228,7 +228,7 @@ public void add(Count count) { | |||
| this.failed += count.failed; | |||
| this.skipped += count.skipped; | |||
| this.retried += count.retried; | |||
| this.ignored += count.retried; | |||
| this.ignored += count.ignored; | |||
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for ignored tests counting in XML reports
Based on the verification results:
- The JUnitReportsTest validates ignored count in JUnit reports but not for the XML reporter
- No specific test exists for the Count class's ignored tests accumulation
- The fix needs test coverage to prevent regression
Consider adding test coverage:
- Add test case in CountTest.java to verify Count class's ignored tests accumulation
- Add test case in AbstractXmlReporterTest to verify ignored tests are correctly reported in XML
🔗 Analysis chain
LGTM! This fix correctly addresses the ignored tests counting issue.
The change fixes issue #3189 by correctly accumulating the ignored count instead of erroneously using the retried count. This ensures that the XML report accurately reflects the number of ignored tests.
Consider adding a clarifying comment to document this fix:
public void add(Count count) {
this.passed += count.passed;
this.failed += count.failed;
this.skipped += count.skipped;
this.retried += count.retried;
+ // Fixed in PR #3190: Use ignored count instead of retried count to fix incorrect reporting
this.ignored += count.ignored;
}Let's verify that test coverage exists for this fix:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test coverage of the ignored tests counting functionality
# Look for test files that verify ignored test counting
rg -l "ignored.*count|count.*ignored" --type java test/
Length of output: 103
Script:
#!/bin/bash
# Let's first find the test directory structure
fd -t d "test"
# Then search for test files related to XML reporting or Count class
rg -l "ignored.*count|count.*ignored" --type java
Length of output: 2838
Script:
#!/bin/bash
# Let's examine the JUnitReportsTest.java to see if it tests ignored count
rg -A 10 -B 10 "ignored.*count|count.*ignored" testng-core/src/test/java/test/junitreports/JUnitReportsTest.java
# Let's also check for any test files that might test the Count class directly
rg -l "class.*Count.*Test|test.*Count" --type java testng-core/src/test/
Length of output: 5981
Script:
#!/bin/bash
# Let's examine CountTest.java which might be directly testing the Count class
rg -A 10 -B 10 "class CountTest" testng-core/src/test/java/test/CountTest.java
# Also check if there are any existing tests for ignored tests in XML reports
ast-grep --pattern 'class $_ {
$$$
@Test
$$$
void $_($$) {
$$$
ignored$$$
$$$
}
$$$
}'
Length of output: 870
Fixes #3189 .
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
Bug Fixes
New Features
Tests