Allow custom thread pool executors to be wired in. #3075
Conversation
WalkthroughThe recent updates to TestNG focus on enhancing thread management and customization capabilities. By introducing 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: 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 (16)
- CHANGES.txt (1 hunks)
- testng-core-api/src/main/java/org/testng/IExecutorServiceFactory.java (1 hunks)
- testng-core/src/main/java/org/testng/SuiteRunner.java (1 hunks)
- testng-core/src/main/java/org/testng/SuiteTaskExecutor.java (2 hunks)
- testng-core/src/main/java/org/testng/TestNG.java (7 hunks)
- testng-core/src/main/java/org/testng/TestTaskExecutor.java (3 hunks)
- testng-core/src/main/java/org/testng/internal/Configuration.java (4 hunks)
- testng-core/src/main/java/org/testng/internal/IConfiguration.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/ConfigInvoker.java (1 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/MethodInvocationHelper.java (5 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/thread/ThreadUtil.java (4 hunks)
- testng-core/src/test/java/test/thread/CustomExecutorServiceFactoryTest.java (1 hunks)
- testng-core/src/test/java/test/thread/issue3066/Issue3066ExecutorServiceFactory.java (1 hunks)
- testng-core/src/test/java/test/thread/issue3066/Issue3066ThreadPoolExecutor.java (1 hunks)
- testng-core/src/test/java/test/thread/issue3066/TestClassSample.java (1 hunks)
Files skipped from review due to trivial changes (1)
- testng-core/src/test/java/test/thread/issue3066/TestClassSample.java
Additional comments: 17
testng-core-api/src/main/java/org/testng/IExecutorServiceFactory.java (1)
- 1-35: The
IExecutorServiceFactoryinterface is well-defined, with clear documentation explaining its purpose and the method signature for creating custom executor services. This interface is a key component of the PR's objective to enhance TestNG's thread management capabilities by allowing users to plug in custom executor services.The documentation and method signature are clear and concise, providing all necessary parameters for creating an
ExecutorService. This change effectively addresses the need for dynamic thread management in TestNG.testng-core/src/main/java/org/testng/internal/thread/ThreadUtil.java (2)
- 47-61: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [33-58]
The refactoring of the
executemethod inThreadUtil.javato accept anIConfigurationparameter and usegetExecutorServiceFactory().create()for executor service creation is a positive change that aligns with the PR's objectives. This modification enhances the configurability and encapsulation of thread pool creation, allowing for more flexible thread management in TestNG.The changes are correctly implemented, and the use of
IConfigurationto obtain the executor service factory is a good practice that promotes consistency and flexibility in the framework's configuration management.
- 88-94: The
createExecutormethod inThreadUtil.javahas been refactored to use theIConfigurationparameter for obtaining the executor service factory, which is a consistent and flexible approach to creating executor services in TestNG. This change supports the PR's objective of enhancing thread management through custom executor service factories.The implementation is correct, and the method provides a clear and concise way to create executor services based on the provided configuration. This is a valuable addition to TestNG's internal utilities.
testng-core/src/main/java/org/testng/internal/invokers/MethodInvocationHelper.java (6)
- 33-33: The addition of the
IConfigurationimport statement is necessary for the changes made in this file to support custom executor services. This change is consistent with the PR's objective to enhance thread management in TestNG.- 67-68: Adding
IConfiguration configas a parameter toinvokeMethodConsideringTimeoutis a significant change. It ensures that custom executor services can be utilized when invoking methods with a timeout. This change aligns with the PR's goal but requires careful consideration of backward compatibility and potential impacts on existing code that uses this method.- 73-73: The update to pass the
configparameter toinvokeWithTimeoutis necessary for the integration of custom executor services. This change ensures that the configuration is available when creating executor services, which is crucial for the flexibility introduced by this PR.- 286-290: The modification of
invokeWithTimeoutto includeIConfiguration configin its method signature is essential for supporting custom executor services. This change allows the method to access the configuration needed to create or manage executor services according to the user's specifications.- 310-311: The change to pass the
configurationparameter toinvokeWithTimeoutWithNewExecutoris consistent with the overall goal of this PR. It ensures that the custom executor service factory can be utilized when invoking methods with a new executor, enhancing the flexibility and control over thread management.- 383-390: The addition of
IConfiguration configurationas a parameter toinvokeWithTimeoutWithNewExecutorand the subsequent use of this configuration to create an executor service viaThreadUtil.createExecutoris a key part of enabling custom executor services. This change directly supports the PR's objective by allowing for dynamic adjustment of thread counts based on the provided configuration.testng-core/src/main/java/org/testng/SuiteRunner.java (1)
- 442-446: The addition of the
configurationparameter to theThreadUtil.execute()call in therunInParallelTestMode()method is a crucial change that enables the customization of thread pool executors in TestNG. This change appears to be correctly implemented and logically fits within the broader context of the SuiteRunner class and the PR's objectives. It's important to ensure that theconfigurationobject passed here is always properly initialized and contains the necessary information for creating custom executor services. Additionally, this change enhances the maintainability and modularity of the SuiteRunner class by allowing for more flexible thread management. Overall, this modification aligns well with the goal of enabling dynamic adjustment of thread counts based on resource availability and represents a positive step towards future-proofing TestNG's thread management capabilities.testng-core/src/main/java/org/testng/internal/invokers/ConfigInvoker.java (1)
- 400-400: The addition of
m_configurationas a parameter to theinvokeMethodConsideringTimeoutmethod call withininvokeConfigurationMethodis a crucial change for integrating the newIExecutorServiceFactoryinterface. This change enables the method to utilize the custom executor service configured via the new factory interface, aligning with the PR's objectives to enhance thread management in TestNG.This modification appears to be correctly implemented, adhering to the syntax and logic required for the intended functionality. It's essential to ensure that all instances where
invokeMethodConsideringTimeoutis called are reviewed to maintain consistency across the codebase. Additionally, thorough testing should be conducted to verify that the custom executor services are correctly utilized during configuration method invocations, especially in scenarios involving timeouts.testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (2)
- 380-380: The addition of
m_configurationas a parameter to theThreadUtil.executemethod call is a significant change. It aligns with the PR's objective to enable custom thread pool executors. This change allows the thread pool behavior to be customized based on the configuration, enhancing TestNG's flexibility in thread management. Ensure that all downstream usages ofThreadUtil.executehave been updated to accommodate this new parameter.- 676-676: The inclusion of
m_configurationin theMethodInvocationHelper.invokeWithTimeoutmethod call is another critical update. This change ensures that custom executor services, as specified in them_configuration, are utilized when invoking methods with a timeout. It's essential to verify that theinvokeWithTimeoutmethod has been appropriately modified to handle this new parameter and that its implementation aligns with the intended behavior of using custom executor services.testng-core/src/main/java/org/testng/TestNG.java (3)
- 19-19: The addition of
java.util.Objectsimport is appropriate for the use ofObjects.requireNonNullin the newsetExecutorServiceFactorymethod, ensuring non-null arguments.- 844-846: The new method
setExecutorServiceFactorycorrectly usesObjects.requireNonNullto enforce non-null arguments, aligning with best practices for argument validation. This method allows setting a custom executor service factory, which is central to the PR's objectives.- 1488-1493: The configuration of
IExecutorServiceFactoryviacla.threadPoolFactoryClassis correctly implemented, using reflection to instantiate the specified class. This enables dynamic configuration of the executor service factory, which is a key feature of the PR. However, it's important to ensure that the specified class indeed implementsIExecutorServiceFactoryto avoid runtime errors.CHANGES.txt (1)
- 2-2: The document provides a comprehensive overview of changes made to TestNG over various versions. It's well-organized and presents the information in a clear and chronological manner.
| package test.thread.issue3066; | ||
|
|
||
| import java.util.concurrent.BlockingQueue; | ||
| import java.util.concurrent.ExecutorService; | ||
| import java.util.concurrent.ThreadFactory; | ||
| import java.util.concurrent.TimeUnit; | ||
| import org.testng.IExecutorServiceFactory; | ||
|
|
||
| public class Issue3066ExecutorServiceFactory implements IExecutorServiceFactory { | ||
| @Override | ||
| public ExecutorService create( | ||
| int corePoolSize, | ||
| int maximumPoolSize, | ||
| long keepAliveTime, | ||
| TimeUnit unit, | ||
| BlockingQueue<Runnable> workQueue, | ||
| ThreadFactory threadFactory) { | ||
| return new Issue3066ThreadPoolExecutor( | ||
| corePoolSize, maximumPoolSize, keepAliveTime, unit, workQueue, threadFactory); | ||
| } | ||
| } |
There was a problem hiding this comment.
The implementation of Issue3066ExecutorServiceFactory correctly adheres to the IExecutorServiceFactory interface, ensuring that a custom executor service can be created with the specified parameters. This change aligns with the PR's objective to allow custom thread pool executors to be wired in, addressing the need for dynamic thread management in TestNG.
One minor suggestion for improvement is to add documentation to the create method, explaining the rationale behind choosing Issue3066ThreadPoolExecutor as the executor service implementation. This would enhance the maintainability and understandability of the code for future contributors.
/**
* Creates a custom executor service tailored for handling TestNG's thread management requirements.
* This implementation uses `Issue3066ThreadPoolExecutor` to provide enhanced control over thread
* execution, particularly beneficial for dynamic adjustment of thread counts based on resource availability.
*
* @param corePoolSize the number of threads to keep in the pool, even if they are idle.
* @param maximumPoolSize the maximum number of threads allowed in the pool.
* @param keepAliveTime the maximum time that excess idle threads will wait for new tasks.
* @param unit the time unit for the keepAliveTime argument.
* @param workQueue the queue to use for holding tasks before they are executed.
* @param threadFactory the factory to use when creating new threads.
* @return an instance of `Issue3066ThreadPoolExecutor`.
*/| package test.thread.issue3066; | ||
|
|
||
| import java.util.concurrent.BlockingQueue; | ||
| import java.util.concurrent.ThreadFactory; | ||
| import java.util.concurrent.ThreadPoolExecutor; | ||
| import java.util.concurrent.TimeUnit; | ||
| import org.jetbrains.annotations.NotNull; | ||
|
|
||
| public class Issue3066ThreadPoolExecutor extends ThreadPoolExecutor { | ||
|
|
||
| private static boolean invoked = false; | ||
|
|
||
| public Issue3066ThreadPoolExecutor( | ||
| int corePoolSize, | ||
| int maximumPoolSize, | ||
| long keepAliveTime, | ||
| @NotNull TimeUnit unit, | ||
| @NotNull BlockingQueue<Runnable> workQueue, | ||
| @NotNull ThreadFactory threadFactory) { | ||
| super(corePoolSize, maximumPoolSize, keepAliveTime, unit, workQueue, threadFactory); | ||
| setInvoked(); | ||
| } | ||
|
|
||
| private static void setInvoked() { | ||
| Issue3066ThreadPoolExecutor.invoked = true; | ||
| } | ||
|
|
||
| public static boolean isInvoked() { | ||
| return invoked; | ||
| } | ||
|
|
||
| public static void restInvokedState() { | ||
| invoked = false; | ||
| } |
There was a problem hiding this comment.
The Issue3066ThreadPoolExecutor class extends ThreadPoolExecutor and introduces a mechanism to track if the executor has been invoked, which is a useful feature for testing and validation purposes. The implementation is straightforward and correctly utilizes the @NotNull annotations to ensure non-null parameters for the constructor.
However, there's a typo in the method name restInvokedState which should likely be resetInvokedState. This typo could lead to confusion and should be corrected for clarity.
- public static void restInvokedState() {
+ public static void resetInvokedState() {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| package test.thread.issue3066; | |
| import java.util.concurrent.BlockingQueue; | |
| import java.util.concurrent.ThreadFactory; | |
| import java.util.concurrent.ThreadPoolExecutor; | |
| import java.util.concurrent.TimeUnit; | |
| import org.jetbrains.annotations.NotNull; | |
| public class Issue3066ThreadPoolExecutor extends ThreadPoolExecutor { | |
| private static boolean invoked = false; | |
| public Issue3066ThreadPoolExecutor( | |
| int corePoolSize, | |
| int maximumPoolSize, | |
| long keepAliveTime, | |
| @NotNull TimeUnit unit, | |
| @NotNull BlockingQueue<Runnable> workQueue, | |
| @NotNull ThreadFactory threadFactory) { | |
| super(corePoolSize, maximumPoolSize, keepAliveTime, unit, workQueue, threadFactory); | |
| setInvoked(); | |
| } | |
| private static void setInvoked() { | |
| Issue3066ThreadPoolExecutor.invoked = true; | |
| } | |
| public static boolean isInvoked() { | |
| return invoked; | |
| } | |
| public static void restInvokedState() { | |
| invoked = false; | |
| } | |
| public static void resetInvokedState() { | |
| invoked = false; | |
| } |
| IInjectorFactory getInjectorFactory(); | ||
|
|
||
| IExecutorFactory getExecutorFactory(); | ||
| IExecutorServiceFactory getExecutorServiceFactory(); | ||
|
|
||
| IInjectorFactory getInjectorFactory(); | ||
| void setExecutorServiceFactory(IExecutorServiceFactory factory); |
There was a problem hiding this comment.
The addition of getInjectorFactory and setExecutorServiceFactory methods to the IConfiguration interface aligns with the PR's objectives to enhance TestNG's configurability and flexibility in thread management. These changes enable the use of custom executor service factories, which is a significant improvement for users needing dynamic thread management based on resource availability.
It's important to ensure that the implementation of these methods in concrete classes properly handles null values and adheres to the contract defined by the interface. Additionally, providing documentation for these new methods would improve their understandability and ease of use.
Consider adding documentation to these methods to explain their purpose and usage within the TestNG framework.
| throw new UnsupportedOperationException("This is NO LONGER Supported in TestNG"); | ||
| } else { | ||
| service = | ||
| new ThreadPoolExecutor( | ||
| threadPoolSize, | ||
| threadPoolSize, | ||
| Integer.MAX_VALUE, | ||
| TimeUnit.MILLISECONDS, | ||
| queue, | ||
| new TestNGThreadFactory(name)); | ||
| this.configuration | ||
| .getExecutorServiceFactory() | ||
| .create( | ||
| threadPoolSize, | ||
| threadPoolSize, | ||
| Integer.MAX_VALUE, | ||
| TimeUnit.MILLISECONDS, | ||
| queue, | ||
| new TestNGThreadFactory(name)); |
There was a problem hiding this comment.
The changes in SuiteTaskExecutor.java to replace direct ThreadPoolExecutor instantiation with calls to configuration.getExecutorServiceFactory().create() are in line with the PR's objectives to allow custom executor service factories. This enhances TestNG's flexibility and configurability in thread management.
However, the introduction of UnsupportedOperationException at line 42 indicates a significant change in functionality. It's crucial to ensure that this exception is appropriately documented and communicated to users, as it represents a departure from previous behavior.
Consider adding documentation or comments explaining the rationale behind throwing UnsupportedOperationException and how users can adapt to this change.
| @Test(description = "GITHUB-3066") | ||
| public void ensureCanWireInCustomExecutorServiceWhenEnabledViaConfigParam() { | ||
| String[] args = { | ||
| "-testclass", | ||
| TestClassSample.class.getName(), | ||
| "-threadpoolfactoryclass", | ||
| Issue3066ExecutorServiceFactory.class.getName(), | ||
| "-parallel", | ||
| "methods" | ||
| }; | ||
| TestNG.privateMain(args, null); | ||
| assertThat(Issue3066ThreadPoolExecutor.isInvoked()).isTrue(); | ||
| } | ||
|
|
||
| @Test(description = "GITHUB-3066") | ||
| public void ensureCanWireInCustomExecutorServiceWhenEnabledViaAPI() { | ||
| TestNG testng = create(TestClassSample.class); | ||
| testng.setExecutorServiceFactory(new Issue3066ExecutorServiceFactory()); | ||
| testng.setParallel(XmlSuite.ParallelMode.METHODS); | ||
| testng.run(); | ||
| assertThat(Issue3066ThreadPoolExecutor.isInvoked()).isTrue(); | ||
| } | ||
|
|
||
| @Test(description = "GITHUB-3066") | ||
| public void ensureCanWireInCustomExecutorServiceWhenEnabledViaAPIForMultipleSuites() { | ||
| XmlSuite xmlSuite1 = createXmlSuite("suite1", "test1", TestClassSample.class); | ||
| XmlSuite xmlSuite2 = createXmlSuite("suite2", "test2", TestClassSample.class); | ||
| TestNG testng = create(); | ||
| testng.setXmlSuites(List.of(xmlSuite1, xmlSuite2)); | ||
| testng.setSuiteThreadPoolSize(2); | ||
| testng.setExecutorServiceFactory(new Issue3066ExecutorServiceFactory()); | ||
| testng.run(); | ||
| assertThat(Issue3066ThreadPoolExecutor.isInvoked()).isTrue(); | ||
| } | ||
|
|
||
| @Test(description = "GITHUB-3066") | ||
| public void ensureCanWireInCustomExecutorServiceWhenEnabledViaConfigForMultipleSuites() { | ||
| AtomicInteger counter = new AtomicInteger(1); | ||
| List<String> suites = new ArrayList<>(); | ||
| File dir = createDirInTempDir("suites"); | ||
| Stream.of(TestClassSample.class, TestClassSample.class) | ||
| .map( | ||
| it -> createXmlSuite("suite-" + counter.get(), "test-" + counter.getAndIncrement(), it)) | ||
| .map(XmlSuite::toXml) | ||
| .forEach( | ||
| it -> { | ||
| Path s1 = Paths.get(dir.getAbsolutePath(), UUID.randomUUID() + "-suite.xml"); | ||
| try { | ||
| Files.writeString(s1, it); | ||
| suites.add(s1.toFile().getAbsolutePath()); | ||
| } catch (IOException ignored) { | ||
| } | ||
| }); | ||
|
|
||
| List<String> args = | ||
| List.of( | ||
| "-threadpoolfactoryclass", | ||
| Issue3066ExecutorServiceFactory.class.getName(), | ||
| "-suitethreadpoolsize", | ||
| "2"); | ||
| TestNG.privateMain(Lists.merge(suites, args).toArray(String[]::new), null); | ||
| assertThat(Issue3066ThreadPoolExecutor.isInvoked()).isTrue(); | ||
| } | ||
|
|
||
| @AfterMethod | ||
| public void resetState() { | ||
| Issue3066ThreadPoolExecutor.restInvokedState(); |
There was a problem hiding this comment.
The test cases in CustomExecutorServiceFactoryTest.java effectively validate the functionality of wiring in custom executor services through configuration parameters, API calls, and for multiple suites. These tests are crucial for ensuring the new functionality works as expected and provides a solid foundation for future enhancements or modifications.
One suggestion for improvement is to include assertions or validations after the TestNG.privateMain and testng.run() calls in the tests to ensure that the custom executor service not only gets invoked but also behaves as expected during test execution.
Consider adding more detailed assertions or validations to verify the behavior of the custom executor service beyond its invocation.
| } | ||
|
|
||
| @Override | ||
| public void setExecutorFactory(IExecutorFactory factory) { | ||
| this.m_executorFactory = factory; | ||
| public void setExecutorServiceFactory(IExecutorServiceFactory executorServiceFactory) { | ||
| this.executorServiceFactory = Objects.requireNonNull(executorServiceFactory); | ||
| } | ||
|
|
||
| @Override | ||
| public IExecutorFactory getExecutorFactory() { | ||
| return this.m_executorFactory; | ||
| public IExecutorServiceFactory getExecutorServiceFactory() { | ||
| return executorServiceFactory; | ||
| } | ||
|
|
||
| @Override |
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 [38-155]
The changes in Configuration.java to replace IExecutorFactory with IExecutorServiceFactory and update the implementation accordingly are in line with the PR's objectives to enhance TestNG's thread management capabilities. The use of Objects.requireNonNull in the setExecutorServiceFactory method ensures that a non-null executor service factory is always set, which is a good practice for avoiding null-related issues.
However, the default assignment of ThreadPoolExecutor::new to executorServiceFactory at line 38 might not be fully aligned with the goal of allowing custom executor services, as it directly ties the configuration to a specific implementation of ExecutorService. It would be beneficial to consider a more flexible default that encourages the use of custom executor services.
Consider revising the default assignment for executorServiceFactory to encourage or facilitate the use of custom executor services, possibly by providing a more generic or abstract default implementation.
krmahadevan
force-pushed
the
fix/3066
branch
from
1efd809
to
cf6cddd
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
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 (16)
- CHANGES.txt (1 hunks)
- testng-core-api/src/main/java/org/testng/IExecutorServiceFactory.java (1 hunks)
- testng-core/src/main/java/org/testng/SuiteRunner.java (1 hunks)
- testng-core/src/main/java/org/testng/SuiteTaskExecutor.java (2 hunks)
- testng-core/src/main/java/org/testng/TestNG.java (7 hunks)
- testng-core/src/main/java/org/testng/TestTaskExecutor.java (3 hunks)
- testng-core/src/main/java/org/testng/internal/Configuration.java (4 hunks)
- testng-core/src/main/java/org/testng/internal/IConfiguration.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/ConfigInvoker.java (1 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/MethodInvocationHelper.java (5 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/thread/ThreadUtil.java (4 hunks)
- testng-core/src/test/java/test/thread/CustomExecutorServiceFactoryTest.java (1 hunks)
- testng-core/src/test/java/test/thread/issue3066/Issue3066ExecutorServiceFactory.java (1 hunks)
- testng-core/src/test/java/test/thread/issue3066/Issue3066ThreadPoolExecutor.java (1 hunks)
- testng-core/src/test/java/test/thread/issue3066/TestClassSample.java (1 hunks)
Files skipped from review as they are similar to previous changes (14)
- CHANGES.txt
- testng-core-api/src/main/java/org/testng/IExecutorServiceFactory.java
- testng-core/src/main/java/org/testng/SuiteRunner.java
- testng-core/src/main/java/org/testng/SuiteTaskExecutor.java
- testng-core/src/main/java/org/testng/internal/Configuration.java
- testng-core/src/main/java/org/testng/internal/IConfiguration.java
- testng-core/src/main/java/org/testng/internal/invokers/ConfigInvoker.java
- testng-core/src/main/java/org/testng/internal/invokers/MethodInvocationHelper.java
- testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java
- testng-core/src/main/java/org/testng/internal/thread/ThreadUtil.java
- testng-core/src/test/java/test/thread/CustomExecutorServiceFactoryTest.java
- testng-core/src/test/java/test/thread/issue3066/Issue3066ExecutorServiceFactory.java
- testng-core/src/test/java/test/thread/issue3066/Issue3066ThreadPoolExecutor.java
- testng-core/src/test/java/test/thread/issue3066/TestClassSample.java
Additional comments: 7
testng-core/src/main/java/org/testng/TestTaskExecutor.java (2)
- 57-65: The refactoring to use
configuration.getExecutorServiceFactory().create()for creating executor services is a significant improvement, offering greater flexibility and configurability. This change aligns with the PR's objectives and enhances TestNG's thread management capabilities. Ensure that the new approach is thoroughly tested, especially in scenarios where global thread pools are reused or newly created, to prevent any unforeseen issues.- 3-8: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [76-89]
The
awaitCompletionmethod's implementation, which awaits the completion of all tasks within the executor service and handles interruptions, remains crucial for ensuring tests do not proceed until all tasks are completed or the specified timeout is reached. This method appears to be correctly implemented, but it's important to verify that the new executor service creation and management logic does not impact its behavior, especially in terms of handling timeouts and interruptions.testng-core/src/main/java/org/testng/TestNG.java (5)
- 19-19: The addition of
java.util.Objectsimport is appropriate for the new null checks introduced in thesetExecutorServiceFactorymethod.- 844-846: The
setExecutorServiceFactorymethod correctly requires a non-nullIExecutorServiceFactoryinstance, ensuring that the TestNG configuration will always have a valid executor service factory set. This is a good use ofObjects.requireNonNullto enforce method contract requirements.- 1488-1493: The parsing and setting of the
threadPoolFactoryClassfrom the command line arguments to configure the executor service factory is correctly implemented. It uses reflection to instantiate the factory class based on its name, ensuring flexibility in specifying custom executor service factories. However, it's important to ensure that the specified class indeed implementsIExecutorServiceFactoryto avoid class cast exceptions at runtime.- 1494-1494: It's good to see the new
setExecutorServiceFactorymethod being integrated into the configuration process based on command line arguments. This allows users to easily specify their custom executor service factory when running tests from the command line, enhancing TestNG's flexibility in thread management.- 844-846: Ensure that the
setExecutorServiceFactorymethod's integration into the TestNG lifecycle does not inadvertently replace or conflict with any existing executor service configurations, especially in complex setups with multiple suites or parallel execution modes.
| throw new UnsupportedOperationException("This is NO LONGER Supported in TestNG"); | ||
|
|
||
| } else { | ||
| boolean reUse = xmlTest.getSuite().useGlobalThreadPool(); | ||
| Supplier<Object> supplier = | ||
| () -> | ||
| new ThreadPoolExecutor( | ||
| threadCount, | ||
| threadCount, | ||
| 0, | ||
| TimeUnit.MILLISECONDS, | ||
| queue, | ||
| new TestNGThreadFactory(name)); | ||
| configuration | ||
| .getExecutorServiceFactory() | ||
| .create( | ||
| threadCount, | ||
| threadCount, | ||
| 0, | ||
| TimeUnit.MILLISECONDS, | ||
| queue, | ||
| new TestNGThreadFactory(name)); |
There was a problem hiding this comment.
The introduction of UnsupportedOperationException at line 51 effectively communicates the deprecation of custom thread pool executors in favor of the new executor service factory approach. However, it's crucial to ensure this change is well-documented and communicated to users to avoid confusion and facilitate a smooth transition. Consider adding documentation or comments explaining why UnsupportedOperationException is thrown and guiding users on how to adapt to this change.
| @@ -51,31 +48,21 @@ public void execute() { | |||
| String name = "test-" + xmlTest.getName(); | |||
| int threadCount = Math.max(xmlTest.getThreadCount(), 1); | |||
| if (RuntimeBehavior.favourCustomThreadPoolExecutor()) { | |||
There was a problem hiding this comment.
Same here. Clean the code by removing the now useless runtime behavior
There was a problem hiding this comment.
@juherr - Are you saying that we can get rid of this entire if condition across the board (both at test level and suite level wherein we error out if the user tries to use the earlier deprecated GraphThreadPoolExecutor) ?
There was a problem hiding this comment.
Yes because it is 2 different logics with different responsability
Closes testng-team#3066 Following should be done. * Implement `org.testng.IExecutorServiceFactory` * plugin the fully qualified class name of the above implementation via the configuration parameter "-threadpoolfactoryclass" If using Maven surefire plugin then it can be wired in as below: <configuration> <properties> <property> <name>threadpoolfactoryclass</name> <value>test.thread.MyExecutorServiceFactory</value> </property> </properties> </configuration> If using TestNG API, then it can be wired in as below: TestNG testng = new TestNG(); testng.setExecutorServiceFactory(new MyExecutorServiceFactory());
krmahadevan
force-pushed
the
fix/3066
branch
from
cf6cddd
to
1e943fc
Compare
|
@juherr - please check now |
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/testng.xmlis excluded by:!**/*.xml
Files selected for processing (19)
- CHANGES.txt (1 hunks)
- testng-core-api/src/main/java/org/testng/IExecutorServiceFactory.java (1 hunks)
- testng-core-api/src/main/java/org/testng/internal/RuntimeBehavior.java (1 hunks)
- testng-core/src/main/java/org/testng/SuiteRunner.java (1 hunks)
- testng-core/src/main/java/org/testng/SuiteTaskExecutor.java (2 hunks)
- testng-core/src/main/java/org/testng/TestNG.java (9 hunks)
- testng-core/src/main/java/org/testng/TestTaskExecutor.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/Configuration.java (4 hunks)
- testng-core/src/main/java/org/testng/internal/IConfiguration.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/ConfigInvoker.java (1 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/MethodInvocationHelper.java (5 hunks)
- testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java (2 hunks)
- testng-core/src/main/java/org/testng/internal/thread/ThreadUtil.java (4 hunks)
- testng-core/src/test/java/test/dataprovider/DataProviderTest.java (2 hunks)
- testng-core/src/test/java/test/thread/CustomExecutorServiceFactoryTest.java (1 hunks)
- testng-core/src/test/java/test/thread/SharedThreadPoolTest.java (2 hunks)
- testng-core/src/test/java/test/thread/issue3066/Issue3066ExecutorServiceFactory.java (1 hunks)
- testng-core/src/test/java/test/thread/issue3066/Issue3066ThreadPoolExecutor.java (1 hunks)
- testng-core/src/test/java/test/thread/issue3066/TestClassSample.java (1 hunks)
Files skipped from review as they are similar to previous changes (14)
- CHANGES.txt
- testng-core-api/src/main/java/org/testng/IExecutorServiceFactory.java
- testng-core/src/main/java/org/testng/SuiteRunner.java
- testng-core/src/main/java/org/testng/SuiteTaskExecutor.java
- testng-core/src/main/java/org/testng/internal/Configuration.java
- testng-core/src/main/java/org/testng/internal/IConfiguration.java
- testng-core/src/main/java/org/testng/internal/invokers/ConfigInvoker.java
- testng-core/src/main/java/org/testng/internal/invokers/MethodInvocationHelper.java
- testng-core/src/main/java/org/testng/internal/invokers/TestInvoker.java
- testng-core/src/main/java/org/testng/internal/thread/ThreadUtil.java
- testng-core/src/test/java/test/thread/CustomExecutorServiceFactoryTest.java
- testng-core/src/test/java/test/thread/issue3066/Issue3066ExecutorServiceFactory.java
- testng-core/src/test/java/test/thread/issue3066/Issue3066ThreadPoolExecutor.java
- testng-core/src/test/java/test/thread/issue3066/TestClassSample.java
Additional comments: 10
testng-core/src/test/java/test/thread/SharedThreadPoolTest.java (3)
- 9-14: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-58]
The removal of the test method
ensureErrorShownWhenUsedWithGraphThreadPoolExecutoraligns with the PR's objective to allow custom thread pool executors. However, it's crucial to ensure that the removal doesn't leave a gap in error handling tests related to thread pool usage. Consider adding alternative tests if the error scenarios are still relevant with the new implementation.
- 9-11: The
cleanupmethod effectively clears the shared state inTestClassSample.threadsafter each test method. This is a good practice to prevent test interference and ensure each test runs in a clean state.- 37-42: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [13-58]
The tests
ensureCommonThreadPoolIsUsed,ensureCommonThreadPoolIsNotUsed,ensureCommonThreadPoolIsUsedWhenUsedWithSuiteFiles, andensureCommonThreadPoolIsNotUsedWhenUsedWithSuiteFilesprovide a comprehensive check on the shared and non-shared thread pool behavior. It's important to ensure these tests cover all new scenarios introduced by the PR, especially regarding the dynamic adjustment of thread counts and the use of custom executor services.testng-core/src/main/java/org/testng/TestTaskExecutor.java (3)
- 49-69: The refactoring in the
executemethod to use theIExecutorServiceFactoryfor creating theExecutorServiceis a significant improvement. It aligns with the PR's objective to allow custom thread pool executors. However, ensure that the transition from the previous implementation to this new approach does not introduce any regressions in thread management and execution behavior. It would be beneficial to add or update unit tests specifically targeting this new logic.- 62-63: The use of
ObjectBagto conditionally reuse the global thread pool (ExecutorService) based on theuseGlobalThreadPoolflag is a clever approach. It ensures that the sameExecutorServicecan be shared across different parts of the TestNG framework, potentially improving resource utilization. Just ensure that this shared usage does not lead to any unintended side effects, especially in concurrent execution scenarios.- 67-69: The instantiation and execution of
GraphOrchestratorusing the newly created or reusedExecutorServiceis a key part of this change. This approach allows for more flexible and dynamic thread management. It's important to verify that theGraphOrchestratorbehaves as expected with both shared and custom executor services, particularly in terms of task execution order and concurrency control.testng-core-api/src/main/java/org/testng/internal/RuntimeBehavior.java (1)
- 29-34: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-103]
The removal of the
favourCustomThreadPoolExecutor()method fromRuntimeBehavior.javais consistent with the PR's direction to deprecate direct usage of customThreadPoolExecutorin favor of a more flexible executor service factory approach. Ensure that this change does not leave any orphaned references or configurations that might have depended on the now-removed method. It might be worth conducting a codebase-wide search to confirm this.Verification successful
The search for references to the removed method
favourCustomThreadPoolExecutorin the codebase did not yield any results. This suggests that the removal of the method is consistent with the intended refactoring and likely does not leave any orphaned references or configurations that depended on it. However, it's important to consider the possibility of indirect dependencies or impacts on external configurations or documentation that were not captured by this search. Given the information available, the removal appears to be safe, but further verification might be needed for a comprehensive assessment.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any references to the removed method across the codebase. rg "favourCustomThreadPoolExecutor"Length of output: 35
testng-core/src/main/java/org/testng/TestNG.java (3)
- 19-19: The addition of
java.util.Objectsimport is appropriate given its usage in the newly addedsetExecutorServiceFactorymethod to ensure the provided factory is not null.- 844-846: The new method
setExecutorServiceFactorycorrectly sets a custom executor service factory via the configuration. UsingObjects.requireNonNullensures that a null factory is not accepted, which is a good practice for avoiding null-related issues later in the execution.- 1479-1485: The logic to dynamically set the executor service factory based on the class name provided via command line arguments is correctly implemented. It uses reflection to instantiate the factory class and then sets it using
setExecutorServiceFactory. This approach allows for flexibility and customization of the thread pool executor by the end-users.
The tests were removed because I got rid of the edit check that checks if GraphThreadPoolExecutor (which we deprecated in last release and which is removed in this PR) is being used in conjunction with shared thread pools feature. Since the edit check itself is gone, the removed tests start failing and so they were removed. |
|
No problem to remove them but it changes the behavior and it should be documented in the changelog. |
Closes #3066
Following should be done.
org.testng.IExecutorServiceFactory-threadpoolfactoryclassIf using Maven surefire plugin then it can be wired in as below:
If using TestNG API, then it can be wired in as
below:
Fixes #3066
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
ExecutorServiceto optimize parallel test execution performance.