Add suite thread pool size testng option #29211

SergeyGotovskiy · 2024-05-18T12:15:25Z

Signed-off-by: sergey gotovsky thomas56miller@gmail.com

Fixes #20773

Context
TestNg have an option to run xml suites in parallel. It's very useful feature for project with big count tests stored in xml (with other params like parallel/threads-count/parameter/etc), but we still can't set up this option via gradle. In this pr I try to add this option.

Contributor Checklist

Review Contribution Guidelines.
Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
Make sure all contributed code can be distributed under the terms of the Apache License 2.0, e.g. the code was written by yourself or the original code is licensed under a license compatible to Apache License 2.0.
Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team.
Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective.
Provide unit tests (under <subproject>/src/test) to verify logic.
Update User Guide, DSL Reference, and Javadoc for public-facing changes.
Ensure that tests pass sanity check: ./gradlew sanityCheck.
Ensure that tests pass locally: ./gradlew <changed-subproject>:quickTest.

Reviewing cheatsheet

Before merging the PR, comments starting with

❌ ❓must be fixed
🤔 💅 should be fixed
💭 may be fixed
🎉 celebrate happy things

Signed-off-by: sergey gotovsky <thomas56miller@gmail.com>

…ng-option # Conflicts: # subprojects/plugins/src/test/groovy/org/gradle/api/tasks/testing/testng/TestNGOptionsTest.groovy # subprojects/testing-jvm/src/integTest/groovy/org/gradle/testing/testng/AbstractTestNGVersionIntegrationTest.groovy # subprojects/testing-jvm/src/integTest/groovy/org/gradle/testing/testng/TestNGFailurePolicyIntegrationTest.groovy # subprojects/testing-jvm/src/main/java/org/gradle/api/internal/tasks/testing/testng/TestNGSpec.java # subprojects/testing-jvm/src/main/java/org/gradle/api/internal/tasks/testing/testng/TestNGTestClassProcessor.java # subprojects/testing-jvm/src/main/java/org/gradle/api/tasks/testing/testng/TestNGOptions.java

…ption

Signed-off-by: sergey gotovsky <thomas56miller@gmail.com>

This is consolidate how the location of the directory is handled between a PersistentCache and a SingleDepthFileAccessTracker. Signed-off-by: sergey gotovsky <thomas56miller@gmail.com>

Signed-off-by: sergey gotovsky <thomas56miller@gmail.com>

- replace `@IgnoreIf` by `@Requires` Signed-off-by: sergey gotovsky <thomas56miller@gmail.com>

Signed-off-by: sergey gotovsky <thomas56miller@gmail.com>

tresat

Thank you for your contribution to Gradle!

This change looks reasonable and should be able to be accepted soon once the few flagged issues are addressed. Please let us know if you need any help.

...jvm/testing-jvm/src/test/groovy/org/gradle/api/tasks/testing/testng/TestNGOptionsTest.groovy

platforms/jvm/testing-jvm/src/main/java/org/gradle/api/tasks/testing/testng/TestNGOptions.java

tresat · 2024-05-20T14:20:31Z

...ure/src/main/java/org/gradle/api/internal/tasks/testing/testng/TestNGTestClassProcessor.java

+            JavaMethod.of(TestNG.class, Object.class, "setSuiteThreadPoolSize", Integer.class).invoke(testNg, suiteThreadPoolSize);
+        } catch (NoSuchMethodException e) {
+            if (suiteThreadPoolSize < 1) {
+                throw new InvalidUserDataException("The version of TestNG used does not support setting thread pool size.");


❗ This message is incorrect, it should say something about how a size < 1 is invalid.

Please add a test to verify this failure behavior as well.

I agree, initially I misunderstood how it works. I've made the changes and tested them—it looks like it's what we need. Please take a look.

So if we have a version of TestNG that doesn't include this method, but we call it (needlessly) to set the thread pool size to 1, we won't see an error? I don't think this is the behavior we want - we'd always want to see the error in the exception case here. Even if the method would have no effect, users should still be made to remove the call from their build script if their TestNG version doesn't support it.

This would be more in line with the behavior of the other methods here and wouldn't need a full integration test to demo using this.

I understand your point. However, the main reason I did it this way is due to the logic of other methods presented in this class. Let's look at the setPreserveOrder method, for example. It also throws an exception only if there is an attempt to set a value different from the default (false by default), which does not affect the program's execution.

I tested this in a separate project with an older version of TestNG (5.12.1) - I set setPreserveOrder to true and received an exception, but when I set setPreserveOrder to false, there were no exception. This is why I rewrote this method to reflect the behavior of existing methods.

However, I am new here, so if I misunderstood something, please let me know what exactly.

OK, I see. I was comparing to the setSuiteThreadPoolSize method, which I thought might be more relevant due to it also affecting parallelism.

I think this is fine then. Can you please add an integration test that uses an older TestNG version and demonstrates this failure, and then I think this is ready to merge?

Something simple like TestNGGroupByInstancesNotSupportedIntegrationTest might be a good example to copy.

I apologize for the delay. I have added a test that checks for the exception in the older version of TestNG. Please take a look.

No worries! Thank you for your contribution, and for following up.

This all looks great, I'm merging it now to get it added for Gradle 8.9.

…iteThreadPoolSize` validation Signed-off-by: sergey gotovsky <thomas56miller@gmail.com>

…thub.com/SergeyGotovskiy/gradle into add-suite-thread-pool-size-testng-option

gitstream-cm · 2024-05-21T21:47:17Z

📊 Changes by Platform: this PR is 93% new code

2 platforms were affected

See details

Platform	Added Lines	% of Total Line Changes	Deleted Lines	% of Total Line Changes	Files Changed	% of Total Files Changed
null	1	2%	0	0%	1	13%
jvm	52	91%	4	7%	7	88%

SergeyGotovskiy

Thank you very much for your time and the feedback on my PR.
I reviewed and tried to address your comments. Please check if it looks better now.

SergeyGotovskiy · 2024-05-21T21:44:34Z

...ure/src/main/java/org/gradle/api/internal/tasks/testing/testng/TestNGTestClassProcessor.java

+            JavaMethod.of(TestNG.class, Object.class, "setSuiteThreadPoolSize", Integer.class).invoke(testNg, suiteThreadPoolSize);
+        } catch (NoSuchMethodException e) {
+            if (suiteThreadPoolSize < 1) {
+                throw new InvalidUserDataException("The version of TestNG used does not support setting thread pool size.");


I agree, initially I misunderstood how it works. I've made the changes and tested them—it looks like it's what we need. Please take a look.

platforms/jvm/testing-jvm/src/main/java/org/gradle/api/tasks/testing/testng/TestNGOptions.java

...jvm/testing-jvm/src/test/groovy/org/gradle/api/tasks/testing/testng/TestNGOptionsTest.groovy

tresat · 2024-05-22T11:55:13Z

...ure/src/main/java/org/gradle/api/internal/tasks/testing/testng/TestNGTestClassProcessor.java

+            JavaMethod.of(TestNG.class, Object.class, "setSuiteThreadPoolSize", Integer.class).invoke(testNg, suiteThreadPoolSize);
+        } catch (NoSuchMethodException e) {
+            if (suiteThreadPoolSize < 1) {
+                throw new InvalidUserDataException("The version of TestNG used does not support setting thread pool size.");


So if we have a version of TestNG that doesn't include this method, but we call it (needlessly) to set the thread pool size to 1, we won't see an error? I don't think this is the behavior we want - we'd always want to see the error in the exception case here. Even if the method would have no effect, users should still be made to remove the call from their build script if their TestNG version doesn't support it.

This would be more in line with the behavior of the other methods here and wouldn't need a full integration test to demo using this.

tresat · 2024-05-22T23:18:19Z

...ure/src/main/java/org/gradle/api/internal/tasks/testing/testng/TestNGTestClassProcessor.java

+            JavaMethod.of(TestNG.class, Object.class, "setSuiteThreadPoolSize", Integer.class).invoke(testNg, suiteThreadPoolSize);
+        } catch (NoSuchMethodException e) {
+            if (suiteThreadPoolSize < 1) {
+                throw new InvalidUserDataException("The version of TestNG used does not support setting thread pool size.");


OK, I see. I was comparing to the setSuiteThreadPoolSize method, which I thought might be more relevant due to it also affecting parallelism.

I think this is fine then. Can you please add an integration test that uses an older TestNG version and demonstrates this failure, and then I think this is ready to merge?

Something simple like TestNGGroupByInstancesNotSupportedIntegrationTest might be a good example to copy.

Signed-off-by: sergey gotovsky <thomas56miller@gmail.com>

tresat · 2024-05-28T12:27:16Z

@bot-gradle test and merge

bot-gradle · 2024-05-28T12:27:23Z

WARN: Based on labels, this pull request addresses notable issue but no changes to release note found.

SergeyGotovskiy added 3 commits May 13, 2022 12:08

add suiteThreadPoolSize option

8c4d260

Signed-off-by: sergey gotovsky <thomas56miller@gmail.com>

Merge branch 'gradle:master' into add-suite-thread-pool-size-testng-o…

79c530c

…ption

SergeyGotovskiy requested review from a team as code owners May 18, 2024 12:15

SergeyGotovskiy requested a review from tresat May 18, 2024 12:15

bot-gradle added from:contributor PR by an external contributor to-triage labels May 18, 2024

SergeyGotovskiy marked this pull request as draft May 18, 2024 12:16

gitstream-cm bot added the 1 min review label May 18, 2024

SergeyGotovskiy and others added 5 commits May 18, 2024 15:30

add suiteThreadPoolSize option

e6c10d4

Signed-off-by: sergey gotovsky <thomas56miller@gmail.com>

Do not canonicalize path of persistent cache

f5f537d

This is consolidate how the location of the directory is handled between a PersistentCache and a SingleDepthFileAccessTracker. Signed-off-by: sergey gotovsky <thomas56miller@gmail.com>

Fix imports

9b4fd21

Signed-off-by: sergey gotovsky <thomas56miller@gmail.com>

Polish DevelocityBuildLifecycleServiceIntegrationTest

511b4e0

- replace `@IgnoreIf` by `@Requires` Signed-off-by: sergey gotovsky <thomas56miller@gmail.com>

Enable isolatedProjectsIntegTest on :enterprise

4616447

Signed-off-by: sergey gotovsky <thomas56miller@gmail.com>

SergeyGotovskiy force-pushed the add-suite-thread-pool-size-testng-option branch from 44e5f20 to 4616447 Compare May 18, 2024 12:38

gitstream-cm bot added the 5 min review label May 18, 2024

SergeyGotovskiy marked this pull request as ready for review May 18, 2024 12:42

SergeyGotovskiy requested review from ldaley and a team as code owners May 18, 2024 12:42

gitstream-cm bot added build-script-change and removed 1 min review labels May 18, 2024

Merge branch 'master' into add-suite-thread-pool-size-testng-option

57f3eae

gitstream-cm bot added 1 min review and removed 5 min review labels May 18, 2024

tresat self-assigned this May 20, 2024

tresat removed the to-triage label May 20, 2024

tresat requested changes May 20, 2024

View reviewed changes

tresat added this to the 8.9 RC1 milestone May 20, 2024

tresat added the 👋 team-triage Issues that need to be triaged by a specific team label May 20, 2024

big-guy removed the 👋 team-triage Issues that need to be triaged by a specific team label May 20, 2024

SergeyGotovskiy added 2 commits May 22, 2024 00:41

change intValue for copyFromOverridesOldOptions test and fix `setSu…

1f3bb91

…iteThreadPoolSize` validation Signed-off-by: sergey gotovsky <thomas56miller@gmail.com>

Merge branch 'add-suite-thread-pool-size-testng-option' of https://gi…

00cf057

…thub.com/SergeyGotovskiy/gradle into add-suite-thread-pool-size-testng-option

SergeyGotovskiy commented May 21, 2024

View reviewed changes

tresat requested changes May 22, 2024

View reviewed changes

tresat removed the 1 min review label May 24, 2024

add TestNGParallelBySuitesNotSupportedIntegrationTest

86df3b6

Signed-off-by: sergey gotovsky <thomas56miller@gmail.com>

SergeyGotovskiy requested a review from tresat May 27, 2024 20:35

tresat approved these changes May 28, 2024

View reviewed changes

bot-gradle added this pull request to the merge queue May 28, 2024

Merged via the queue into gradle:master with commit ed3724d May 28, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add suite thread pool size testng option #29211

Add suite thread pool size testng option #29211

SergeyGotovskiy commented May 18, 2024 •

edited

tresat left a comment

tresat May 20, 2024

SergeyGotovskiy May 21, 2024

tresat May 22, 2024

SergeyGotovskiy May 22, 2024

tresat May 22, 2024

SergeyGotovskiy May 25, 2024

tresat May 28, 2024

gitstream-cm bot commented May 21, 2024

SergeyGotovskiy left a comment

SergeyGotovskiy May 21, 2024

tresat May 22, 2024

tresat May 22, 2024

tresat commented May 28, 2024

bot-gradle commented May 28, 2024

Add suite thread pool size testng option #29211

Add suite thread pool size testng option #29211

Conversation

SergeyGotovskiy commented May 18, 2024 • edited

Contributor Checklist

Reviewing cheatsheet

tresat 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

gitstream-cm bot commented May 21, 2024

SergeyGotovskiy 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

tresat commented May 28, 2024

bot-gradle commented May 28, 2024

SergeyGotovskiy commented May 18, 2024 •

edited