Allow listeners to be disabled at runtime #3088
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe update introduces runtime control for listener inclusion by adding an 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-api/src/main/java/org/testng/ITestNGListener.java (1 hunks)
- testng-core/src/main/java/org/testng/SuiteRunner.java (1 hunks)
- testng-core/src/main/java/org/testng/TestNG.java (2 hunks)
- testng-core/src/main/java/org/testng/TestRunner.java (3 hunks)
- testng-core/src/main/java/org/testng/internal/DynamicGraph.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/Parameters.java (5 hunks)
- testng-core/src/main/java/org/testng/internal/TestListenerHelper.java (1 hunks)
- testng-core/src/main/java/org/testng/internal/annotations/JDK15AnnotationFinder.java (1 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/InvokedMethodListenerInvoker.java (1 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java (3 hunks)
- testng-core/src/test/java/test/listeners/ListenersTest.java (2 hunks)
- testng-core/src/test/java/test/listeners/issue2381/SampleGlobalListener.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2381/TestClassSample.java (1 hunks)
Files not reviewed due to errors (1)
- (no review received)
Files skipped from review due to trivial changes (1)
- testng-core/src/test/java/test/listeners/issue2381/TestClassSample.java
Additional comments: 22
testng-core-api/src/main/java/org/testng/ITestNGListener.java (1)
- 8-13: The addition of the
isEnabled()method with a default implementation returningtrueis a thoughtful approach to introduce dynamic enabling/disabling of listeners without breaking existing implementations. This change enhances flexibility and control for TestNG users.testng-core/src/main/java/org/testng/internal/invokers/InvokedMethodListenerInvoker.java (1)
- 54-56: The implementation of the enabled check in the
invokeListenermethod correctly ensures that only enabled listeners are invoked. This is a key part of the new feature allowing dynamic control over listener execution.testng-core/src/test/java/test/listeners/issue2381/SampleGlobalListener.java (1)
- 21-136: The
SampleGlobalListenerclass effectively demonstrates the new feature by implementing various listener interfaces and overriding theisEnabled()method to returnfalse. This serves as a good test case for validating the dynamic disabling of listeners at runtime.testng-core/src/main/java/org/testng/internal/TestListenerHelper.java (1)
- 102-104: The implementation of the enabled check in the
runTestListenersmethod ensures that test listeners are only invoked if they are enabled. This aligns with the PR's objective and enhances the flexibility of listener execution.testng-core/src/main/java/org/testng/internal/invokers/TestMethodWorker.java (1)
- 171-173: The refactoring to use Java streams for filtering enabled listeners and iterating over them in the invocation of before and after class methods enhances code readability and conciseness. This change aligns with modern Java practices and supports the new feature's functionality.
Also applies to: 237-239
testng-core/src/main/java/org/testng/internal/annotations/JDK15AnnotationFinder.java (1)
- 169-172: The modifications to include conditional checks for the transformer's enabled status before invoking the
transformmethod for different types of annotations ensure that transformations occur only when appropriate. This enhancement aligns with the PR's objective and the dynamic control introduced by theisEnabled()method.Also applies to: 174-176, 180-182, 184-186, 188-190, 192-194
testng-core/src/main/java/org/testng/internal/DynamicGraph.java (2)
- 12-12: The addition of the
org.testng.ITestNGListenerimport is necessary for the new functionality introduced in thesetStatusmethod. This change is consistent with the PR's objectives.- 146-148: The modifications to filter and invoke visualisers based on their enabled status align with the PR's goal of allowing listeners to be disabled at runtime. This implementation uses modern Java practices and is efficient. However, ensure that all
visualisersindeed implementITestNGListenerto avoid potentialClassCastException.testng-core/src/main/java/org/testng/SuiteRunner.java (1)
- 240-243: The modifications to use streams for filtering and invoking listeners based on their enabled status are well-implemented and align with the PR's objectives. This approach enhances code readability and conciseness. Ensure that the
configuration.getListenerComparator()used for ordering listeners provides a stable and expected order for listener invocation, especially when listeners have dependencies on each other's execution order.Also applies to: 245-249
testng-core/src/test/java/test/listeners/ListenersTest.java (1)
- 525-532: The new test method
ensureListenersCanBeDisabledcorrectly implements the functionality to test the dynamic disabling of listeners at runtime. It's crucial to ensure that theSampleGlobalListenercorrectly overrides theisEnabled()method to returnfalsewhen it should be disabled. Additionally, the effectiveness of this test relies on the correct implementation of theisEnabled()check within the TestNG core.Ensure that the
SampleGlobalListenercorrectly implements theisEnabled()method and that its integration within the TestNG core is functioning as expected.testng-core/src/main/java/org/testng/internal/Parameters.java (4)
- 802-807: The implementation of filtering listeners based on their enabled status before data provider execution is a good practice. It ensures that only the necessary listeners are invoked, which can improve performance and maintainability. However, it's important to ensure that the
isEnabled()method's default implementation returnstrueto maintain backward compatibility with existing listeners that do not override this method.- 822-824: Similar to the previous comment, filtering listeners based on their enabled status in the event of a data provider failure is a good practice. This approach ensures that only relevant listeners are notified about the failure, which can help in debugging and error handling. Again, ensure that the
isEnabled()method's default implementation returnstruefor backward compatibility.- 845-850: The use of the
isEnabled()method to filter listeners after data provider execution is consistent with the previous implementations. This consistency in handling listener invocation based on their enabled status across different stages of data provider execution is commendable. It enhances the flexibility and control over listener execution, aligning with the PR's objectives.- 862-866: Filtering
IDataProviderInterceptorinstances based on their enabled status before intercepting the parameters is a thoughtful addition. It allows for more granular control over which interceptors are applied, potentially reducing overhead and improving execution efficiency. As with the listeners, ensure that any default or existing interceptors are considered enabled unless explicitly overridden.testng-core/src/main/java/org/testng/TestRunner.java (4)
- 748-750: The addition of the
isEnabled()check before applying theIMethodInterceptoris in line with the PR's objectives and correctly implemented. Please ensure that the default implementation ofisEnabled()returnstrueto maintain backward compatibility with existing listeners that do not override this method.- 858-861: The addition of the
isEnabled()check before invokingITestListener.onStart()is correctly implemented and aligns with the PR's objectives. Ensure the default implementation ofisEnabled()returnstruefor backward compatibility.- 865-869: The addition of the
isEnabled()check before invokingITestListener.onFinish()is correctly implemented. As with theonStart()method, please ensure the default implementation ofisEnabled()returnstrue.- 1106-1108: The addition of the
isEnabled()check before invokingIExecutionListener.onExecutionStart()is correctly implemented. Ensure the default implementation ofisEnabled()returnstrueto maintain backward compatibility with existing execution listeners that do not override this method.testng-core/src/main/java/org/testng/TestNG.java (4)
- 1115-1115: The use of
.stream().filter(ITestNGListener::isEnabled)to filter enabled listeners before invokingalteron them is a good application of Java 8 streams and lambda expressions. This ensures that only enabled listeners are processed, enhancing performance and maintainability.- 1123-1125: The refactoring to use streams and lambda expressions in
runExecutionListenersfor the start phase is consistent with modern Java practices. Filtering enabled listeners before invokingonExecutionStartis efficient and clean.- 1132-1134: Similarly, the use of streams and lambda expressions in the finish phase of
runExecutionListenersmaintains consistency and ensures that only enabled listeners are notified of the execution finish. This approach is both efficient and readable.- 1153-1155: In
generateReports, filtering out disabled reporters before generating reports is a prudent choice. This prevents unnecessary processing and potential errors from disabled reporters. The use of streams here aligns with the overall approach of leveraging modern Java features for clarity and efficiency.
juherr
reviewed
Mar 13, 2024
By providing this capability, we now allow users to enable/disable user level listeners at runtime. This provides a much more easier way of doing it, instead of adding You can imagine this to be something like a cheap version of |
|
|
I was only drawing parity in terms of the functional equivalency and not necessarily hinting at the implementation aspect. For e.g., if one would like to have a listener conditionally executed (specific OS, or IDE vs CI or any other condition), I feel that this capability will be a lot more simpler instead of adding if conditions to all the method implementations which makes the code verbose. |
|
OK. Let me time to think about it. |
@juherr - Please check this now. I have refactored the code wherein the check is being used to determine if the listener is to be wired in or not (in previous implementation, it was used to determine if listener is to be called or not) and thus aligning a bit more closer in terms of being the control flag at bootstrap time. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
testng-core/src/test/resources/2381.xmlis excluded by:!**/*.xml
Files selected for processing (9)
- CHANGES.txt (1 hunks)
- testng-core-api/src/main/java/org/testng/ITestNGListener.java (1 hunks)
- testng-core/src/main/java/org/testng/TestNG.java (1 hunks)
- testng-core/src/main/java/org/testng/internal/annotations/JDK15AnnotationFinder.java (1 hunks)
- testng-core/src/test/java/test/listeners/ListenersTest.java (2 hunks)
- testng-core/src/test/java/test/listeners/issue2381/FactoryTestClassSample.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2381/SampleGlobalListener.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2381/SampleTransformer.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2381/TestClassSample.java (1 hunks)
Files skipped from review as they are similar to previous changes (7)
- CHANGES.txt
- testng-core-api/src/main/java/org/testng/ITestNGListener.java
- testng-core/src/main/java/org/testng/TestNG.java
- testng-core/src/main/java/org/testng/internal/annotations/JDK15AnnotationFinder.java
- testng-core/src/test/java/test/listeners/ListenersTest.java
- testng-core/src/test/java/test/listeners/issue2381/SampleGlobalListener.java
- testng-core/src/test/java/test/listeners/issue2381/TestClassSample.java
Additional comments: 5
testng-core/src/test/java/test/listeners/issue2381/FactoryTestClassSample.java (3)
- 11-12: The usage of the
@Factoryannotation with a data provider is correctly implemented. The parameter nameignoredis acceptable in this context, as it indicates the parameter is necessary for the method signature but not used within the method body.- 14-17: The
@DataProvidermethoddpis correctly implemented, returning the expected data format for TestNG. This setup is suitable for demonstration or testing purposes.- 19-26: The usage of lifecycle annotations
@BeforeSuite,@Test, and@AfterSuiteis correctly implemented. The methods are empty, which is acceptable in this context, as the focus is likely on the structure rather than the implementation.testng-core/src/test/java/test/listeners/issue2381/SampleTransformer.java (2)
- 27-30: The implementation of the
isEnabledmethod, returningfalse, aligns with the PR's objectives of testing the dynamic disabling of listeners. This approach is crucial for verifying the new functionality.- 32-60: The transformation methods (
transform) are consistently implemented with a logging mechanism that effectively aids in verifying the transformation process. This approach is practical and aligns with the objectives of providing insights into the transformation actions.
juherr
approved these changes
Mar 14, 2024
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
testng-core/src/test/resources/2381.xmlis excluded by:!**/*.xml
Files selected for processing (9)
- CHANGES.txt (1 hunks)
- testng-core-api/src/main/java/org/testng/ITestNGListener.java (1 hunks)
- testng-core/src/main/java/org/testng/TestNG.java (1 hunks)
- testng-core/src/main/java/org/testng/internal/annotations/JDK15AnnotationFinder.java (1 hunks)
- testng-core/src/test/java/test/listeners/ListenersTest.java (2 hunks)
- testng-core/src/test/java/test/listeners/issue2381/FactoryTestClassSample.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2381/SampleGlobalListener.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2381/SampleTransformer.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2381/TestClassSample.java (1 hunks)
Files skipped from review as they are similar to previous changes (9)
- CHANGES.txt
- testng-core-api/src/main/java/org/testng/ITestNGListener.java
- testng-core/src/main/java/org/testng/TestNG.java
- testng-core/src/main/java/org/testng/internal/annotations/JDK15AnnotationFinder.java
- testng-core/src/test/java/test/listeners/ListenersTest.java
- testng-core/src/test/java/test/listeners/issue2381/FactoryTestClassSample.java
- testng-core/src/test/java/test/listeners/issue2381/SampleGlobalListener.java
- testng-core/src/test/java/test/listeners/issue2381/SampleTransformer.java
- testng-core/src/test/java/test/listeners/issue2381/TestClassSample.java
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
testng-core/src/test/resources/2381.xmlis excluded by:!**/*.xml
Files selected for processing (9)
- CHANGES.txt (1 hunks)
- testng-core-api/src/main/java/org/testng/ITestNGListener.java (1 hunks)
- testng-core/src/main/java/org/testng/TestNG.java (1 hunks)
- testng-core/src/main/java/org/testng/internal/annotations/JDK15AnnotationFinder.java (1 hunks)
- testng-core/src/test/java/test/listeners/ListenersTest.java (2 hunks)
- testng-core/src/test/java/test/listeners/issue2381/FactoryTestClassSample.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2381/SampleGlobalListener.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2381/SampleTransformer.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue2381/TestClassSample.java (1 hunks)
Files skipped from review as they are similar to previous changes (9)
- CHANGES.txt
- testng-core-api/src/main/java/org/testng/ITestNGListener.java
- testng-core/src/main/java/org/testng/TestNG.java
- testng-core/src/main/java/org/testng/internal/annotations/JDK15AnnotationFinder.java
- testng-core/src/test/java/test/listeners/ListenersTest.java
- testng-core/src/test/java/test/listeners/issue2381/FactoryTestClassSample.java
- testng-core/src/test/java/test/listeners/issue2381/SampleGlobalListener.java
- testng-core/src/test/java/test/listeners/issue2381/SampleTransformer.java
- testng-core/src/test/java/test/listeners/issue2381/TestClassSample.java
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #2381
Fixes #2381 .
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