Honour inheritance when parsing listener factories #3096
Conversation
WalkthroughThe recent changes focus on enhancing test execution behavior, addressing issues with test class inheritance, method interception, listener control, and instance management during method invocation. Additionally, improvements in listener handling, including the use of Changes
Assessment against linked issues
Related issues
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: 3
Configuration used: CodeRabbit UI
Files selected for processing (10)
- CHANGES.txt (1 hunks)
- testng-core/src/main/java/org/testng/internal/TestListenerHelper.java (4 hunks)
- testng-core/src/test/java/org/testng/internal/TestListenerHelperTest.java (3 hunks)
- testng-core/src/test/java/org/testng/internal/listeners/ListenerFactoryContainer.java (1 hunks)
- testng-core/src/test/java/org/testng/internal/listeners/TestClassContainer.java (1 hunks)
- testng-core/src/test/java/org/testng/internal/listeners/TestClassWithCompositeListener.java (1 hunks)
- testng-core/src/test/java/test/listeners/ListenersTest.java (2 hunks)
- testng-core/src/test/java/test/listeners/issue3095/ChildClassSample.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue3095/MyTestNgFactory.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue3095/SuperClassSample.java (1 hunks)
Files skipped from review due to trivial changes (1)
- testng-core/src/test/java/org/testng/internal/listeners/ListenerFactoryContainer.java
Additional comments: 7
testng-core/src/test/java/test/listeners/issue3095/SuperClassSample.java (1)
- 1-6: The usage of
@Listenersto specifyMyTestNgFactoryas a listener factory is correctly implemented.testng-core/src/test/java/test/listeners/issue3095/ChildClassSample.java (1)
- 1-8: The inheritance of
@ListenersfromSuperClassSampleand the definition of a test method are correctly implemented.testng-core/src/test/java/org/testng/internal/listeners/TestClassWithCompositeListener.java (1)
- 3-9: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-6]
The update to use
ListenerFactoryContainer.DummyListenerFactoryin the@Listenersannotation is correctly implemented and aligns with the refactor to encapsulate listener factories.testng-core/src/test/java/org/testng/internal/TestListenerHelperTest.java (1)
- 45-69: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-88]
The tests in
TestListenerHelperTestare well-structured and cover the new functionality introduced in the PR. It would be beneficial to add more detailed comments to the test methods, especiallytestFindAllListenersDuplicateListenerFactories, to explain the scenarios being tested and the expected outcomes.+ // Test to verify that TestNG can handle duplicate listener factories without issues. @Test(description = "GITHUB-3095") public void testFindAllListenersDuplicateListenerFactories() {testng-core/src/main/java/org/testng/internal/TestListenerHelper.java (1)
- 176-212: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [3-209]
The use of
Optionaland the updated logic inListenerHolderfor handling listener annotations and factories are correctly implemented. However, refining the exception message when multiple listener factories are found would enhance clarity and provide more context.- throw new TestNGException( - "Found more than one class implementing ITestNGListenerFactory:" - + c - + " and " - + listenerFactoryClass); + throw new TestNGException(String.format( + "Found more than one class implementing ITestNGListenerFactory: %s and %s. " + + "Please ensure that only one ITestNGListenerFactory is specified.", + c.getName(), listenerFactoryClass.getName()));CHANGES.txt (2)
- 3-3: The change log entry for GITHUB-3095 is clear and directly addresses the issue described in the PR objectives, specifically the handling of
ITestNGListenerFactoryannotations on superclasses and preventing crashes due to multiple implementations.- 1-6: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [3-22]
All listed changes in the version 7.10.0 log are consistent with the PR objectives and the AI-generated summary, indicating a comprehensive fix for the identified bug and related issues. The documentation of fixes and new features is clear, contributing to the maintainability and usability of the TestNG framework.
testng-core/src/test/java/org/testng/internal/listeners/TestClassContainer.java
Show resolved
Hide resolved
testng-core/src/main/java/org/testng/internal/TestListenerHelper.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 (10)
- CHANGES.txt (1 hunks)
- testng-core/src/main/java/org/testng/internal/TestListenerHelper.java (4 hunks)
- testng-core/src/test/java/org/testng/internal/TestListenerHelperTest.java (3 hunks)
- testng-core/src/test/java/org/testng/internal/listeners/ListenerFactoryContainer.java (1 hunks)
- testng-core/src/test/java/org/testng/internal/listeners/TestClassContainer.java (1 hunks)
- testng-core/src/test/java/org/testng/internal/listeners/TestClassWithCompositeListener.java (1 hunks)
- testng-core/src/test/java/test/listeners/ListenersTest.java (2 hunks)
- testng-core/src/test/java/test/listeners/issue3095/ChildClassSample.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue3095/MyTestNgFactory.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue3095/SuperClassSample.java (1 hunks)
Files skipped from review as they are similar to previous changes (10)
- CHANGES.txt
- testng-core/src/main/java/org/testng/internal/TestListenerHelper.java
- testng-core/src/test/java/org/testng/internal/TestListenerHelperTest.java
- testng-core/src/test/java/org/testng/internal/listeners/ListenerFactoryContainer.java
- testng-core/src/test/java/org/testng/internal/listeners/TestClassContainer.java
- testng-core/src/test/java/org/testng/internal/listeners/TestClassWithCompositeListener.java
- testng-core/src/test/java/test/listeners/ListenersTest.java
- testng-core/src/test/java/test/listeners/issue3095/ChildClassSample.java
- testng-core/src/test/java/test/listeners/issue3095/MyTestNgFactory.java
- testng-core/src/test/java/test/listeners/issue3095/SuperClassSample.java
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (10)
- CHANGES.txt (1 hunks)
- testng-core/src/main/java/org/testng/internal/TestListenerHelper.java (4 hunks)
- testng-core/src/test/java/org/testng/internal/TestListenerHelperTest.java (3 hunks)
- testng-core/src/test/java/org/testng/internal/listeners/ListenerFactoryContainer.java (1 hunks)
- testng-core/src/test/java/org/testng/internal/listeners/TestClassContainer.java (1 hunks)
- testng-core/src/test/java/org/testng/internal/listeners/TestClassWithCompositeListener.java (1 hunks)
- testng-core/src/test/java/test/listeners/ListenersTest.java (2 hunks)
- testng-core/src/test/java/test/listeners/issue3095/ChildClassSample.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue3095/MyTestNgFactory.java (1 hunks)
- testng-core/src/test/java/test/listeners/issue3095/SuperClassSample.java (1 hunks)
Files skipped from review as they are similar to previous changes (10)
- CHANGES.txt
- testng-core/src/main/java/org/testng/internal/TestListenerHelper.java
- testng-core/src/test/java/org/testng/internal/TestListenerHelperTest.java
- testng-core/src/test/java/org/testng/internal/listeners/ListenerFactoryContainer.java
- testng-core/src/test/java/org/testng/internal/listeners/TestClassContainer.java
- testng-core/src/test/java/org/testng/internal/listeners/TestClassWithCompositeListener.java
- testng-core/src/test/java/test/listeners/ListenersTest.java
- testng-core/src/test/java/test/listeners/issue3095/ChildClassSample.java
- testng-core/src/test/java/test/listeners/issue3095/MyTestNgFactory.java
- testng-core/src/test/java/test/listeners/issue3095/SuperClassSample.java
Closes #3095
Fixes #3095 .
Did you remember to?
CHANGES.txt./gradlew autostyleApplyRoot cause: TestNG was enhanced such that the
@Listenersannotation was inheritable via the PR #3002Summary by CodeRabbit