Support ITestNGFactory customisation #3060
Conversation
|
Warning Rate Limit Exceeded@krmahadevan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 2 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe update introduces the ability to inject custom listener factories into TestNG, enhancing flexibility and customization for users. It addresses issues with empty results and deprecated JUnit support, while also adding new capabilities for listener management. This change allows for more dynamic and user-defined handling of TestNG listeners, improving the framework's adaptability to different testing scenarios. Changes
Assessment against linked issues
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 ignored due to path filters (1)
testng-core/src/test/resources/testng.xmlis excluded by:!**/*.xml
Files selected for processing (12)
- CHANGES.txt (1 hunks)
- testng-core/src/main/java/org/testng/CommandLineArgs.java (1 hunks)
- testng-core/src/main/java/org/testng/TestNG.java (4 hunks)
- testng-core/src/main/java/org/testng/TestRunner.java (2 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/TestListenerHelper.java (1 hunks)
- testng-core/src/main/java/org/testng/internal/objects/pojo/CreationAttributes.java (2 hunks)
- testng-core/src/test/java/test/listeners/factory/ExampleListener.java (1 hunks)
- testng-core/src/test/java/test/listeners/factory/SampleTestCase.java (1 hunks)
- testng-core/src/test/java/test/listeners/factory/SampleTestFactory.java (1 hunks)
- testng-core/src/test/java/test/listeners/factory/TestNGFactoryTest.java (1 hunks)
Files skipped from review due to trivial changes (1)
- testng-core/src/test/java/test/listeners/factory/SampleTestCase.java
Additional comments: 7
testng-core/src/main/java/org/testng/internal/objects/pojo/CreationAttributes.java (1)
- 12-17: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [7-31]
Ensure that removing null checks on context parameters does not introduce potential null pointer exceptions. If contexts are expected to be non-null, consider reinstating null checks.
testng-core/src/main/java/org/testng/internal/IConfiguration.java (1)
- 13-15: Changes to IConfiguration interface align with the PR's objective and adhere to best practices.
testng-core/src/main/java/org/testng/internal/Configuration.java (1)
- 66-81: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [28-77]
Changes to Configuration class correctly implement the new listener factory functionality and follow encapsulation principles.
testng-core/src/main/java/org/testng/internal/TestListenerHelper.java (1)
- 164-165: Deprecation of
createListenerFactorymethod aligns with the introduction of new listener factory functionality.testng-core/src/main/java/org/testng/CommandLineArgs.java (1)
- 169-172: Addition of
listenerFactoryfield andLISTENER_FACTORYconstant correctly implements the new functionality for specifying custom listener factories through command line arguments.testng-core/src/main/java/org/testng/TestRunner.java (1)
- 381-382: The use of
Optional.ofNullablefollowed by.orElsefor initializingITestNGListenerFactoryis correctly implemented. This approach cleanly handles potential null values and provides a clear fallback mechanism.CHANGES.txt (1)
- 3-3: The added line correctly summarizes a new feature introduced in TestNG, aligning with the expected format and content for this document.
| public static ExampleListener instance; | ||
|
|
||
| public ExampleListener() { | ||
| setInstance(this); | ||
| } | ||
|
|
||
| private static void setInstance(ExampleListener instance) { | ||
| ExampleListener.instance = instance; | ||
| } |
There was a problem hiding this comment.
The use of a public static instance variable for singleton pattern is not thread-safe. Consider using a thread-safe singleton pattern implementation.
| public void onExecutionStart() {} | ||
|
|
||
| @Override | ||
| public void onExecutionFinish() {} |
There was a problem hiding this comment.
Empty implementation of onExecutionStart and onExecutionFinish. If these are placeholders, consider adding comments to clarify their intended use.
| public static ITestNGListenerFactory instance; | ||
|
|
||
| public boolean wasInvoked = false; | ||
|
|
||
| public SampleTestFactory() { | ||
| setInstance(this); | ||
| } | ||
|
|
||
| private static void setInstance(ITestNGListenerFactory instance) { | ||
| SampleTestFactory.instance = instance; | ||
| } |
There was a problem hiding this comment.
The use of a public static instance variable for singleton pattern is not thread-safe. Consider using a thread-safe singleton pattern implementation.
|
|
||
| public static ITestNGListenerFactory instance; | ||
|
|
||
| public boolean wasInvoked = false; |
There was a problem hiding this comment.
The wasInvoked flag is publicly accessible, which could lead to unintended modifications. Consider making it private and providing a public method to access its value.
| public class TestNGFactoryTest extends SimpleBaseTest { | ||
|
|
||
| @Test(description = "GITHUB-3059") | ||
| public void testListenerFactoryViaConfigurationArg() { | ||
| String[] args = | ||
| new String[] { | ||
| CommandLineArgs.LISTENER_FACTORY, | ||
| SampleTestFactory.class.getName(), | ||
| CommandLineArgs.TEST_CLASS, | ||
| SampleTestCase.class.getName(), | ||
| CommandLineArgs.LISTENER, | ||
| ExampleListener.class.getName() | ||
| }; | ||
| TestNG testng = TestNG.privateMain(args, null); | ||
| assertThat(SampleTestFactory.instance).isNotNull(); | ||
| assertThat(ExampleListener.instance).isNotNull(); | ||
| assertThat(testng.getStatus()).isZero(); | ||
| } | ||
|
|
||
| @Test(description = "GITHUB-3059") | ||
| public void testListenerFactoryViaTestNGApi() { | ||
| TestNG testng = new TestNG(); | ||
| SampleTestFactory factory = new SampleTestFactory(); | ||
| testng.setListenerFactory(factory); | ||
| testng.setListenerClasses(List.of(ExampleListener.class)); | ||
| testng.setTestClasses(new Class[] {SampleTestCase.class}); | ||
| testng.run(); | ||
| assertThat(testng.getStatus()).isZero(); | ||
| assertThat(factory.wasInvoked).isTrue(); | ||
| } |
There was a problem hiding this comment.
Tests cover basic functionality of the listener factory feature. Consider adding more comprehensive tests to cover edge cases or potential failure scenarios.
krmahadevan
force-pushed
the
allow_testlistener_factory_to_be_configurable
branch
from
30a556e
to
38b4fd4
Compare
testng-core/src/main/java/org/testng/internal/objects/pojo/CreationAttributes.java
Show resolved
Hide resolved
Closes #3059
Fixes #3059
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
TestListenerHelperas part of enhancing listener management.