Minor refactoring around RunOrder enum #3106
Conversation
Renamed and re-organised some internal classes The run order based on instances was using Hash codes to determine execution order. Now that we have Unique identity aware test class object containers, we should be first trying to use that and if it’s not available only then fall Back to using object hash codes (This is for instance aware comparators)
WalkthroughThe recent updates to TestNG focus on refining method ordering and sorting mechanisms. The introduction of Changes
Possibly related 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.
Actionable comments posted: 1
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (8)
- testng-core-api/src/main/java/org/testng/internal/RuntimeBehavior.java (1 hunks)
- testng-core/src/main/java/org/testng/TestNG.java (2 hunks)
- testng-core/src/main/java/org/testng/TestRunner.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/IInstanceIdentity.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/IOrderMethods.java (1 hunks)
- testng-core/src/main/java/org/testng/internal/MethodSorting.java (1 hunks)
- testng-core/src/main/java/org/testng/reporters/FailedReporter.java (2 hunks)
- testng-core/src/test/java/test/BaseTest.java (3 hunks)
Additional Context Used
Additional comments not posted (12)
testng-core/src/main/java/org/testng/internal/IOrderMethods.java (1)
1-15: TheIOrderMethodsinterface is well-defined and documented, providing a clear contract for producing aComparator<ITestNGMethod>. This approach enhances modularity and flexibility in defining method order logic.testng-core/src/main/java/org/testng/internal/IInstanceIdentity.java (2)
3-4: The imports forArraysandObjectsare correctly added and necessary for the implementation of theisIdentityAwaremethod.
22-29: TheisIdentityAwaremethod is efficiently implemented using Java streams and lambda expressions. It correctly handles null inputs and checks if all passed objects are instances ofIInstanceIdentity, adhering to best practices.testng-core/src/main/java/org/testng/internal/MethodSorting.java (3)
1-85: TheMethodSortingenum is well-designed, encapsulating different sorting strategies for test methods in a type-safe manner. It provides a flexible and extensible way to define and apply sorting logic.
31-47: Consider refining the comparison logic in the INSTANCES strategy to ensure deterministic behavior. The use ofObject::toStringmight introduce non-deterministic sorting due to varyingtoStringimplementations. Replacing or removing this part of the comparison chain could enhance consistency.
73-83: ThebasedOnandparsemethods are correctly implemented, providing a flexible way to configure the sorting behavior based on system properties. This approach enhances the adaptability of the sorting logic.testng-core-api/src/main/java/org/testng/internal/RuntimeBehavior.java (1)
96-96: The modification to no longer provide a default value for the "testng.order" property enhances configurability for method sorting. Consider documenting this change or including it in release notes to inform users and guide them on explicitly configuring the sorting behavior if necessary.testng-core/src/test/java/test/BaseTest.java (2)
171-171: The replacement ofSystematiserwithMethodSortingin therunmethod correctly aligns with the new method sorting logic, enhancing flexibility and configurability for test method ordering.
349-349: The usage ofMethodSortingin thenewTestRunnermethod ofInternalTestRunnerFactorycorrectly implements the new sorting logic, ensuring that test methods are sorted according to the configured strategy.testng-core/src/main/java/org/testng/reporters/FailedReporter.java (1)
114-114: The replacement ofSystematiser.getComparator()withMethodSorting.basedOn()in thegenerateXmlTestmethod correctly aligns with the new method sorting logic, enhancing the accuracy and flexibility of method ordering in the context of generating test reports for failed and skipped tests.testng-core/src/main/java/org/testng/TestRunner.java (1)
220-220: The replacement ofSystematiser.getComparator()withMethodSorting.basedOn()aligns with the PR's objective to improve method execution ordering logic by leveraging unique identity-aware test class object containers. Ensure thatMethodSorting.basedOn()is implemented to prioritize unique identities over hash codes and integrates well with the existing system without introducing regressions.testng-core/src/main/java/org/testng/TestNG.java (1)
36-36: The import ofMethodSortingis correctly added, and its usage inMethodSorting.basedOn()replacesSystematiser.getComparator()as intended.Ensure the implementation of
MethodSorting.basedOn()aligns with the new method ordering logic.
| @@ -1362,7 +1362,7 @@ private SuiteRunner createSuiteRunner(XmlSuite xmlSuite) { | |||
| container, | |||
| m_classListeners.values(), | |||
| holder, | |||
| Systematiser.getComparator()); | |||
| MethodSorting.basedOn()); | |||
There was a problem hiding this comment.
Consider refactoring the createSuiteRunner method to improve readability and maintainability. Breaking it into smaller, more focused methods could enhance the code structure and make it easier to understand and modify in the future.
Renamed and re-organised some internal classes
The run order based on instances was using
Hash codes to determine execution order.
Now that we have Unique identity aware test class
object containers, we should be first trying to
use that and if it’s not available only then fall
Back to using object hash codes
(This is for instance aware comparators)
Summary by CodeRabbit