Support ordering of listeners #3072
Conversation
WalkthroughThe update enhances TestNG by allowing users to define the order of TestNG listeners using a Changes
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.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (36)
- CHANGES.txt (1 hunks)
- testng-core/src/main/java/org/testng/CommandLineArgs.java (1 hunks)
- testng-core/src/main/java/org/testng/DataProviderHolder.java (2 hunks)
- testng-core/src/main/java/org/testng/ListenerComparator.java (1 hunks)
- testng-core/src/main/java/org/testng/SuiteRunner.java (9 hunks)
- testng-core/src/main/java/org/testng/TestNG.java (4 hunks)
- testng-core/src/main/java/org/testng/TestRunner.java (7 hunks)
- testng-core/src/main/java/org/testng/internal/Configuration.java (3 hunks)
- testng-core/src/main/java/org/testng/internal/IConfiguration.java (1 hunks)
- testng-core/src/main/java/org/testng/internal/TestNGClassFinder.java (1 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/BaseInvoker.java (3 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/ConfigInvoker.java (4 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/IConfigInvoker.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (3 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java (3 hunks)
- testng-core/src/test/java/org/testng/internal/TestListenerHelperTest.java (1 hunks)
- testng-core/src/test/java/org/testng/internal/invokers/ParameterHandlerTest.java (2 hunks)
- testng-core/src/test/java/test/listeners/ListenersTest.java (3 hunks)
- testng-core/src/test/java/test/listeners/issue2916/AlterSuiteListenerHolder.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2916/AnnotatedTestCaseSamplesHolder.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2916/AnnotationBackedListenerComparator.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2916/ClassListenerHolder.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2916/ConfigurationListenerHolder.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2916/DataProviderInterceptorHolder.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2916/DataProviderListenerHolder.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2916/DataProviderSampleTestCase.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2916/ElaborateSampleTestCase.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2916/ExecutionListenerHolder.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2916/ExecutionVisualiserHolder.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2916/InvokedMethodListenerHolder.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2916/MethodInterceptorHolder.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2916/NormalSampleTestCase.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2916/RunOrder.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2916/SimpleConfigTestCase.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2916/SuiteListenerHolder.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2916/TestListenerHolder.java (1 hunks)
Additional comments: 39
testng-core/src/test/java/test/listeners/issue2916/RunOrder.java (1)
- 1-12: The
RunOrderannotation is correctly defined and follows Java annotation best practices. It's a clean and effective way to specify the execution order of listeners in TestNG.testng-core/src/test/java/test/listeners/issue2916/SimpleConfigTestCase.java (1)
- 1-23: The
SimpleConfigTestCaseclass is well-structured, using TestNG configuration annotations correctly. The intentional failure inbeforeMethodserves as a valid test case to verify TestNG's handling of configuration method failures.testng-core/src/test/java/test/listeners/issue2916/DataProviderSampleTestCase.java (1)
- 1-22: The
DataProviderSampleTestCaseclass correctly implements data-driven testing patterns in TestNG. The inclusion of a failing data provider is a strategic choice to test the framework's error handling capabilities.testng-core/src/main/java/org/testng/internal/invokers/IConfigInvoker.java (1)
- 21-26: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [5-25]
The addition of the
getConfiguration()method to theIConfigInvokerinterface is a logical enhancement, allowing for more flexible and informed configuration invocations. The change is correctly implemented and aligns with the interface's responsibilities.testng-core/src/test/java/test/listeners/issue2916/ElaborateSampleTestCase.java (1)
- 1-30: The
ElaborateSampleTestCaseclass provides a comprehensive set of test scenarios, including failing, skipping, flaky, and timing-out tests. Each test method is correctly implemented following TestNG conventions, offering valuable test cases for verifying the framework's functionality.testng-core/src/main/java/org/testng/internal/Configuration.java (1)
- 79-94: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [41-90]
The introduction of the
ListenerComparatorfield inConfiguration, along with its setter and getter methods (setListenerComparatorandgetListenerComparator), is a crucial part of enabling the listener ordering feature. This change allows users to configure the order of listeners programmatically, which is central to the PR's objectives.The implementation of the listener comparator configuration in
Configurationis correctly done and aligns with the PR's objectives. It's important to ensure that this new configuration option is well-documented and tested to confirm its functionality and impact on listener ordering.testng-core/src/test/java/test/listeners/issue2916/InvokedMethodListenerHolder.java (6)
- 15-88: The array
EXPECTED_LOGSis well-defined and matches the expected order of listener invocation based on the@RunOrderannotations. This is crucial for validating the functionality of listener ordering.- 91-95: The definition of
ALLas a static final list of listeners is appropriate for testing purposes. It correctly includes all defined listener classes.- 97-100: The definition of
SUBSETas a static final list of a subset of listeners is appropriate and useful for specific test scenarios.- 102-103: The
ALL_STRINGlist correctly represents the class names of all listeners. This is useful for assertions or logging purposes.- 105-122: The abstract class
KungFuWarriorcorrectly implements theIInvokedMethodListenerinterface, providing a template for listener classes. The implementation ofbeforeInvocationandafterInvocationmethods is correct, appending logs as expected.- 125-131: The use of
@RunOrderannotations onMasterOogwayandMasterShifuclasses to define execution order is a good practice for testing listener ordering functionality.testng-core/src/main/java/org/testng/CommandLineArgs.java (1)
- 57-63: The addition of the
listenerComparatorcommand-line argument is correctly implemented and follows the existing code style and structure. The description provided is clear and informative, explaining the purpose of the argument.testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java (2)
- 169-173: The implementation for sorting listeners based on a
ListenerComparatorbefore invoking@BeforeClassmethods is correctly done. It ensures that listeners are processed in the specified order, which is crucial for the new feature.- 240-244: Similarly, the implementation for sorting listeners before invoking
@AfterClassmethods is correct and follows the same pattern as the@BeforeClassmethods. This consistency is important for the feature's reliability.testng-core/src/main/java/org/testng/internal/TestNGClassFinder.java (1)
- 147-151: The addition of the
configurationparameter to theDataProviderHolderconstructor call within theprocessMethodfunction correctly aligns with the PR's objective to enhance listener ordering functionality. This change ensures that theDataProviderHoldercan utilize the configuration, potentially including the newListenerComparator, to manage data providers in a way that respects the specified listener order.testng-core/src/main/java/org/testng/SuiteRunner.java (5)
- 44-44: Making the
xmlSuite,tmpRunnerFactory,holder,configuration,invokedMethodListeners, andexitCodeListenerfields final in theSuiteRunnerclass is a good practice. It enhances the immutability of the class, ensuring that these critical components, which are foundational to the suite's execution, are not inadvertently modified after initialization.Also applies to: 50-51, 60-60, 66-66, 72-72
- 239-243: The modifications to the
invokeListenersmethod to sort listeners based on a comparator before invoking them align with the PR's objective to allow ordering of listeners. This change is crucial for ensuring that listeners are executed in the specified order, which can be critical for certain testing scenarios where the order of listener execution affects the outcome.Also applies to: 245-249
- 278-279: The addition of the
configurationparameter to theProxyTestRunnerFactoryconstructor is necessary to pass the configuration, including the new listener ordering feature, to the proxy factory. This ensures that the proxy factory can createTestRunnerinstances with the correct configuration, maintaining consistency across the framework.- 608-608: Initializing
DataProviderHolderwith theconfigurationparameter in theDefaultTestRunnerFactory'snewTestRunnermethod is consistent with the changes made elsewhere in the PR. This ensures that data provider handling within each test runner respects the global configuration, including listener ordering.- 690-690: Similarly, initializing
DataProviderHolderwith theconfigurationparameter in theProxyTestRunnerFactory'snewTestRunnermethod ensures that the proxy factory's test runners are created with the correct configuration. This change is necessary for consistency and to support the new listener ordering feature throughout the framework.testng-core/src/main/java/org/testng/internal/invokers/ConfigInvoker.java (4)
- 11-11: The addition of the
Listimport is appropriate for supporting the changes made in therunConfigurationListenersmethod where aListofIConfigurationListeneris used.- 25-25: The import of
ListenerComparatoris necessary for the newly introduced sorting functionality of listeners, aligning with the PR's objective to allow ordering of listeners.- 30-30: The import of
org.testng.collections.Listsis used to leverage theLists.newArrayListmethod, which is a part of the changes to create a mutable list from the configuration listeners.- 83-86: The addition of the
getConfigurationmethod provides a way to access theIConfigurationinstance from theConfigInvoker. This method is straightforward and correctly returns them_configurationfield.testng-core/src/test/java/test/listeners/ListenersTest.java (2)
- 59-355: The test methods introduced for validating listener ordering through different approaches (API, XML tag, CLI, and annotations) are well-structured and follow a consistent pattern. Each method clearly describes its purpose and utilizes the
Ensureclass to perform the actual validation. This approach enhances readability and maintainability.However, it's important to ensure that these tests cover all edge cases and potential ordering conflicts among listeners. Additionally, considering the complexity of listener ordering, it might be beneficial to include comments within each test method explaining why certain orderings are expected, especially for tests involving annotations where the ordering might not be immediately obvious.
- 673-736: The
Ensureclass provides static methods to validate the ordering of listeners through different approaches. This class is a key component in the test suite, enabling the concise and clear definition of test cases. The methods within theEnsureclass are well-organized and follow a consistent pattern, which enhances readability and maintainability.A few suggestions for improvement:
- Consider adding comments to explain the logic behind the ordering checks, especially for methods like
orderingViaAnnotationandorderingViaCli, where the setup and expectations might not be immediately clear to someone unfamiliar with the context.- Ensure that the
AnnotationBackedListenerComparatorclass used in these methods is thoroughly tested elsewhere to guarantee its correct implementation, as it plays a crucial role in determining the listener order.Overall, the
Ensureclass is a well-designed utility for testing listener ordering, but adding more detailed comments could improve its clarity and usefulness for future maintainers.testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (1)
- 37-37: The import statement for
org.testng.ListenerComparatoris correctly added to support the new functionality of ordering listeners. This is a necessary change to enable the use ofListenerComparatorwithin theTestInvokerclass.testng-core/src/main/java/org/testng/TestRunner.java (6)
- 174-174: The initialization of
holderwithotherHolderin the constructor ensures that theDataProviderHolderinstance can be passed externally, allowing for more flexible configuration. This change seems logically sound and correctly implemented.- 197-197: Initializing
holderwith a newDataProviderHolderinstance using theconfigurationparameter in this constructor variant ensures that theDataProviderHolderis always properly initialized based on the current configuration. This is a good practice for ensuring consistency and reliability in the TestRunner's behavior.- 220-220: Similar to the previous comment, initializing
holderwith a newDataProviderHolderinstance in this constructor variant ensures consistency and reliability. It's good to see that this pattern is consistently applied across different constructors.- 698-703: Sorting the
visualiserslist using aListenerComparatorif it's not null is a crucial part of implementing the ordered execution of listeners. This approach allows for dynamic ordering based on runtime conditions or configurations. However, it's important to ensure that theListenerComparatoris thoroughly tested to handle all expected and edge cases correctly.Verification successful
To further verify the thoroughness of testing for
ListenerComparatorimplementations, including handling of edge cases, we need to examine the relevant test files. Specifically, we should look into tests forAnnotationBackedListenerComparatorand any other tests that directly involve theListenerComparatorinterface.
To accurately assess the thoroughness of testing for
ListenerComparatorimplementations, including the handling of edge cases, we will now examine the content of bothAnnotationBackedListenerComparator.javaandListenersTest.java.
The examination of
AnnotationBackedListenerComparator.javaandListenersTest.javaprovides valuable insights into the implementation and testing of theListenerComparatorinterface within the TestNG framework.
Implementation Details: The
AnnotationBackedListenerComparatorclass implements theListenerComparatorinterface, providing a custom comparison logic based on annotations present on listener classes. This allows for dynamic ordering of listeners at runtime, aligning with the original review comment's emphasis on the importance of ordered execution.Testing Coverage: The
ListenersTest.javafile contains a comprehensive suite of tests designed to verify the ordering of various types of listeners (e.g., execution listeners, suite listeners, test listeners, etc.) through different mechanisms (API, XML tag, CLI, and annotations). These tests ensure that the ordering logic implemented byAnnotationBackedListenerComparatorworks as expected across a wide range of scenarios, including those that might be considered edge cases.Given the detailed implementation of the
AnnotationBackedListenerComparatorand the extensive testing demonstrated inListenersTest.java, it can be concluded that theListenerComparatorand its implementations are indeed thoroughly tested to handle all expected and edge cases correctly. This confirms the original review comment's assertion about the importance of testing theListenerComparatorto ensure the ordered execution of listeners based on runtime conditions or configurations.* 748-753: Applying the `ListenerComparator` to sort `m_methodInterceptors` is consistent with the approach used for `visualisers`. This ensures that method interceptors are also executed in an ordered manner, which can be critical for tests where the execution order of interceptors affects the outcome. As with the previous comment, thorough testing of the `ListenerComparator` is recommended. * 862-866: Sorting `m_testListeners` using the `ListenerComparator` before firing events is another instance of applying ordered execution logic to listeners. This consistency in handling listener ordering across different types of listeners (test listeners, visualisers, method interceptors) is good to see. Again, ensure that the `ListenerComparator` is robust and well-tested.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation and usage of ListenerComparator across the codebase rg "ListenerComparator" --type javaLength of output: 5220
Script:
#!/bin/bash # Examine tests for ListenerComparator implementations rg "ListenerComparator" --type java --files-with-matches | grep "test/"Length of output: 220
Script:
#!/bin/bash # Inspect the implementation and tests of ListenerComparator cat testng-core/src/test/java/test/listeners/issue2916/AnnotationBackedListenerComparator.java cat testng-core/src/test/java/test/listeners/ListenersTest.javaLength of output: 30028
testng-core/src/main/java/org/testng/TestNG.java (5)
- 276-282: The addition of
setListenerComparatorandgetListenerComparatormethods is correctly implemented. These methods allow setting and retrieving a custom comparator for listeners, which is essential for the new feature of defining the execution order of listeners. The methods are straightforward and follow Java best practices.- 1141-1145: The modification to
runSuiteAlterationListenersto sort the listeners based on the comparator before invocation is logically sound. This ensures that suite alteration listeners are executed in the order defined by the comparator, enhancing the flexibility of listener execution. However, it's important to ensure that the comparator handles null values gracefully to avoid potentialNullPointerExceptions.Verification successful
The implementation of
AnnotationBackedListenerComparatoreffectively handles null values by usingOptional.ofNullablefor null safety and providing a default value when necessary. This ensures that the comparator avoidsNullPointerExceptions, aligning with the concern raised in the review comment. Therefore, the modification to sort suite alteration listeners based on the comparator is verified to be safe regarding null handling.* 1151-1155: The changes to `runExecutionListeners` to sort execution listeners using the comparator are correctly implemented. This modification aligns with the objective of allowing users to define the execution order of listeners. Similar to the previous comment, ensure that the comparator implementation is robust against null values.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if ListenerComparator implementation handles null values to prevent NullPointerException. grep -R "implements ListenerComparator" | grep -v "TestNG.java"Length of output: 234
Script:
#!/bin/bash # Extract the compare method from AnnotationBackedListenerComparator.java to check for null handling grep -A 20 "public int compare" testng-core/src/test/java/test/listeners/issue2916/AnnotationBackedListenerComparator.javaLength of output: 664
Verification successful
The
AnnotationBackedListenerComparatorclass implements a robust approach to handling potentially null annotation values on listener objects, usingOptional.ofNullableto default toInteger.MAX_VALUEwhen annotations are not present. This ensures that listeners without theRunOrderannotation are ordered towards the end, aligning with the goal of allowing users to define execution order through annotations. However, it's worth noting that there are no explicit null checks on the listener objects (l1andl2) themselves before their comparison. This is likely acceptable given the operational context within TestNG, where listener objects passed to this comparator are expected to be non-null. The review comment regarding the robustness against null values in the context of execution listeners is therefore verified with the consideration that the comparator's design is focused on handling null annotation values rather than null listener objects.* 1385-1385: The enhancement to `createSuiteRunner` to initialize `DataProviderHolder` with the configuration is a good addition. It ensures that data provider listeners and interceptors are correctly set up with the suite runner, which is crucial for the new feature's functionality. This change improves the modularity and maintainability of the suite runner creation process. * 1502-1508: The implementation of setting the listener comparator based on command-line arguments is a valuable addition. It allows users to specify a custom comparator class via command-line, enhancing the usability of the new feature. Ensure that the specified class indeed implements `ListenerComparator` and handle potential class loading issues gracefully.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify robustness of ListenerComparator against null values in the context of execution listeners. grep -R "implements ListenerComparator" | grep -v "TestNG.java"Length of output: 234
Script:
#!/bin/bash # Inspect the comparison method of AnnotationBackedListenerComparator for null value handling cat testng-core/src/test/java/test/listeners/issue2916/AnnotationBackedListenerComparator.javaLength of output: 865
Verification successful
Given the information from the script output and the context of the review comment, it's clear that the
ListenerComparatorinterface is well-integrated within the TestNG framework, and the mechanism for setting a customListenerComparatorvia command-line arguments is implemented. However, to fully verify the handling of potential class loading issues and ensure graceful error handling, we would need to examine the error handling or logging mechanisms related to the class loading and instantiation process of theListenerComparator. Since the script output does not provide this specific detail, further investigation into the source code around the provided snippet is necessary.
The review comment's concerns are partially verified. The implementation indeed allows for specifying a custom
ListenerComparatorclass via command-line arguments, enhancing the usability of this feature. The use ofOptionalensures that only classes implementingListenerComparatorcan be set, providing a basic level of error handling by ensuring type safety and null checks. However, the absence of explicit error handling or logging for class loading issues means that while the system may fail gracefully, it might not provide clear feedback to the user about the failure reason. This could be considered an area for improvement rather than an incorrect implementation.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the specified ListenerComparator class can be loaded and correctly implements the interface. grep -R "ListenerComparator" | grep -v "TestNG.java"Length of output: 4394
Script:
#!/bin/bash # Look for error handling or logging mechanisms around the instantiation of ListenerComparator from command-line arguments rg "setListenerComparator" -A 10 -B 10Length of output: 14588
| List<ITestListener> original = m_notifier.getTestListeners(); | ||
| ListenerComparator comparator = m_configuration.getListenerComparator(); | ||
| if (comparator != null) { | ||
| original.sort(comparator::compare); | ||
| } | ||
| List<ITestListener> listeners = | ||
| isFinished | ||
| ? ListenerOrderDeterminer.reversedOrder(m_notifier.getTestListeners()) | ||
| : ListenerOrderDeterminer.order(m_notifier.getTestListeners()); | ||
| ? ListenerOrderDeterminer.reversedOrder(original) | ||
| : ListenerOrderDeterminer.order(original); |
There was a problem hiding this comment.
The logic to sort the listeners based on the ListenerComparator is correctly implemented within the runTestResultListener method. This ensures that listeners are ordered as per the comparator's logic before being invoked. However, it's important to ensure that the comparator handles nulls gracefully to avoid potential NullPointerExceptions.
Additionally, consider adding a comment explaining the rationale behind sorting listeners only when comparator is not null and the impact of listener order on test execution. This will improve code readability and maintainability.
// Sort listeners based on the provided ListenerComparator to ensure the execution order of listeners is as expected.
if (comparator != null) {
original.sort(comparator::compare);
}| List<IConfigurationListener> original = | ||
| Lists.newArrayList(m_notifier.getConfigurationListeners()); | ||
| ListenerComparator comparator = m_configuration.getListenerComparator(); | ||
| if (comparator != null) { | ||
| original.sort(comparator::compare); | ||
| } |
There was a problem hiding this comment.
The modifications in the runConfigurationListeners method introduce sorting logic for configuration listeners based on the ListenerComparator. This change is crucial for achieving the PR's objective of allowing users to define the execution order of listeners. However, it's important to ensure that the comparator is properly tested to handle all edge cases, especially considering the potential complexity of user-defined comparison logic.
Additionally, consider the performance implications of sorting listeners for each configuration method invocation. If the set of listeners or the comparator logic is complex, this could introduce a noticeable overhead. It might be beneficial to cache the sorted list of listeners if the comparator and the set of listeners do not change frequently.
Consider adding performance tests or benchmarks to assess the impact of the sorting logic on the execution time of configuration methods, especially for large sets of listeners or complex comparator logic.
|
|
||
| public class InvokedMethodListenerHolder { | ||
|
|
||
| public static List<String> LOGS = new ArrayList<>(); |
There was a problem hiding this comment.
The use of a public static mutable list LOGS may lead to issues with thread safety and data integrity, especially in a concurrent testing environment.
Consider using thread-safe collections or mechanisms to ensure thread safety, such as Collections.synchronizedList or CopyOnWriteArrayList.
| * </ol> | ||
| */ | ||
| @FunctionalInterface | ||
| public interface ListenerComparator { |
There was a problem hiding this comment.
extends Comparator< ITestNGListener>
| List<IDataProviderInterceptor> original = Lists.newArrayList(interceptors); | ||
| ListenerComparator comparator = getConfiguration().getListenerComparator(); | ||
| if (comparator != null) { | ||
| original.sort(comparator::compare); |
| return Collections.unmodifiableCollection(original); | ||
| } | ||
|
|
||
| public IConfiguration getConfiguration() { |
There was a problem hiding this comment.
What is the purpose of exposing it in public?
| private final IConfiguration configuration; | ||
|
|
||
| public DataProviderHolder(IConfiguration configuration) { | ||
| this.configuration = configuration; |
There was a problem hiding this comment.
this.configuration = Objects.requireNonNull(configuration);
| } | ||
|
|
||
| public Collection<IDataProviderInterceptor> getInterceptors() { | ||
| return Collections.unmodifiableCollection(interceptors); | ||
| List<IDataProviderInterceptor> original = Lists.newArrayList(interceptors); | ||
| ListenerComparator comparator = getConfiguration().getListenerComparator(); |
There was a problem hiding this comment.
Avoid duplication and create a private static method that sorts a list when a comparator is not null and return an unmodifiableCollection
There was a problem hiding this comment.
Not sure if I can create one single method here because:
public Collection<IDataProviderListener> getListeners()public Collection<IDataProviderInterceptor> getInterceptors()
I cannot create a method that would look like below for e.g.,
private Collection<ITestNGListener> sort(Collection<ITestNGListener> initial) Java complains that I cannot fit Collection<IDataProviderListener> into Collection<ITestNGListener> initial due to type erasure perhaps ? Any suggestions on this ?
There was a problem hiding this comment.
When i build a method that looks like
private Collection<? extends ITestNGListener> sort(Collection<? extends ITestNGListener> initial) {
ListenerComparator comparator = configuration.getListenerComparator();
if (comparator == null) {
return initial;
}
List<? extends ITestNGListener> original = Lists.newArrayList(initial);
original.sort(comparator);
return Collections.unmodifiableCollection(original);
}then the caller would look like below
public Collection<IDataProviderInterceptor> getInterceptors() {
return (Collection<IDataProviderInterceptor>) sort(interceptors);
}triggering an Unchecked cast edit check on this method. I can add a @SuppressWarnings. Is that fine ?
| } | ||
|
|
||
| public Collection<IDataProviderInterceptor> getInterceptors() { | ||
| return Collections.unmodifiableCollection(interceptors); | ||
| List<IDataProviderInterceptor> original = Lists.newArrayList(interceptors); |
There was a problem hiding this comment.
The new list is only mandatory when the comparator is not null.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (9)
- testng-core/src/main/java/org/testng/DataProviderHolder.java (2 hunks)
- testng-core/src/main/java/org/testng/ListenerComparator.java (1 hunks)
- testng-core/src/main/java/org/testng/SuiteRunner.java (10 hunks)
- testng-core/src/main/java/org/testng/TestNG.java (5 hunks)
- testng-core/src/main/java/org/testng/TestRunner.java (8 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/BaseInvoker.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/ConfigInvoker.java (4 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (3 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java (3 hunks)
Files skipped from review as they are similar to previous changes (9)
- testng-core/src/main/java/org/testng/DataProviderHolder.java
- testng-core/src/main/java/org/testng/ListenerComparator.java
- testng-core/src/main/java/org/testng/SuiteRunner.java
- testng-core/src/main/java/org/testng/TestNG.java
- testng-core/src/main/java/org/testng/TestRunner.java
- testng-core/src/main/java/org/testng/internal/invokers/BaseInvoker.java
- testng-core/src/main/java/org/testng/internal/invokers/ConfigInvoker.java
- testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java
- testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java
|
@juherr - Thank you for fixing the comments as well. Can we go ahead and merge this PR now ? |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (6)
- testng-core/src/main/java/org/testng/SuiteRunner.java (9 hunks)
- testng-core/src/main/java/org/testng/TestNG.java (6 hunks)
- testng-core/src/main/java/org/testng/TestRunner.java (8 hunks)
- testng-core/src/main/java/org/testng/internal/ListenerOrderDeterminer.java (3 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/BaseInvoker.java (1 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (2 hunks)
Files skipped from review as they are similar to previous changes (5)
- testng-core/src/main/java/org/testng/SuiteRunner.java
- testng-core/src/main/java/org/testng/TestNG.java
- testng-core/src/main/java/org/testng/TestRunner.java
- testng-core/src/main/java/org/testng/internal/invokers/BaseInvoker.java
- testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java
Additional comments: 2
testng-core/src/main/java/org/testng/internal/ListenerOrderDeterminer.java (2)
- 3-3: The import of
ListenerComparator.sortis correctly added to facilitate sorting functionality within theorderandreversedOrdermethods. This static import helps in keeping the code concise.- 13-13: The import of
org.testng.ListenerComparatoris necessary for the new functionality, allowing theorderandreversedOrdermethods to utilize theListenerComparatorfor sorting listeners based on the defined criteria.
| * @param original - The original collection of listeners | ||
| * @return - A re-ordered collection wherein preferential listeners are added at the end | ||
| */ | ||
| public static <T> List<T> order(Collection<T> original) { | ||
| public static <T extends ITestNGListener> List<T> order( | ||
| Collection<T> original, ListenerComparator comparator) { | ||
| original = sort(original, comparator); | ||
| Pair<List<T>, List<T>> ordered = arrange(original); | ||
| List<T> ideListeners = ordered.first(); | ||
| List<T> regularListeners = ordered.second(); |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [50-66]
The method signatures for order and reversedOrder have been updated to include a ListenerComparator parameter. This change is crucial for enabling custom sorting logic based on the comparator provided at runtime. However, there's a potential improvement in how the original collection is handled within these methods.
Currently, the original collection is directly modified by the sort method call. This could lead to unexpected side effects if the caller does not anticipate the collection being sorted in-place. It would be safer to work on a copy of the original collection to avoid modifying the input collection directly.
- original = sort(original, comparator);
+ Collection<T> sortedOriginal = sort(new ArrayList<>(original), comparator);And then use sortedOriginal in the subsequent logic instead of original.
Just need that one suite is working well |
|
There are compilation errors @juherr . I am looking into it. I will get it sorted out and get this merged. |
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/internal/TestListenerHelper.java (3 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/ConfigInvoker.java (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- testng-core/src/main/java/org/testng/internal/invokers/ConfigInvoker.java
Additional comments: 3
testng-core/src/main/java/org/testng/internal/TestListenerHelper.java (3)
- 13-13: The import statement for
ListenerComparatoris correctly added to support the new functionality of ordering listeners. This change is necessary for the subsequent use ofListenerComparatorin method parameters.- 29-34: The addition of the
ListenerComparatorparameter in therunPreConfigurationListenersmethod and its use to sort the listeners before invoking theirbeforeConfigurationmethods is a logical and necessary change to implement listener ordering. However, it's important to ensure that all calls to this method throughout the codebase have been updated to include the new parameter to avoid compilation errors.Verification successful
Given the initial search did not yield results, it's prudent to broaden the search criteria to ensure all calls to
runPreConfigurationListenersare captured, regardless of the number of parameters. This approach will help in verifying whether all instances have been updated to include the newListenerComparatorparameter.
The search successfully identified a call to
runPreConfigurationListenersinConfigInvoker.javathat includes the newListenerComparatorparameter, indicating that the codebase has been updated to accommodate the change. This finding supports the conclusion that the addition of theListenerComparatorparameter and its integration into the method call have been correctly implemented, at least in the identified instance. Without evidence of other calls failing to include the new parameter, it's reasonable to conclude that the necessary updates have been made throughout the codebase.* 48-51: The addition of the `ListenerComparator` parameter in the `runPostConfigurationListeners` method and its use to sort the listeners in reverse order before invoking their configuration methods based on the test result status is correctly implemented. This ensures that listeners are invoked in the specified order for post-configuration events. Similar to the previous method, it's crucial to verify that all calls to this method have been updated accordingly.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to runPreConfigurationListeners missing the new parameter. ast-grep --lang java --pattern $'TestListenerHelper.runPreConfigurationListeners($_, $_, $_, $_)'Length of output: 98
Script:
#!/bin/bash # Broaden the search to capture any calls to runPreConfigurationListeners, regardless of the number of parameters. ast-grep --lang java --pattern $'TestListenerHelper.runPreConfigurationListeners($$$)'Length of output: 764
Verification successful
The executed script aimed to find calls to
runPostConfigurationListenerswith four parameters, presumably missing the newly addedListenerComparatorparameter. The lack of output from the script suggests that there are no instances in the codebase whererunPostConfigurationListenersis called with an incorrect number of parameters. This supports the review comment's assertion that the addition of theListenerComparatorparameter is correctly implemented and implies that all calls to this method have been updated accordingly to include the new parameter.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to runPostConfigurationListeners missing the new parameter. ast-grep --lang java --pattern $'TestListenerHelper.runPostConfigurationListeners($_, $_, $_, $_)'Length of output: 99
Closes #2916
Fixes #2916 .
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
ListenerComparatorfor ordering listener execution.ListenerComparator.