Map object to configurations #3089
Conversation
WalkthroughThe TestNG updates encompass a range of improvements and fixes aimed at enhancing test method listener behaviors, documentation accuracy, unique ID assignments, synchronization enhancements, Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (14)
- CHANGES.txt (1 hunks)
- testng-core/src/main/java/org/testng/ClassMethodMap.java (1 hunks)
- testng-core/src/main/java/org/testng/TestClass.java (5 hunks)
- testng-core/src/main/java/org/testng/TestRunner.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/ITestClassConfigInfo.java (3 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/ConfigInvoker.java (1 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (1 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/TestNgMethodUtils.java (2 hunks)
- testng-core/src/test/java/test/listeners/ListenersTest.java (3 hunks)
- testng-core/src/test/java/test/listeners/issue3082/IUniqueObject.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue3082/ObjectRepository.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue3082/ObjectTrackingMethodListener.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue3082/TestClassSample.java (1 hunks)
Files skipped from review due to trivial changes (1)
- testng-core/src/test/java/test/listeners/issue3082/IUniqueObject.java
Additional comments: 20
testng-core/src/test/java/test/listeners/issue3082/ObjectTrackingMethodListener.java (1)
- 1-16: The implementation of
ObjectTrackingMethodListenercorrectly tracks method invocations for objects implementingIUniqueObject. Good use of type checking and repository usage.testng-core/src/test/java/test/listeners/issue3082/ObjectRepository.java (1)
- 1-39: The implementation of
ObjectRepositoryuses concurrent data structures effectively, ensuring thread safety. Methods for managing invocations and errors are correctly implemented.testng-core/src/main/java/org/testng/internal/ITestClassConfigInfo.java (1)
- 29-50: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-49]
The enhancements to
ITestClassConfigInfowith methods for after class configurations and static utility methods for fetching these configurations fromITestClassinstances are well-designed, improving the interface's functionality while maintaining backward compatibility.testng-core/src/test/java/test/listeners/issue3082/TestClassSample.java (1)
- 1-68: The
TestClassSampleis well-implemented, correctly using TestNG features like factory methods, data providers, and before/after annotations. The custom tracking of method invocations usingObjectRepositoryis logically sound and supports the test's objectives.testng-core/src/main/java/org/testng/ClassMethodMap.java (1)
- 35-35: Refactoring to use
computeIfAbsentfor adding methods to the map inClassMethodMapis an excellent improvement, enhancing efficiency, readability, and thread safety.testng-core/src/main/java/org/testng/internal/invokers/TestNgMethodUtils.java (1)
- 71-101: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [5-260]
The enhancements in
TestNgMethodUtilswith the introduction ofOptional,Predicate, and thesameInstancepredicate, along with the modifications to method signatures, significantly improve the handling of instance-specific configurations. These changes enhance the code's functionality, readability, and correctness.testng-core/src/main/java/org/testng/TestClass.java (1)
- 235-263: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [7-260]
The updates to
TestClass, including the addition ofafterClassConfigand related methods, as well as the use ofStreamfor method processing, significantly improve the handling of after class configurations. These changes follow modern Java practices and enhance the class's functionality, efficiency, and readability.testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java (1)
- 134-134: The modifications in
TestMethodWorkerto ensure correct instance management during method execution and configuration method handling are well-implemented. These changes address the issue of incorrect instance management and improve the code's correctness and functionality.Also applies to: 235-236
testng-core/src/main/java/org/testng/internal/invokers/ConfigInvoker.java (1)
- 239-239: Adding a
nullargument beforearguments.getTestClass()in thefilterMethodscall could potentially introduce nullability issues or affect the method's behavior in unexpected ways. Please verify the necessity of this change and consider documenting its purpose or refactoring to avoid passingnulldirectly if possible.testng-core/src/test/java/test/listeners/ListenersTest.java (3)
- 14-14: The addition of
SoftAssertionsfrom AssertJ is a good choice for making multiple assertions in a single test without stopping at the first failure. This approach enhances the test's ability to report multiple issues at once, which is particularly useful in complex testing scenarios like the one being addressed.- 52-53: The imports for
ObjectRepositoryandObjectTrackingMethodListenerare crucial for the new test method to function. These classes seem to be part of the solution to track object instances correctly and validate the fix. Ensuring these are correctly imported and utilized is key to the test's success.- 527-543: The new test method
ensureListenerWorksWithCorrectTestClassInstanceis well-structured and follows best practices for testing. It sets up aTestNGinstance with specific configurations, adds theObjectTrackingMethodListener, and asserts the state ofObjectRepositoryusingSoftAssertions. This method effectively validates the changes made to address the issue of incorrect instance returns inIInvokedMethodListener.However, it's important to ensure that the
ObjectRepositoryandObjectTrackingMethodListenerare implemented in a way that accurately tracks and reports instances. The test assumes that theObjectRepositorycorrectly tracks instances and errors, and that theObjectTrackingMethodListeneraccurately captures method invocations.Given the context of the PR, this test method is a critical piece of the validation process. It's designed to ensure that the changes made to TestNG correctly handle instances in parallel testing scenarios, which is the core issue the PR aims to resolve.
testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (2)
- 186-186: The addition of the
instanceparameter tofilterBeforeTestMethodsis aligned with the PR's objective to ensure correct instance handling during method filtering. This change is crucial for achieving accurate instance management in parallel testing scenarios.- 188-188: Similarly, the addition of the
instanceparameter tofilterAfterTestMethodssupports the goal of accurate instance management. This modification is essential for maintaining consistency in instance handling across different phases of testing.testng-core/src/main/java/org/testng/TestRunner.java (2)
- 489-495: The modifications to the
fixMethodsWithClasscalls to use the newclassLevelConfigsmethod for retrieving before and after class methods are a significant change. This approach centralizes the logic for obtaining class-level configuration methods, which can improve maintainability and readability. However, it's crucial to ensure that this change does not inadvertently alter the behavior of test initialization, especially in complex scenarios involving inheritance and overrides.Ensure comprehensive testing around these changes, particularly in scenarios involving inheritance and overrides of configuration methods, to verify that the behavior remains consistent with previous versions.
- 559-565: The introduction of the
classLevelConfigsmethod is a good practice as it encapsulates the logic for retrieving before and after class methods into a single, reusable method. This change enhances the code's readability and maintainability by reducing duplication and centralizing the logic.Consider adding detailed Javadoc comments to the
classLevelConfigsmethod, explaining its purpose, parameters, and the significance of thebeforeConfigflag. This documentation will help future maintainers and contributors understand the method's role in the test initialization process.CHANGES.txt (4)
- 2-2: The entry for the current version (7.10.0) is present, ensuring the document is up-to-date with the latest changes.
- 2-5: The summary of changes for version 7.10.0 is well-detailed, covering various fixes and new features, which helps in understanding the scope of the update.
- 2-5: Ensure consistency in the formatting of the entries across different versions. Each entry should follow a similar structure to maintain readability and ease of understanding for the users.
- 2-5: Check for any typographical or grammatical issues in the descriptions of the changes to ensure clarity and professionalism in the documentation.
testng-core/src/main/java/org/testng/internal/ITestClassConfigInfo.java
Outdated
Show resolved
Hide resolved
testng-core/src/main/java/org/testng/internal/invokers/TestNgMethodUtils.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- testng-core/src/main/java/org/testng/TestClass.java (5 hunks)
- testng-core/src/main/java/org/testng/internal/ConfigurationMethod.java (7 hunks)
- testng-core/src/main/java/org/testng/internal/ITestClassConfigInfo.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/NoOpTestClass.java (5 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/TestNgMethodUtils.java (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- testng-core/src/main/java/org/testng/TestClass.java
- testng-core/src/main/java/org/testng/internal/ITestClassConfigInfo.java
- testng-core/src/main/java/org/testng/internal/invokers/TestNgMethodUtils.java
Additional comments: 8
testng-core/src/main/java/org/testng/internal/NoOpTestClass.java (5)
- 15-25: The replacement of arrays with
Listfor method containers (m_beforeClassMethods,m_beforeTestMethods, etc.) is a positive change that enhances flexibility and potentially improves performance. However, ensure that all usages of these fields throughout the codebase have been updated to work withListinstead of arrays to avoid runtime errors.- 42-51: When initializing
Listfields in the constructor withList.of(), ensure that the methods being passed (testClass.getBeforeSuiteMethods(),testClass.getBeforeTestConfigurationMethods(), etc.) return non-null values.List.of()does not accept null elements. If there's a possibility of null values, consider using a null-safe approach or filtering nulls before creating the list.- 59-63: The methods
setBeforeTestMethodsandsetAfterTestMethodnow accept arrays and convert them toListusingList.of(). This is a good use ofList.of()for immutable list creation. However, ensure that callers of these methods are aware of the change and that the passed arrays are not expected to be modified after being passed to these methods, as the resultingListwill be immutable.- 39-90: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [69-113]
The conversion of method containers to
Listand then back to arrays usingtoArray(ITestNGMethod[]::new)in getter methods is correctly implemented. This ensures compatibility with existing interfaces while leveraging the benefits ofList. Just ensure that the performance impact of these conversions is minimal, especially in contexts where these methods are called frequently.
- 125-125: The conversion of
m_afterGroupsMethodsto an array ingetAfterGroupsMethodsis consistent with the changes made to other method containers. This maintains the consistency of the API while internally usingListfor better flexibility.testng-core/src/main/java/org/testng/internal/ConfigurationMethod.java (3)
- 144-150: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [147-192]
The method
createMethodscorrectly replaces arrays withList<ITestNGMethod>for creating configuration methods. This change enhances flexibility and code readability. Ensure that all callers of this method have been updated to handleListinstead of arrays.
- 189-198: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [195-218]
The methods
createSuiteConfigurationMethods,createTestConfigurationMethods, and others correctly utilize thecreateMethodsutility to generate lists of configuration methods. This maintains consistency in how configuration methods are created and stored, leveraging the benefits of usingList.
- 292-298: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [295-322]
The use of
Arrays.stream()followed by.parallel()increateAfterConfigurationMethodsis a good practice for parallel processing of arrays. However, ensure that the operations performed within the stream are thread-safe and that parallel processing does not introduce any unintended side effects, especially sinceConfigurationMethodinstances are being created and modified.
testng-core/src/main/java/org/testng/internal/NoOpTestClass.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- testng-core/src/main/java/org/testng/TestRunner.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/ITestClassConfigInfo.java (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- testng-core/src/main/java/org/testng/TestRunner.java
- testng-core/src/main/java/org/testng/internal/ITestClassConfigInfo.java
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- testng-core/src/main/java/org/testng/TestRunner.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/ITestClassConfigInfo.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/NoOpTestClass.java (5 hunks)
Files skipped from review as they are similar to previous changes (3)
- testng-core/src/main/java/org/testng/TestRunner.java
- testng-core/src/main/java/org/testng/internal/ITestClassConfigInfo.java
- testng-core/src/main/java/org/testng/internal/NoOpTestClass.java
| fixMethodsWithClass(tc.getBeforeTestMethods(), tc, null); | ||
| fixMethodsWithClass(tc.getAfterTestMethods(), tc, null); | ||
| fixMethodsWithClass(tc.getAfterClassMethods(), tc, afterClassMethods); | ||
| fixMethodsWithClass(afterClassConfigMethods(tc), tc, afterClassMethods); |
There was a problem hiding this comment.
Sorry, I didn't catch it before but why not have a fixMethodsWithClass(Array) that will call fixMethodsWithClass(List)? That will be a good start for cleaning without the need to change everything.
There was a problem hiding this comment.
The change to list has been made and pushed already. Lets go ahead with that.
I will raise a separate PR, that will deprecate all the user facing methods on ITestNGMethod that return an array and have it replaced with a List.
This test has recently started failing after the PR testng-team#3089 got merged. With the below exception org.ops4j.pax.exam.TestContainerException: java.lang.IllegalArgumentException: Can not set java.util.List field org.testng.internal.NoOpTestClass.m_beforeTestMethods to [Lorg.testng.ITestNGMethod; This PR changes the type of a field Which was previously an array to a list. The PaxExam listener is making Use of reflection to access the data member “m_beforeTestMethods” instead of it just using The “setter” that is part of NoOpTestClass. Since this library is being used ONLY for test and Since the library in question seems to be something That is not updated for quite sometime, resorting to class path overriding wherein we duplicate the Same class into TestNG codebase and then fix the Problem locally so that the tests will pass.
Closes #3082
Fixes #3082 .
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
computeIfAbsent.ReentrantLockfor improved concurrency handling.