New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SUREFIRE-1967] Fix parallel test ordering to prevent high resource consumption #407
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
package org.apache.maven.surefire.its.jiras; | ||
|
||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
import org.apache.maven.surefire.its.fixture.SurefireJUnit4IntegrationTestCase; | ||
import org.junit.Test; | ||
|
||
import static org.apache.maven.surefire.its.fixture.HelperAssertions.assumeJavaMaxVersion; | ||
|
||
/** | ||
* Test TestNG setup and teardown ordering with parallelism | ||
* | ||
* @author findepi | ||
*/ | ||
public class Surefire1967CheckTestNgMethodParallelOrderingIT | ||
extends SurefireJUnit4IntegrationTestCase | ||
{ | ||
@Test | ||
public void testNgParallelOrdering() | ||
{ | ||
unpack( "surefire-1967-testng-method-parallel-ordering" ) | ||
.sysProp( "testNgVersion", "7.3.0" ) | ||
.executeTest() | ||
.verifyErrorFree( 12 ); | ||
} | ||
|
||
// Since the test ordering guarantees currently depend on reflection, it's useful to test with | ||
// some older version too. | ||
@Test | ||
public void testNgParallelOrderingWithVersion6() | ||
{ | ||
unpack( "surefire-1967-testng-method-parallel-ordering" ) | ||
.sysProp( "testNgVersion", "6.10" ) | ||
.executeTest() | ||
.verifyErrorFree( 12 ); | ||
} | ||
|
||
// TestNG 6.2.1 is the newest version that doesn't have XmlClass.setIndex method yet. | ||
// Note that the problem of wrong setup methods ordering (SUREFIRE-1967) was not observed on that version. | ||
// This is likely because SUREFIRE-1967 is related to a change in TestNG 6.3, where preserve-order became true by | ||
// default (https://github.com/cbeust/testng/commit/8849b3406ef2184ceb6002768a2d087d7a8de8d5). | ||
@Test | ||
public void testNgParallelOrderingWithEarlyVersion6() | ||
{ | ||
unpack( "surefire-1967-testng-method-parallel-ordering" ) | ||
.sysProp( "testNgVersion", "6.2.1" ) | ||
.executeTest() | ||
.verifyErrorFree( 12 ); | ||
} | ||
|
||
// TestNG 5.13+ already has XmlClass.m_index field, but doesn't have XmlClass.setIndex method. | ||
// Note that the problem of wrong setup methods ordering (SUREFIRE-1967) was not observed on that version. | ||
// This is likely because SUREFIRE-1967 is related to a change in TestNG 6.3, where preserve-order became true by | ||
// default (https://github.com/cbeust/testng/commit/8849b3406ef2184ceb6002768a2d087d7a8de8d5). | ||
@Test | ||
public void testNgParallelOrderingWithVersion5() | ||
{ | ||
// TestNG 5.13 does not work with Java 17 | ||
assumeJavaMaxVersion( 16 ); | ||
|
||
unpack( "surefire-1967-testng-method-parallel-ordering" ) | ||
.sysProp( "testNgVersion", "5.13" ) | ||
.executeTest() | ||
.verifyErrorFree( 12 ); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<!-- | ||
~ Licensed to the Apache Software Foundation (ASF) under one | ||
~ or more contributor license agreements. See the NOTICE file | ||
~ distributed with this work for additional information | ||
~ regarding copyright ownership. The ASF licenses this file | ||
~ to you under the Apache License, Version 2.0 (the | ||
~ "License"); you may not use this file except in compliance | ||
~ with the License. You may obtain a copy of the License at | ||
~ | ||
~ http://www.apache.org/licenses/LICENSE-2.0 | ||
~ | ||
~ Unless required by applicable law or agreed to in writing, | ||
~ software distributed under the License is distributed on an | ||
~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
~ KIND, either express or implied. See the License for the | ||
~ specific language governing permissions and limitations | ||
~ under the License. | ||
--> | ||
|
||
<project xmlns="http://maven.apache.org/POM/4.0.0" | ||
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> | ||
<modelVersion>4.0.0</modelVersion> | ||
|
||
<parent> | ||
<groupId>org.apache.maven.surefire</groupId> | ||
<artifactId>it-parent</artifactId> | ||
<version>1.0</version> | ||
<relativePath>../pom.xml</relativePath> | ||
</parent> | ||
|
||
<groupId>org.apache.maven.plugins.surefire</groupId> | ||
<artifactId>surefire-1967-testng-method-parallel-ordering</artifactId> | ||
<version>1.0-SNAPSHOT</version> | ||
<name>TestNG parallel ordering</name> | ||
|
||
<dependencies> | ||
<dependency> | ||
<groupId>org.testng</groupId> | ||
<artifactId>testng</artifactId> | ||
<version>${testNgVersion}</version> | ||
</dependency> | ||
</dependencies> | ||
|
||
<build> | ||
<plugins> | ||
<plugin> | ||
<groupId>org.apache.maven.plugins</groupId> | ||
<artifactId>maven-surefire-plugin</artifactId> | ||
<version>${surefire.version}</version> | ||
<configuration> | ||
<threadCount>2</threadCount> | ||
<parallel>methods</parallel> | ||
</configuration> | ||
</plugin> | ||
</plugins> | ||
</build> | ||
|
||
</project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
package testng.parallelOrdering; | ||
|
||
import org.testng.annotations.AfterClass; | ||
import org.testng.annotations.BeforeClass; | ||
import org.testng.annotations.Test; | ||
|
||
import java.util.concurrent.ThreadLocalRandom; | ||
import java.util.concurrent.atomic.AtomicInteger; | ||
|
||
public abstract class Base | ||
{ | ||
private static final AtomicInteger resources = new AtomicInteger(); | ||
|
||
// This simulates resource allocation | ||
@BeforeClass | ||
public void setupAllocateResources() | ||
{ | ||
int concurrentResources = resources.incrementAndGet(); | ||
if (concurrentResources > 2) { | ||
throw new IllegalStateException("Tests execute in two threads, so there should be at most 2 resources allocated, got: " + concurrentResources); | ||
} | ||
} | ||
|
||
// This simulates freeing resources | ||
@AfterClass(alwaysRun = true) | ||
public void tearDownReleaseResources() | ||
{ | ||
resources.decrementAndGet(); | ||
} | ||
|
||
@Test | ||
public void test1() | ||
throws Exception | ||
{ | ||
sleepShortly(); | ||
} | ||
|
||
@Test | ||
public void test2() | ||
throws Exception | ||
{ | ||
sleepShortly(); | ||
} | ||
|
||
@Test | ||
public void test3() | ||
throws Exception | ||
{ | ||
sleepShortly(); | ||
} | ||
|
||
// Sleep random time to let tests interleave. Keep sleep short not to extend tests duration too much. | ||
private void sleepShortly() | ||
throws InterruptedException | ||
{ | ||
Thread.sleep(ThreadLocalRandom.current().nextInt(3)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
package testng.parallelOrdering; | ||
|
||
public class TestClass1 extends Base {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
package testng.parallelOrdering; | ||
|
||
public class TestClass2 extends Base {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
package testng.parallelOrdering; | ||
|
||
public class TestClass3 extends Base {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
package testng.parallelOrdering; | ||
|
||
public class TestClass4 extends Base {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,6 +52,10 @@ | |
import static org.apache.maven.surefire.api.cli.CommandLineOption.LOGGING_LEVEL_DEBUG; | ||
import static org.apache.maven.surefire.api.cli.CommandLineOption.SHOW_ERRORS; | ||
import static org.apache.maven.surefire.api.util.ReflectionUtils.instantiate; | ||
import static org.apache.maven.surefire.api.util.ReflectionUtils.invokeSetter; | ||
import static org.apache.maven.surefire.api.util.ReflectionUtils.newInstance; | ||
import static org.apache.maven.surefire.api.util.ReflectionUtils.tryGetConstructor; | ||
import static org.apache.maven.surefire.api.util.ReflectionUtils.tryGetMethod; | ||
import static org.apache.maven.surefire.api.util.ReflectionUtils.tryLoadClass; | ||
import static org.apache.maven.surefire.api.util.internal.ConcurrencyUtils.countDownToZero; | ||
|
||
|
@@ -72,6 +76,17 @@ final class TestNGExecutor | |
private static final boolean HAS_TEST_ANNOTATION_ON_CLASSPATH = | ||
tryLoadClass( TestNGExecutor.class.getClassLoader(), "org.testng.annotations.Test" ) != null; | ||
|
||
// 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. | ||
private static final Method XML_CLASS_SET_INDEX = tryGetMethod( XmlClass.class, "setIndex", int.class ); | ||
|
||
// For TestNG versions [5.13, 6.3) where XmlClass.setIndex is not available, invoke XmlClass(String, boolean, int) | ||
// constructor. Note that XmlClass(String, boolean, int) was replaced with XmlClass(String, int) when | ||
// XmlClass.setIndex already existed. | ||
private static final Constructor<XmlClass> XML_CLASS_CONSTRUCTOR_WITH_INDEX = | ||
tryGetConstructor( XmlClass.class, String.class, boolean.class, int.class ); | ||
|
||
private TestNGExecutor() | ||
{ | ||
throw new IllegalStateException( "not instantiable constructor" ); | ||
|
@@ -127,7 +142,7 @@ static void run( Iterable<Class<?>> testClasses, String testSourceDirectory, | |
suiteAndNamedTests.testNameToTest.put( metadata.testName, xmlTest ); | ||
} | ||
|
||
xmlTest.getXmlClasses().add( new XmlClass( testClass.getName() ) ); | ||
xmlTest.getXmlClasses().add( newXmlClassInstance( testClass.getName(), xmlTest.getXmlClasses().size() ) ); | ||
} | ||
|
||
testng.setXmlSuites( xmlSuites ); | ||
|
@@ -137,6 +152,31 @@ static void run( Iterable<Class<?>> testClasses, String testSourceDirectory, | |
testng.run(); | ||
} | ||
|
||
private static XmlClass newXmlClassInstance( String testClassName, int index ) | ||
{ | ||
// In case of parallel test execution with parallel="methods", TestNG orders test execution | ||
// by XmlClass.m_index field. When unset (equal for all XmlClass instances), TestNG can | ||
// invoke `@BeforeClass` setup methods on many instances, without invoking `@AfterClass` | ||
// tearDown methods, thus leading to high resource consumptions when test instances | ||
// allocate resources. | ||
// With index set, order of setup, test and tearDown methods is reasonable, with approximately | ||
// #thread-count many test classes being initialized at given point in time. | ||
// Note: XmlClass.m_index field is set automatically by TestNG when it reads a suite file. | ||
|
||
if ( XML_CLASS_SET_INDEX != null ) | ||
{ | ||
XmlClass xmlClass = new XmlClass( testClassName ); | ||
invokeSetter( xmlClass, XML_CLASS_SET_INDEX, index ); | ||
return xmlClass; | ||
} | ||
if ( XML_CLASS_CONSTRUCTOR_WITH_INDEX != null ) | ||
{ | ||
boolean loadClass = true; // this is the default | ||
return newInstance( XML_CLASS_CONSTRUCTOR_WITH_INDEX, testClassName, loadClass, index ); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't we have here reflective access for the Instead of setting the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code is compiled against TestNG 5.10 and that version doesn't have the
Yes, I can lookup a constructor taking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pls add a line with a comment regarding TestNG 5.10. It was very good point. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @findepi It would be an ideal situation to ensure same behavior in both versions. So in that case the constructor would help us There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two constructors handle the same information across versions There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You mean this one: The code is compiled against TestNG 5.10 and that version doesn't have the 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:
i tried to make it convey same meaning: that we have to use the reflection. |
||
return new XmlClass( testClassName ); | ||
} | ||
|
||
private static boolean isCliDebugOrShowErrors( List<CommandLineOption> mainCliOptions ) | ||
{ | ||
return mainCliOptions.contains( LOGGING_LEVEL_DEBUG ) || mainCliOptions.contains( SHOW_ERRORS ); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is rather a cosmetic change proposal
Base.java
pom.xml
Obviously the
2
value inpom.xml
can be passed along (same as${surefire.version}
) to avoid duplication.The main advantage in adding this cosmetic change is that you could easily test the behaviour with multiple values for the
threadCount
(1
,2
,3
and a value greater than the number of methods being tested).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't see advantage of doing this. Please help me understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You currently hardcoded the logic for checking the concurrency level in the
Base
class to be2
.By using a
sysProp
for the thread count inSurefire1967CheckTestNgMethodParallelOrderingIT
you could test the functionality not only with differentTestNG
versions, but also with differentthreadCount
values (serial -1
, concurrent -2
, and also a greater value than the number of methods in the test suite).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that parameterizing threadCount would allow me to run tests with different threadCount values. Would we actually want to run the test more times? What additional value would it bring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see an advantage in seeing whether we test multiple values (at least
1
and2
) for the thread count.Looking closer at
Base
class we could add a@Before
setup and@After
and tear down methods to verify whether the amount of tests being executed in parallel is indeed between1
and the configuredthreadCount
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...