[SUREFIRE-1967] Fix parallel test ordering to prevent high resource consumption #407

findepi · 2021-12-14T16:50:50Z

[ https://issues.apache.org/jira/browse/SUREFIRE-1967 ]

Before the change, TestNG run from Surefire can execute @BeforeClass
on many different test classes/instances, without invoking @AfterClass
yet, leading to high resource utilization. This is not observed when
tests are invoked via a suite file. This is because XmlClass.m_index
field is used by TestNG to order test execution and this field used not
to be set by Surefire. This commit lets Surefire initialize XmlClass
object in a similar manner to how TestNG suite file XML parser does.

Following this checklist to help us incorporate your
contribution quickly and easily:

Make sure there is a JIRA issue filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles,
where you replace SUREFIRE-XXX with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean install to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
You have run the integration tests successfully (mvn -Prun-its clean install).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

findepi · 2021-12-14T16:52:29Z

cc @losipiuk @findinpath

findinpath · 2021-12-14T17:25:35Z

...providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGExecutor.java

+            {
+                throw new RuntimeException( e );
+            }
+        }


shouldn't we have here reflective access for the m_index field as well?

Instead of setting the m_index via reflection we could use the appropriate constructor to set it:

XmlClass(String className, int index, boolean loadClasses) in testng 5.x see https://github.com/cbeust/testng/blob/testng-5.14.1/src/main/java/org/testng/xml/XmlClass.java#L36

XmlClass(String className, int index) in tesng 7.x see https://github.com/cbeust/testng/blob/master/testng-core-api/src/main/java/org/testng/xml/XmlClass.java

The code is compiled against TestNG 5.10 and that version doesn't have the index field in XmlClass, hence use of reflection.

Instead of setting the m_index via reflection we could use the appropriate constructor to set it:

Yes, I can lookup a constructor taking (String.class, int.class) arguments. It's not as robust or future-proof as finding a setIndex method though.

Pls add a line with a comment regarding TestNG 5.10. It was very good point.

@findepi It would be an ideal situation to ensure same behavior in both versions. So in that case the constructor would help us XmlClass(String className, int index, boolean loadClasses). Am I right?

These two constructors handle the same information across versions
XmlClass(String className, int index, boolean loadClasses)
XmlClass(String name, Boolean declaredClass, int index)
@juherr Can you ensure that the constructors, at least two, would not be removed in the future? Difficult question for IT development, I know. :-)

@Tibor17 it's possible to select and invoke one of the constructors, but i don't think the added complexity is worth the effort (i am mostly concerned about future code readability and maintainability). Note that after all, we're solving a problem that exists in TestNG >= 6.3, and TestNG 5.x is not going to change. The m_index field is used in TestNG 5.x when preserve-order is set, but it's not possible to set it when invoking tests via surefire. More precisely, preserve-order seems to require a suite file, but then TestNG will populate the m_index, not us.

Pls add a line with a comment regarding TestNG 5.10. It was very good point.

You mean this one:

The code is compiled against TestNG 5.10 and that version doesn't have the index field in XmlClass, hence use of reflection. ?

I wouldn't want to put exact TestNG 5.10 version in the source, as it's subject to change, but i added explanatory comment above XML_CLASS_SET_INDEX constant:

// Using reflection because XmlClass.setIndex is available since TestNG 6.3 // XmlClass.m_index field is available since TestNG 5.13, but prior to 6.3 required invoking constructor // and constructor XmlClass constructor signatures evolved over time.

i tried to make it convey same meaning: that we have to use the reflection.

findinpath · 2021-12-14T17:53:19Z

Please do take a look at

https://github.com/cbeust/testng/blob/master/testng-core/src/main/java/org/testng/internal/DynamicGraphHelper.java#L82-L89

It seems that the orders of the classes is not preserved when running the tests in parallel.

findepi · 2021-12-14T21:00:47Z

It seems that the orders of the classes is not preserved when running the tests in parallel.

When you run without parallel, i didn't observe high resource consumption at all -- in my simple test project the tests are being setup and torn down in turns, with no two tests being initialized at the same time

I tested the surefire+testng behavior before and after the change, with parallel="methods" and thread count 2.
I used TestNG versions 6.10 (as currently used in Trino), and 7.3.0 (the newest that worked with surefire 3.0.0-M5, surefire 3.0.0-M6 doesn't work with my local maven version). For both testng versions, before the change i would get high number of test classes initialized concurrently, and not torn down yet.

After the change, i wouldn't see more than 2 (test thread count) test classes initialized at the same time.

findepi · 2021-12-14T21:02:57Z

@Tibor17 This is my first contribution here. i've signed & sent the CLA. Would you mind approving the CI to run?

slawekjaranowski · 2021-12-14T21:26:36Z

Does this change has no influences on existing tests?

Can you prepare some test which can confirm this behavior?
Start from unit test.

findepi · 2021-12-15T09:46:12Z

@slawekjaranowski thanks for your feedback

Does this change has no influences on existing tests?

I've completed run of mvn -Prun-its clean install

$ mvn -Prun-its clean install
[....]
[INFO] Reactor Summary for Apache Maven Surefire 3.0.0-M6-SNAPSHOT:
[INFO]
[INFO] Apache Maven Surefire .............................. SUCCESS [  3.785 s]
[INFO] SureFire Logger API ................................ SUCCESS [  3.929 s]
[INFO] SureFire API ....................................... SUCCESS [ 18.091 s]
[INFO] Surefire Extensions API ............................ SUCCESS [  3.599 s]
[INFO] Surefire Extensions SPI ............................ SUCCESS [  1.129 s]
[INFO] SureFire Booter .................................... SUCCESS [ 12.088 s]
[INFO] Maven Surefire Test-Grouping Support ............... SUCCESS [  3.121 s]
[INFO] SureFire Providers ................................. SUCCESS [  0.399 s]
[INFO] Shared JUnit3 Provider Code ........................ SUCCESS [  2.562 s]
[INFO] Shared Java 5 Provider Base ........................ SUCCESS [  2.905 s]
[INFO] Shared JUnit4 Provider Code ........................ SUCCESS [  3.168 s]
[INFO] Shared JUnit48 Provider Code ....................... SUCCESS [  5.048 s]
[INFO] SureFire JUnit Runner .............................. SUCCESS [  2.701 s]
[INFO] SureFire JUnit4 Runner ............................. SUCCESS [  2.712 s]
[INFO] Maven Surefire Common .............................. SUCCESS [ 51.055 s]
[INFO] SureFire JUnitCore Runner .......................... SUCCESS [02:17 min]
[INFO] SureFire JUnit Platform Runner ..................... SUCCESS [  5.813 s]
[INFO] SureFire TestNG Utils .............................. SUCCESS [  2.796 s]
[INFO] SureFire TestNG Runner ............................. SUCCESS [  3.960 s]
[INFO] ShadeFire JUnit3 Provider .......................... SUCCESS [  1.236 s]
[INFO] Surefire Report Parser ............................. SUCCESS [  3.479 s]
[INFO] Maven Surefire Plugin .............................. SUCCESS [  5.551 s]
[INFO] Maven Failsafe Plugin .............................. SUCCESS [ 26.472 s]
[INFO] Maven Surefire Report Plugin ....................... SUCCESS [  8.341 s]
[INFO] Maven Surefire Integration Tests ................... SUCCESS [  01:06 h]
[INFO] Surefire Shared Utils .............................. SUCCESS [  1.649 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  01:11 h
[INFO] Finished at: 2021-12-15T10:20:56+01:00
[INFO] ------------------------------------------------------------------------

What else i should do?

Can you prepare some test which can confirm this behavior?
Start from unit test.

I am not sure how i can test this in a unit manner.

I found tests in https://github.com/apache/maven-surefire/tree/master/surefire-its/src/test/resources and I think i would be able to add a test like that. I already have a local project I am testing with, and would need to clean it up.

@slawekjaranowski please advise.

slawekjaranowski · 2021-12-15T18:31:18Z

Look into org.apache.maven.surefire.its.CheckTestNgGroupThreadParallelIT
and try prepare similar with your case, the most difficult will be prepare assertions ....

findepi · 2021-12-15T19:32:55Z

@slawekjaranowski thanks, this looks awesome. Can you please share the fastest way to run single test either from IDE or command line?

i tried this

mvn -Prun-its clean install -Dtest=CheckTestNgGroupThreadParallelIT -pl :surefire-its

and got

[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.maven.surefire.its.CheckTestNgGroupThreadParallelIT
[INFO] [stdout] Using JAVA_HOME=/Library/Java/JavaVirtualMachines/zulu-11.jdk/Contents/Home in forked launcher.
...
[ERROR] Plugin org.apache.maven.plugins:maven-surefire-plugin:null or one of its dependencies could not be resolved: org.apache.maven.plugins:maven-surefire-plugin:jar:null ...

should i be setting surefire.version by hand?

slawekjaranowski · 2021-12-15T19:50:33Z

try

 mvn verify -pl surefire-its -P run-its -Dit.test=CheckTestNgGroupThreadParallelIT

magic it.test instead of test

findepi · 2021-12-15T19:56:39Z

@slawekjaranowski thanks, added a test.
At least locally, it did fail before the PR's change and succeeded afterwards.

...ire-its/src/test/java/org/apache/maven/surefire/its/CheckTestNgMethodParallelOrderingIT.java

...providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGExecutor.java

findepi · 2021-12-15T20:56:49Z

@slawekjaranowski thanks for review, applied comments

surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/pom.xml

findepi · 2021-12-16T08:52:41Z

@slawekjaranowski thanks for your review, comments applied.
i also added a test run with some older testng version. i determined it's useful, because the solution relies on reflection, so could silently stop working.

findepi · 2021-12-20T09:16:37Z

@slawekjaranowski i've received a confirmation that my signed CLA was received by Apache.
Please let me know what else I should change here.

slawekjaranowski · 2021-12-21T12:58:55Z

@findepi - please follow dev mailing list.

findepi · 2021-12-21T21:46:15Z

@slawekjaranowski do you want me to sign up to Maven Developer List (dev-subscribe@maven.apache.org)?

slawekjaranowski · 2021-12-21T22:41:24Z

simply send email for given address and follow on response.

findepi · 2021-12-22T06:22:13Z

@slawekjaranowski thanks, done

findepi · 2021-12-23T21:31:27Z

@slawekjaranowski should i do any other changes here? is there a chance to get this into 3.0.0-M6?
As we add more tests, we face dilemma whether to increase tests' heap size or reduce parallelism. While GitHub Actions provide something like 7 GB memory, we are heavy testcontainers users (Trino plugins require integration tests), so we need to put aside quite some memory for docker containers. Thus we lean towards removing test parallelism, but this will increase build times. (just sharing for context)

Tibor17 · 2021-12-24T11:28:34Z

@findepi
Pls do not use return XmlClass.class.getMethod( "setIndex", int.class );. We have class something like SurefireReflector and there is setter reflector. It is a utility widely used in Surefire project and it makes the reflection calls very compact without catching exceptions, etc. Thx

Tibor17 · 2021-12-24T11:43:46Z

@findepi
Your commit in this PR is small one and we do not have to require CLA. If you want to provide some invention property to ASF Maven project like a big feature from another project as I did in 2013 then yes I had to sign CLA. But this is a small fix which is not used in another projects for sure because it is not applicable to another projects and where you use Java and Maven API.

findepi · 2021-12-24T11:54:30Z

@Tibor17 thanks for your review.
I didn't notice the ReflectionUtils class even though it's already used in TestNGExecutor. This indeed simplifies the code flow, as it handles reflection-related exceptions. Please take another look.

findepi · 2022-01-08T20:13:06Z

Rebased to rerun the CI. @slawekjaranowski can you please approve the CI run?

findepi · 2022-01-10T12:40:45Z

Apologies, I've accidentally removed the test exclusion with TestNG 5 on JDK 17, so inevitably the build was failing. Brought it back now. The build is green in my fork https://github.com/findepi/maven-surefire/actions/runs/1676804939

@slawekjaranowski can you please approve the build run here?

findepi · 2022-01-11T10:44:12Z

thanks for approving the PR run.
I see that JUnit47ParallelIT.forcedShutdownVerifyingLogs failed in macOS-latest jdk-8-temurin.
I hope that's not related, but I am not sure what I should do. @slawekjaranowski @Tibor17 please advise.

Tibor17 · 2022-01-12T15:16:21Z

JUnit47ParallelIT.forcedShutdownVerifyingLogs is not related to this issue since the IT is not related to TestNG.

Tibor17 · 2022-01-12T15:44:38Z

Matcher matcher = Pattern.compile( "^(\\d+)\\..*$" ).matcher( System.getProperty( "java.version" ) );
         assertTrue( matcher.matches() );
int javaVersionFirstNumber = Integer.parseInt( matcher.group( 1 ) );
         assumeTrue( "TestNG 5.13 does not work with Java 17", javaVersionFirstNumber < 17 );

can be replaced with

import static org.apache.maven.surefire.its.fixture.HelperAssertions.assumeJavaMaxVersion;
assumeJavaMaxVersion( 16 );

findepi · 2022-01-12T16:04:37Z

can be replaced with

import static org.apache.maven.surefire.its.fixture.HelperAssertions.assumeJavaMaxVersion;

nice, thanks @Tibor17, updated.

Tibor17 · 2022-01-12T16:15:08Z

One more thing. The Base class has try-catch. Please use throws Exception , it is typical for tests to process the exception by itself in the report, not necessary to rethrow exceptions.

...urefire-1967-testng-method-parallel-ordering/src/test/java/testng/parallelOrdering/Base.java

Tibor17 · 2021-12-24T11:57:59Z

...providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGExecutor.java

+            {
+                throw new RuntimeException( e );
+            }
+        }


Pls add a line with a comment regarding TestNG 5.10. It was very good point.

Tibor17 · 2021-12-24T12:31:50Z

...providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGExecutor.java

+            {
+                throw new RuntimeException( e );
+            }
+        }


@findepi It would be an ideal situation to ensure same behavior in both versions. So in that case the constructor would help us XmlClass(String className, int index, boolean loadClasses). Am I right?

Tibor17 · 2021-12-24T12:39:30Z

...providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGExecutor.java

+            {
+                throw new RuntimeException( e );
+            }
+        }


These two constructors handle the same information across versions
XmlClass(String className, int index, boolean loadClasses)
XmlClass(String name, Boolean declaredClass, int index)
@juherr Can you ensure that the constructors, at least two, would not be removed in the future? Difficult question for IT development, I know. :-)

Tibor17 · 2022-01-12T15:53:53Z

surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/pom.xml

+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-surefire-plugin</artifactId>
+        <version>${surefire.version}</version>
+        <configuration>


If the CPU is overloaded in Jenkins, this hypothesis may be broken and parallel threads turn to serial. Don;t rely on parallel execution even if you run parallel tests. Let's see the Jenkins test result especially on Windows...

Tibor17 · 2022-01-12T17:33:27Z

@findepi
One more thing. Is this new method tryGetConstructor called in some unit test?
It would be great to have a coverage. I know your implementation is okay but it is useful for our test coverage graph to keep the coverage on the same level or let it grow.

…onsumption Before the change, TestNG run from Surefire can execute `@BeforeClass` on many different test classes/instances, without invoking `@AfterClass` yet, leading to high resource utilization. This is not observed when tests are invoked via a suite file. This is because `XmlClass.m_index` field is used by TestNG to order test execution and this field used not to be set by Surefire. This commit lets Surefire initialize `XmlClass` object in a similar manner to how TestNG suite file XML parser does.

slawekjaranowski · 2022-01-12T21:39:59Z

OK, local branch for jenkins created
https://ci-maven.apache.org/job/Maven/job/maven-box/job/maven-surefire/job/SUREFIRE-1967/

Tibor17 · 2022-01-13T14:47:22Z

Currently I have forced rebuild of the Jenkins build because yesterday our INFRA team restarted the Jenkins and the previous job did not finish.

slawekjaranowski · 2022-01-13T16:40:53Z

There new commit appear - so I recreated branch: https://github.com/apache/maven-surefire/tree/SUREFIRE-1967
build start once again
last commit: f291fce

findepi · 2022-01-14T10:56:01Z

@slawekjaranowski thanks for triggering the Jenkins build.
I see the GHA succeeded on https://github.com/apache/maven-surefire/tree/SUREFIRE-1967 branch (https://github.com/apache/maven-surefire/actions/runs/1693777765). How was the Jenkins build?

Tibor17 · 2022-01-14T13:30:52Z

@findepi
Thx for contributing!

findepi · 2022-01-14T14:07:01Z

Thank you @Tibor17 for the merge, and thanks @Tibor17 @slawekjaranowski for all the review feedback.

findepi force-pushed the findepi/fix-parallel-test-ordering-to-prevent-high-resource-consumption-e8385a branch from 220af5f to e60a302 Compare December 14, 2021 17:15

findinpath reviewed Dec 14, 2021

View reviewed changes

findepi force-pushed the findepi/fix-parallel-test-ordering-to-prevent-high-resource-consumption-e8385a branch from e60a302 to 0725ea4 Compare December 15, 2021 19:55

slawekjaranowski requested changes Dec 15, 2021

View reviewed changes

...ire-its/src/test/java/org/apache/maven/surefire/its/CheckTestNgMethodParallelOrderingIT.java Outdated Show resolved Hide resolved

...providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGExecutor.java Outdated Show resolved Hide resolved

findepi force-pushed the findepi/fix-parallel-test-ordering-to-prevent-high-resource-consumption-e8385a branch from 0725ea4 to 5d55203 Compare December 15, 2021 20:56

slawekjaranowski requested changes Dec 15, 2021

View reviewed changes

surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/pom.xml Outdated Show resolved Hide resolved

findepi force-pushed the findepi/fix-parallel-test-ordering-to-prevent-high-resource-consumption-e8385a branch from 5d55203 to 3d44630 Compare December 16, 2021 08:44

slawekjaranowski requested a review from Tibor17 December 23, 2021 21:47

findepi force-pushed the findepi/fix-parallel-test-ordering-to-prevent-high-resource-consumption-e8385a branch from 3d44630 to bb78b57 Compare December 24, 2021 11:54

findepi force-pushed the findepi/fix-parallel-test-ordering-to-prevent-high-resource-consumption-e8385a branch 2 times, most recently from 2e03cb9 to b9fe306 Compare January 10, 2022 10:03

slawekjaranowski approved these changes Jan 12, 2022

View reviewed changes

findepi force-pushed the findepi/fix-parallel-test-ordering-to-prevent-high-resource-consumption-e8385a branch from b9fe306 to 3a0fdf4 Compare January 12, 2022 16:03

findepi force-pushed the findepi/fix-parallel-test-ordering-to-prevent-high-resource-consumption-e8385a branch from 3a0fdf4 to 9582fb7 Compare January 12, 2022 16:15

Tibor17 approved these changes Jan 12, 2022

View reviewed changes

findepi force-pushed the findepi/fix-parallel-test-ordering-to-prevent-high-resource-consumption-e8385a branch from 9582fb7 to f291fce Compare January 12, 2022 20:46

Tibor17 merged commit 909637c into apache:master Jan 14, 2022

findepi deleted the findepi/fix-parallel-test-ordering-to-prevent-high-resource-consumption-e8385a branch January 14, 2022 14:06

findepi mentioned this pull request Jan 15, 2022

In CI print logs only when any test in a class fails trinodb/trino#10620

Closed

findepi mentioned this pull request Jan 24, 2022

Crashed tests in presto-hive trinodb/trino#5527

Closed

findepi mentioned this pull request Feb 23, 2022

Define additional repos only in submodules using them trinodb/trino#11119

Merged

findepi mentioned this pull request Mar 3, 2022

Remove test concurrency related workarounds trinodb/trino#11294

Open

findepi mentioned this pull request Apr 7, 2022

trino-hive tests sometimes fail with OutOfMemoryError trinodb/trino#11848

Closed

nineinchnick mentioned this pull request Apr 7, 2022

Update Surefire plugin airlift/airbase#310

Merged

[SUREFIRE-1967] Fix parallel test ordering to prevent high resource consumption #407

[SUREFIRE-1967] Fix parallel test ordering to prevent high resource consumption #407

Conversation

findepi commented Dec 14, 2021 • edited

findepi commented Dec 14, 2021

findinpath Dec 14, 2021 • edited

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

findinpath commented Dec 14, 2021

findepi commented Dec 14, 2021

findepi commented Dec 14, 2021

slawekjaranowski commented Dec 14, 2021

findepi commented Dec 15, 2021 • edited

slawekjaranowski commented Dec 15, 2021

findepi commented Dec 15, 2021 • edited

slawekjaranowski commented Dec 15, 2021

findepi commented Dec 15, 2021

findepi commented Dec 15, 2021

findepi commented Dec 16, 2021

findepi commented Dec 20, 2021

slawekjaranowski commented Dec 21, 2021

findepi commented Dec 21, 2021

slawekjaranowski commented Dec 21, 2021

findepi commented Dec 22, 2021

findepi commented Dec 23, 2021

Tibor17 commented Dec 24, 2021

Tibor17 commented Dec 24, 2021

findepi commented Dec 24, 2021

findepi commented Jan 8, 2022

findepi commented Jan 10, 2022

findepi commented Jan 11, 2022

Tibor17 commented Jan 12, 2022

Tibor17 commented Jan 12, 2022

findepi commented Jan 12, 2022

Tibor17 commented Jan 12, 2022

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

Tibor17 commented Jan 12, 2022 • edited

slawekjaranowski commented Jan 12, 2022

Tibor17 commented Jan 13, 2022

slawekjaranowski commented Jan 13, 2022 • edited

findepi commented Jan 14, 2022

Tibor17 commented Jan 14, 2022

findepi commented Jan 14, 2022

findepi commented Dec 14, 2021 •

edited

findinpath Dec 14, 2021 •

edited

findepi commented Dec 15, 2021 •

edited

findepi commented Dec 15, 2021 •

edited

Tibor17 commented Jan 12, 2022 •

edited

slawekjaranowski commented Jan 13, 2022 •

edited