Add CacheTest for dataprovider issue 3236 #3237
Conversation
Introduces a new test class to verify dataprovider behavior with retries and caching disabled. The test ensures unique data per retry and validates invocation count.
WalkthroughA new test class, Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant CacheTest
participant DataProvider
participant MyRetry
TestRunner->>CacheTest: Run testMethod (with DataProvider)
CacheTest->>DataProvider: getData()
DataProvider-->>CacheTest: New UUID (no cache on retry)
CacheTest->>CacheTest: Assert UUID, increment counters
alt Assertion fails & retry allowed
CacheTest->>MyRetry: retry()
MyRetry-->>TestRunner: true (allow retry)
TestRunner->>CacheTest: Retry testMethod (new UUID)
else Assertion passes or retries exhausted
MyRetry-->>TestRunner: false (stop retry)
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 0
🧹 Nitpick comments (2)
testng-core/src/test/java/test/dataprovider/issue3236/CacheTest.java (2)
14-15: Consider test isolation and thread safety concerns.The static
invocationCountand instance fieldcurrentUuidcould cause issues:
- Static state persists across test runs, potentially affecting test isolation
currentUuidis not thread-safe, though this may be acceptable for sequential test executionConsider using TestNG's
@BeforeMethodto reset state or making the test more self-contained:public class CacheTest { - public static final AtomicInteger invocationCount = new AtomicInteger(0); + private final AtomicInteger invocationCount = new AtomicInteger(0); private String currentUuid;
17-23: Test logic is correct but could be clearer.The test successfully validates that the data provider is called on retry when
cacheDataForTestRetries = false, but the logic is somewhat convoluted with the hardcoded invocation count check.Consider adding a comment to explain the test flow:
@Test(dataProvider = "dp", retryAnalyzer = MyRetry.class) public void testMethod(String uuid) { + // Verify that fresh UUID is generated on each retry (no caching) assertEquals(currentUuid, uuid); + // Fail on first invocation to trigger retry, pass on second if (invocationCount.get() != 2) { throw new RuntimeException("Failed for " + uuid); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
testng-core/src/test/java/test/dataprovider/issue3236/CacheTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: krmahadevan
PR: testng-team/testng#3181
File: testng-core/src/test/java/test/invocationcount/issue3180/RetryAnalyzer.java:6-9
Timestamp: 2024-09-30T05:21:21.304Z
Learning: In test code for the TestNG project, it's acceptable to hardcode parameters like `retryLimit` since they don't need to be configurable.
Learnt from: krmahadevan
PR: testng-team/testng#3181
File: testng-core/src/test/java/test/invocationcount/issue3180/RetryAnalyzer.java:6-9
Timestamp: 2024-10-09T09:04:04.750Z
Learning: In test code for the TestNG project, it's acceptable to hardcode parameters like `retryLimit` since they don't need to be configurable.
Learnt from: juherr
PR: testng-team/testng#3171
File: testng-core/src/test/java/test/invocationcount/issue1719/TestclassSample.java:24-45
Timestamp: 2024-09-05T07:53:27.213Z
Learning: When suggesting improvements to test code, encapsulating complex or repetitive logic into separate methods can enhance readability and maintainability. Additionally, logging important messages before assertions can improve the debugging experience by ensuring that the details are captured in the test output.
Learnt from: juherr
PR: testng-team/testng#3171
File: testng-core/src/test/java/test/invocationcount/issue1719/TestclassSample.java:24-45
Timestamp: 2024-10-09T09:04:04.750Z
Learning: When suggesting improvements to test code, encapsulating complex or repetitive logic into separate methods can enhance readability and maintainability. Additionally, logging important messages before assertions can improve the debugging experience by ensuring that the details are captured in the test output.
testng-core/src/test/java/test/dataprovider/issue3236/CacheTest.java (4)
Learnt from: krmahadevan
PR: testng-team/testng#3181
File: testng-core/src/test/java/test/invocationcount/issue3180/RetryAnalyzer.java:6-9
Timestamp: 2024-09-30T05:21:21.304Z
Learning: In test code for the TestNG project, it's acceptable to hardcode parameters like `retryLimit` since they don't need to be configurable.
Learnt from: krmahadevan
PR: testng-team/testng#3181
File: testng-core/src/test/java/test/invocationcount/issue3180/RetryAnalyzer.java:6-9
Timestamp: 2024-10-09T09:04:04.750Z
Learning: In test code for the TestNG project, it's acceptable to hardcode parameters like `retryLimit` since they don't need to be configurable.
Learnt from: krmahadevan
PR: testng-team/testng#3181
File: testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java:183-215
Timestamp: 2024-10-01T05:35:26.176Z
Learning: Prefer keeping test data methods duplicated for better understanding and readability, even if it introduces duplication.
Learnt from: krmahadevan
PR: testng-team/testng#3181
File: testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java:183-215
Timestamp: 2024-10-18T04:03:57.942Z
Learning: Prefer keeping test data methods duplicated for better understanding and readability, even if it introduces duplication.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: 21, oracle, ubuntu, Pacific/Chatham, fr_FR, stress JIT
- GitHub Check: 21, oracle, ubuntu, UTC, de_DE
- GitHub Check: 11, temurin, same hashcode, macos, Pacific/Chatham, fr_FR, stress JIT
- GitHub Check: 24, oracle, same hashcode, windows, America/New_York, de_DE, stress JIT
- GitHub Check: 17, temurin, windows, UTC, tr_TR
- GitHub Check: 11, corretto, windows, America/New_York, fr_FR, stress JIT
- GitHub Check: 11, liberica, windows, UTC, de_DE, stress JIT
🔇 Additional comments (2)
testng-core/src/test/java/test/dataprovider/issue3236/CacheTest.java (2)
25-30: Data provider implementation correctly tests the core functionality.The
cacheDataForTestRetries = falsesetting and UUID generation on each invocation properly tests the intended behavior.
32-40: Retry analyzer implementation is correct and follows TestNG patterns.The retry logic correctly allows one retry attempt, which aligns with the test's expectation of 2 total invocations.
Note: Based on retrieved learnings, hardcoded retry limits are acceptable in TestNG test code since they don't need to be configurable.
Updated the visibility of the invocationCount field from public static to private in CacheTest to improve encapsulation and restrict access.
Sorry? I'm afraid that I did not get You. I basically used this https://github.com/testng-team/testng/blob/823799c833aa6e6f47c91996b09dcbba3b931b17/testng-core/src/test/java/test/dataprovider/issue3041/SampleTestCase.java as a template for the reproduction of the issue I just reported. Do You want me to invoke this test from TestNG instance like this? https://github.com/testng-team/testng/blob/823799c833aa6e6f47c91996b09dcbba3b931b17/testng-core/src/test/java/test/dataprovider/issue3041/SampleTestCase.java |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
testng-core/src/test/java/test/dataprovider/DataProviderTest.java (1)
721-728: Verify the expected test behavior and consider adding more descriptive assertions.The test logic appears correct and follows the established pattern. However, to ensure the test validates the intended behavior, verify that:
- The retry mechanism correctly marks the first failed attempt as skipped
- The fresh data generation is actually being tested (though this is implicitly verified by the CacheTest implementation)
Consider adding more descriptive assertion messages to make test failures clearer:
- assertThat(tla.getFailedTests()).size().isEqualTo(0); - assertThat(tla.getSkippedTests()).size().isEqualTo(1); + assertThat(tla.getFailedTests()).withFailMessage("Expected no failed tests after retry").size().isEqualTo(0); + assertThat(tla.getSkippedTests()).withFailMessage("Expected exactly one skipped test due to retry").size().isEqualTo(1);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
testng-core/src/test/java/test/dataprovider/DataProviderTest.java(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
testng-core/src/test/java/test/dataprovider/DataProviderTest.java (4)
Learnt from: krmahadevan
PR: testng-team/testng#3181
File: testng-core/src/test/java/test/invocationcount/issue3180/RetryAnalyzer.java:6-9
Timestamp: 2024-10-09T09:04:04.750Z
Learning: In test code for the TestNG project, it's acceptable to hardcode parameters like `retryLimit` since they don't need to be configurable.
Learnt from: krmahadevan
PR: testng-team/testng#3181
File: testng-core/src/test/java/test/invocationcount/issue3180/RetryAnalyzer.java:6-9
Timestamp: 2024-09-30T05:21:21.304Z
Learning: In test code for the TestNG project, it's acceptable to hardcode parameters like `retryLimit` since they don't need to be configurable.
Learnt from: krmahadevan
PR: testng-team/testng#3181
File: testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java:183-215
Timestamp: 2024-10-18T04:03:57.942Z
Learning: Prefer keeping test data methods duplicated for better understanding and readability, even if it introduces duplication.
Learnt from: krmahadevan
PR: testng-team/testng#3181
File: testng-core/src/test/java/test/invocationcount/FailedInvocationCountTest.java:183-215
Timestamp: 2024-10-01T05:35:26.176Z
Learning: Prefer keeping test data methods duplicated for better understanding and readability, even if it introduces duplication.
🧬 Code Graph Analysis (1)
testng-core/src/test/java/test/dataprovider/DataProviderTest.java (1)
testng-core/src/test/java/test/dataprovider/issue3236/CacheTest.java (1)
CacheTest(12-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: 24, oracle, macos, America/New_York, ru_RU, stress JIT
- GitHub Check: 11, corretto, windows, America/New_York, fr_FR, stress JIT
- GitHub Check: 24, oracle, ubuntu, Pacific/Chatham, ru_RU
- GitHub Check: 24, oracle, ubuntu, America/New_York, ru_RU
- GitHub Check: 11, zulu, windows, UTC, tr_TR, stress JIT
- GitHub Check: 17, zulu, ubuntu, Pacific/Chatham, de_DE, stress JIT
- GitHub Check: 21, microsoft, same hashcode, macos, Pacific/Chatham, fr_FR, stress JIT
🔇 Additional comments (1)
testng-core/src/test/java/test/dataprovider/DataProviderTest.java (1)
64-64: LGTM!The import statement correctly references the new test class and follows the established pattern.
testng-core/src/test/java/test/dataprovider/DataProviderTest.java
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Done, I suppose |
Introduces a new test class to verify dataprovider behavior with retries and caching disabled. The test ensures unique data per retry and validates invocation count.
Fixes # .
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