Support suite level thread pools for data provider #2982

krmahadevan · 2023-09-19T16:48:52Z

Closes #2980

We can now configure TestNG such that it uses a
Suite level thread pool when running data driven
Tests in parallel.

This can be enabled via the configuration

“-shareThreadPoolForDataProviders”
with a value of “true”

Alternatively one can also use the suite level
attribute “share-thread-pool-for-data-providers”

Fixes #2980 .

Did you remember to?

Add test case(s)
Update CHANGES.txt
Auto applied styling via ./gradlew autostyleApply

We encourage pull requests that:

Add new features to TestNG (or)
Fix bugs in TestNG

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.

juherr

The change is ok except the new public api and the dtd which should be upgraded

juherr · 2023-09-19T16:52:29Z

testng-core/src/main/resources/testng-1.0.dtd

@@ -73,6 +75,7 @@ Cedric Beust & Alexandru Popescu
    time-out CDATA #IMPLIED
    skipfailedinvocationcounts (true | false) "false"
    data-provider-thread-count CDATA "10"
+    share-thread-pool-for-data-providers (true | false) "false"


We should release a new version of the dtd.

Sure. Can you please guide me on the following?

When do we release a new dtd version? That way we have a process that we can follow whenever there are dtd changes.

Assuming that we have the process defined, how do we go about deploying the new dtd.

Since a user can still have their suite xml dtd point to the newer one, how would the new dtd publishing help?

DTD (or XSD) is a contract like an interface.
If you want to modify it, you should release a new version and be able to manage all versions in the XML reader.

@juherr - Sure. I understand the intent of the DTD/XSD. But what purpose does this solve for TestNG is what I am trying to understand. Our users are mostly going to be on the same XSD (asking them to go update all the suite files with pointing it to a newer version is kind of like a sub-optimal user experience for me). The XSD exists only because we would like to validate our xml schema and it really doesn't serve any functional purpose from the user's perspective. So why would they want to update. So eventually what benefit does TestNG have by doing this? On the other hand, if we just go and update the same XSD with newer attributes, a user would just need to update to the newer TestNG version that is aware of the newer attributes and this will just work for a user. This is what my concern is.

testng-core-api/src/main/java/org/testng/IObjectBag.java

juherr · 2023-09-19T16:58:00Z

testng-core/src/main/java/org/testng/internal/invokers/MethodRunner.java

+    IObjectBag objectBag = context.getSuite().getObjectBag();
+    boolean sharedThreadPool = context.getSuite().getXmlSuite().isShareThreadPoolForDataProviders();
+
+    @SuppressWarnings("unchecked")


I don't understand why the warning cannot be fixed

Any suggestions on how to fix this? I am basically trying to figure out how to push in a PoolService<List<ITestResult>> using generics. I can only replace PoolService with T. I am not sure how to represent the List<ITestResult> here.

Try replacing the diamond operators with the real value.
The inference is maybe not working as expected here.
And maybe it is a javac issue you should try to reproduce in its own independent context and write an issue ;)

This is now removed.

I'm still seing it 😉

The @SuppressWarnings cannot be removed because the ObjectBag stores objects and I cannot make it store specific types. I did this because I hope to use the ObjectBag as a generic bean bag sort of place.

juherr · 2023-09-19T17:02:18Z

testng-core/src/test/java/test/dataprovider/DataProviderTest.java

+  public Object[][] getSuiteFileNames() {
+    return new Object[][] {
+      {
+        "src/test/resources/2980_with_shared_threadpool_enabled.xml",


Be careful. It only works because resources are not filtered. Otherwise they should be taken from the build context.

Can you please tell me how to do this via the build context?

Also I think this is the usual convention being followed for the other tests that have existed in the codebase. Would we need filtering on test resources? I thought that made sense when we apply filtering on main resources.

The resources are supposed to be copied into the target folder by the resource plugin which can modify them during the operation.
I prefer to read resources from the target folder then.

@juherr - I am not sure why would the resource plugin have to filter out resources (I don't see a use case for having to do this).
There are many tests that just load resources from the file system and work with it. So trying to understand as to why do we need to complicate things by having it loaded from a class path as a stream and then translate that to a file before working with it.

testng-core/src/test/java/test/dataprovider/DataProviderTest.java

testng-core/src/test/java/test/dataprovider/issue2980/LoggingListener.java

juherr · 2023-09-22T15:03:30Z

@krmahadevan Please, let me few days to elaborate my answers

krmahadevan · 2023-09-22T15:08:38Z

Sure @juherr i will wait to hear back from you.

dr29bart · 2023-09-22T20:16:45Z

@krmahadevan

how -dataproviderthreadcount will affect thread pool size with -shareThreadPoolForDataProviders=true?
Does it make sense to use enum for the pool type similar to FailurePolicy? It will be something like threadPoolForDataProviders
1. ISOLATED\LEGACY - there will be a separate thread pool for each test with a data provider(parallel=true)
2. COMMON - there will be an extra thread pool for all tests with a data provider (parallel=true)
3. SHARED - all (with and without data provider) tests will share the same thread pool

krmahadevan · 2023-09-23T12:36:07Z

@krmahadevan

how -dataproviderthreadcount will affect thread pool size with -shareThreadPoolForDataProviders=true?

This will cause the entire suite to share a common data provider thread pool for all data driven tests within a <suite>. So going by your enum proposition, it now supports the notion of COMMON

Does it make sense to use enum for the pool type similar to FailurePolicy? It will be something like threadPoolForDataProviders

ISOLATED\LEGACY - there will be a separate thread pool for each test with a data provider(parallel=true)

COMMON - there will be an extra thread pool for all tests with a data provider (parallel=true)

SHARED - all (with and without data provider) tests will share the same thread pool

The current implementation does that but without using an enum. The enum approach is lucrative. We could consider exploring it.

Now with respect to SHARED, its going to be a bit more trickier because we are trying to merge two types of thread pools. The regular test method thread pool basically owners the directed acyclic graph that TestNG creates for test dependencies and the one that data provider uses, it doesn't take into account all of that because this would just be one of the nodes in the DAG. Merging them would require a bit more closer look at the codebase.

krmahadevan · 2023-10-22T11:31:43Z

@juherr - ping!

testng-core-api/src/main/java/org/testng/IObjectBag.java

krmahadevan · 2023-11-06T05:45:38Z

@juherr - Please take a look. I have revamped the approach.

Closes testng-team#2980 We can now configure TestNG such that it uses a Suite level thread pool when running data driven Tests in parallel. This can be enabled via the configuration “-shareThreadPoolForDataProviders” with a value of “true” Alternatively one can also use the suite level attribute “share-thread-pool-for-data-providers”

juherr

Lgtm with some risks on dtd

juherr · 2023-11-28T06:28:27Z

testng-core-api/src/main/java/org/testng/xml/XmlSuite.java

@@ -123,6 +124,9 @@ public String toString() {
  private String m_parentModule = "";
  private String m_guiceStage = "";

+  /** Represents a unique id for this suite. Can be used for uniquely identifying the xml suite. */
+  public final UUID SUITE_ID = UUID.randomUUID();


As not static, in camelCase

I wanted this to stand out as if it is a constant. That was why I intentionally broke the naming conventions and used all CAPS

krmahadevan requested a review from juherr as a code owner September 19, 2023 16:48

juherr requested changes Sep 19, 2023

View reviewed changes

krmahadevan force-pushed the global_threadpool_2980 branch from eed793f to 4c332ba Compare October 1, 2023 03:43

juherr reviewed Nov 3, 2023

View reviewed changes

testng-core-api/src/main/java/org/testng/IObjectBag.java Outdated Show resolved Hide resolved

krmahadevan force-pushed the global_threadpool_2980 branch from 4c332ba to 84fd562 Compare November 6, 2023 05:41

krmahadevan force-pushed the global_threadpool_2980 branch from 84fd562 to 9d7ceb8 Compare November 23, 2023 15:34

krmahadevan requested a review from juherr November 28, 2023 03:32

juherr approved these changes Nov 28, 2023

View reviewed changes

krmahadevan merged commit 7357104 into testng-team:master Nov 28, 2023
8 of 9 checks passed

krmahadevan deleted the global_threadpool_2980 branch November 28, 2023 08:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support suite level thread pools for data provider #2982

Support suite level thread pools for data provider #2982

krmahadevan commented Sep 19, 2023

juherr left a comment

juherr Sep 19, 2023

krmahadevan Sep 19, 2023

juherr Nov 3, 2023

krmahadevan Nov 5, 2023

juherr Sep 19, 2023

krmahadevan Sep 20, 2023

juherr Nov 3, 2023

krmahadevan Nov 6, 2023

juherr Nov 6, 2023

krmahadevan Nov 6, 2023

juherr Sep 19, 2023

krmahadevan Sep 19, 2023

krmahadevan Sep 20, 2023

juherr Nov 3, 2023

krmahadevan Nov 6, 2023

juherr commented Sep 22, 2023

krmahadevan commented Sep 22, 2023

dr29bart commented Sep 22, 2023 •

edited

Loading

krmahadevan commented Sep 23, 2023 •

edited

Loading

krmahadevan commented Oct 22, 2023

krmahadevan commented Nov 6, 2023

juherr left a comment

juherr Nov 28, 2023

krmahadevan Nov 28, 2023

Support suite level thread pools for data provider #2982

Support suite level thread pools for data provider #2982

Conversation

krmahadevan commented Sep 19, 2023

Did you remember to?

juherr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juherr commented Sep 22, 2023

krmahadevan commented Sep 22, 2023

dr29bart commented Sep 22, 2023 • edited Loading

krmahadevan commented Sep 23, 2023 • edited Loading

krmahadevan commented Oct 22, 2023

krmahadevan commented Nov 6, 2023

juherr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr29bart commented Sep 22, 2023 •

edited

Loading

krmahadevan commented Sep 23, 2023 •

edited

Loading