[SUREFIRE-1967] Fix parallel test ordering to prevent high resource consumption#407
Conversation
220af5f to
e60a302
Compare
| { | ||
| throw new RuntimeException( e ); | ||
| } | ||
| } |
There was a problem hiding this comment.
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#L36XmlClass(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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Pls add a line with a comment regarding TestNG 5.10. It was very good point.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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. :-)
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
|
Please do take a look at 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 After the change, i wouldn't see more than 2 (test thread count) test classes initialized at the same time. |
|
@Tibor17 This is my first contribution here. i've signed & sent the CLA. Would you mind approving the CI to run? |
|
Does this change has no influences on existing tests? Can you prepare some test which can confirm this behavior? |
|
@slawekjaranowski thanks for your feedback
I've completed run of What else i should do?
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. |
|
Look into |
|
@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 and got should i be setting |
|
try magic |
e60a302 to
0725ea4
Compare
|
@slawekjaranowski thanks, added a test. |
...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
0725ea4 to
5d55203
Compare
|
@slawekjaranowski thanks for review, applied comments |
surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/pom.xml
Outdated
Show resolved
Hide resolved
5d55203 to
3d44630
Compare
|
@slawekjaranowski thanks for your review, comments applied. |
|
@slawekjaranowski i've received a confirmation that my signed CLA was received by Apache. |
|
@findepi - please follow dev mailing list. |
|
@slawekjaranowski do you want me to sign up to Maven Developer List ( |
|
simply send email for given address and follow on response. |
|
@slawekjaranowski thanks, done |
|
@slawekjaranowski should i do any other changes here? is there a chance to get this into 3.0.0-M6? |
|
@findepi |
|
@findepi |
3d44630 to
bb78b57
Compare
|
@Tibor17 thanks for your review. |
2e03cb9 to
b9fe306
Compare
|
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? |
|
thanks for approving the PR run. |
|
|
can be replaced with |
b9fe306 to
3a0fdf4
Compare
nice, thanks @Tibor17, updated. |
|
One more thing. The |
3a0fdf4 to
9582fb7
Compare
...urefire-1967-testng-method-parallel-ordering/src/test/java/testng/parallelOrdering/Base.java
Outdated
Show resolved
Hide resolved
| { | ||
| throw new RuntimeException( e ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Pls add a line with a comment regarding TestNG 5.10. It was very good point.
| { | ||
| throw new RuntimeException( e ); | ||
| } | ||
| } |
There was a problem hiding this comment.
@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?
| { | ||
| throw new RuntimeException( e ); | ||
| } | ||
| } |
There was a problem hiding this comment.
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. :-)
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-surefire-plugin</artifactId> | ||
| <version>${surefire.version}</version> | ||
| <configuration> |
There was a problem hiding this comment.
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...
|
@findepi |
…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.
9582fb7 to
f291fce
Compare
|
OK, local branch for jenkins created |
|
Currently I have forced rebuild of the Jenkins build because yesterday our INFRA team restarted the Jenkins and the previous job did not finish. |
|
There new commit appear - so I recreated branch: https://github.com/apache/maven-surefire/tree/SUREFIRE-1967 |
|
@slawekjaranowski thanks for triggering the Jenkins build. |
|
@findepi |
|
Thank you @Tibor17 for the merge, and thanks @Tibor17 @slawekjaranowski for all the review feedback. |
|
Resolve #2880 |
1 similar comment
|
Resolve #2880 |
[ https://issues.apache.org/jira/browse/SUREFIRE-1967 ]
Before the change, TestNG run from Surefire can execute
@BeforeClasson many different test classes/instances, without invoking
@AfterClassyet, leading to high resource utilization. This is not observed when
tests are invoked via a suite file. This is because
XmlClass.m_indexfield is used by TestNG to order test execution and this field used not
to be set by Surefire. This commit lets Surefire initialize
XmlClassobject in a similar manner to how TestNG suite file XML parser does.
Following this checklist to help us incorporate your
contribution quickly and easily:
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.
[SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles,where you replace
SUREFIRE-XXXwith the appropriate JIRA issue. Best practiceis to use the JIRA issue title in the pull request title and in the first line of the
commit message.
mvn clean installto make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
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.