You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maven.apache.org by sj...@apache.org on 2022/01/12 21:35:57 UTC
[maven-surefire] branch SUREFIRE-1967 created (now 1078869)
This is an automated email from the ASF dual-hosted git repository.
sjaranowski pushed a change to branch SUREFIRE-1967
in repository https://gitbox.apache.org/repos/asf/maven-surefire.git.
at 1078869 [SUREFIRE-1967] Fix parallel test ordering to prevent high resource consumption
This branch includes the following new commits:
new 1078869 [SUREFIRE-1967] Fix parallel test ordering to prevent high resource consumption
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
[maven-surefire] 01/01: [SUREFIRE-1967] Fix parallel test ordering to prevent high resource consumption
Posted by sj...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
sjaranowski pushed a commit to branch SUREFIRE-1967
in repository https://gitbox.apache.org/repos/asf/maven-surefire.git
commit 107886976df90c6904fbeca835b615de4f6dda0f
Author: Piotr Findeisen <pi...@gmail.com>
AuthorDate: Mon Jan 10 11:03:00 2022 +0100
[SUREFIRE-1967] Fix parallel test ordering to prevent high resource consumption
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.
---
.../maven/surefire/api/util/ReflectionUtils.java | 12 ++++
.../surefire/api/util/ReflectionUtilsTest.java | 29 ++++++++
...ire1967CheckTestNgMethodParallelOrderingIT.java | 83 ++++++++++++++++++++++
.../pom.xml | 60 ++++++++++++++++
.../test/java/testng/parallelOrdering/Base.java | 58 +++++++++++++++
.../java/testng/parallelOrdering/TestClass1.java | 3 +
.../java/testng/parallelOrdering/TestClass2.java | 3 +
.../java/testng/parallelOrdering/TestClass3.java | 3 +
.../java/testng/parallelOrdering/TestClass4.java | 3 +
.../maven/surefire/testng/TestNGExecutor.java | 42 ++++++++++-
10 files changed, 295 insertions(+), 1 deletion(-)
diff --git a/surefire-api/src/main/java/org/apache/maven/surefire/api/util/ReflectionUtils.java b/surefire-api/src/main/java/org/apache/maven/surefire/api/util/ReflectionUtils.java
index 48b4032..27821d7 100644
--- a/surefire-api/src/main/java/org/apache/maven/surefire/api/util/ReflectionUtils.java
+++ b/surefire-api/src/main/java/org/apache/maven/surefire/api/util/ReflectionUtils.java
@@ -88,6 +88,18 @@ public final class ReflectionUtils
}
}
+ public static <T> Constructor<T> tryGetConstructor( Class<T> clazz, Class<?>... arguments )
+ {
+ try
+ {
+ return clazz.getConstructor( arguments );
+ }
+ catch ( NoSuchMethodException e )
+ {
+ return null;
+ }
+ }
+
public static <T> T newInstance( Constructor<?> constructor, Object... params )
{
try
diff --git a/surefire-api/src/test/java/org/apache/maven/surefire/api/util/ReflectionUtilsTest.java b/surefire-api/src/test/java/org/apache/maven/surefire/api/util/ReflectionUtilsTest.java
index a0f3b57..0f8c5da 100644
--- a/surefire-api/src/test/java/org/apache/maven/surefire/api/util/ReflectionUtilsTest.java
+++ b/surefire-api/src/test/java/org/apache/maven/surefire/api/util/ReflectionUtilsTest.java
@@ -21,6 +21,8 @@ package org.apache.maven.surefire.api.util;
import org.junit.Test;
+import java.lang.reflect.Constructor;
+
import static org.fest.assertions.Assertions.assertThat;
/**
@@ -32,6 +34,23 @@ import static org.fest.assertions.Assertions.assertThat;
public class ReflectionUtilsTest
{
@Test
+ public void shouldGetConstructor() throws Exception
+ {
+ Constructor<ClassWithParameterizedConstructor> constructor =
+ ReflectionUtils.tryGetConstructor( ClassWithParameterizedConstructor.class, int.class );
+ assertThat( constructor ).isNotNull();
+ // Verify the Constructor returned really is for the class it should be
+ assertThat( constructor.newInstance( 10 ) ).isInstanceOf( ClassWithParameterizedConstructor.class );
+ }
+
+ @Test
+ public void shouldNotGetNonExistingConstructor()
+ {
+ assertThat( ReflectionUtils.tryGetConstructor( ClassWithParameterizedConstructor.class, String.class ) )
+ .isNull();
+ }
+
+ @Test
public void shouldReloadClass() throws Exception
{
ClassLoader cl = Thread.currentThread().getContextClassLoader();
@@ -121,4 +140,14 @@ public class ReflectionUtilsTest
return 1L;
}
}
+
+ // The constructor has to be public for ReflectionUtils.tryGetConstructor to find it. Currently, the checkstyle
+ // rules require that class be public too, and a public class must be documented, hence minimalist javadoc.
+ /** */
+ public static class ClassWithParameterizedConstructor
+ {
+ public ClassWithParameterizedConstructor( int param )
+ {
+ }
+ }
}
diff --git a/surefire-its/src/test/java/org/apache/maven/surefire/its/jiras/Surefire1967CheckTestNgMethodParallelOrderingIT.java b/surefire-its/src/test/java/org/apache/maven/surefire/its/jiras/Surefire1967CheckTestNgMethodParallelOrderingIT.java
new file mode 100644
index 0000000..5f0ed9d
--- /dev/null
+++ b/surefire-its/src/test/java/org/apache/maven/surefire/its/jiras/Surefire1967CheckTestNgMethodParallelOrderingIT.java
@@ -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 );
+ }
+}
diff --git a/surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/pom.xml b/surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/pom.xml
new file mode 100644
index 0000000..f71d97c
--- /dev/null
+++ b/surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/pom.xml
@@ -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>
diff --git a/surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/src/test/java/testng/parallelOrdering/Base.java b/surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/src/test/java/testng/parallelOrdering/Base.java
new file mode 100644
index 0000000..ab37f14
--- /dev/null
+++ b/surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/src/test/java/testng/parallelOrdering/Base.java
@@ -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));
+ }
+}
diff --git a/surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/src/test/java/testng/parallelOrdering/TestClass1.java b/surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/src/test/java/testng/parallelOrdering/TestClass1.java
new file mode 100644
index 0000000..34c524c
--- /dev/null
+++ b/surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/src/test/java/testng/parallelOrdering/TestClass1.java
@@ -0,0 +1,3 @@
+package testng.parallelOrdering;
+
+public class TestClass1 extends Base {}
diff --git a/surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/src/test/java/testng/parallelOrdering/TestClass2.java b/surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/src/test/java/testng/parallelOrdering/TestClass2.java
new file mode 100644
index 0000000..8b11761
--- /dev/null
+++ b/surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/src/test/java/testng/parallelOrdering/TestClass2.java
@@ -0,0 +1,3 @@
+package testng.parallelOrdering;
+
+public class TestClass2 extends Base {}
diff --git a/surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/src/test/java/testng/parallelOrdering/TestClass3.java b/surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/src/test/java/testng/parallelOrdering/TestClass3.java
new file mode 100644
index 0000000..b5ea787
--- /dev/null
+++ b/surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/src/test/java/testng/parallelOrdering/TestClass3.java
@@ -0,0 +1,3 @@
+package testng.parallelOrdering;
+
+public class TestClass3 extends Base {}
diff --git a/surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/src/test/java/testng/parallelOrdering/TestClass4.java b/surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/src/test/java/testng/parallelOrdering/TestClass4.java
new file mode 100644
index 0000000..51b9fc5
--- /dev/null
+++ b/surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/src/test/java/testng/parallelOrdering/TestClass4.java
@@ -0,0 +1,3 @@
+package testng.parallelOrdering;
+
+public class TestClass4 extends Base {}
diff --git a/surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGExecutor.java b/surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGExecutor.java
index 56fc9d5..c0799f7 100644
--- a/surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGExecutor.java
+++ b/surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGExecutor.java
@@ -52,6 +52,10 @@ import java.util.concurrent.atomic.AtomicInteger;
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 @@ final class TestNGExecutor
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 @@ final class TestNGExecutor
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 );
+ }
+ return new XmlClass( testClassName );
+ }
+
private static boolean isCliDebugOrShowErrors( List<CommandLineOption> mainCliOptions )
{
return mainCliOptions.contains( LOGGING_LEVEL_DEBUG ) || mainCliOptions.contains( SHOW_ERRORS );