You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maven.apache.org by ti...@apache.org on 2022/01/23 06:07:13 UTC
[maven-surefire] 04/12: [SUREFIRE-1967] Fix parallel test ordering to prevent high resource consumption
This is an automated email from the ASF dual-hosted git repository.
tibordigana pushed a commit to branch release/2.22.3
in repository https://gitbox.apache.org/repos/asf/maven-surefire.git
commit 43e576747379362c3e037cde34a8520ffffef27f
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.
(cherry picked from commit 909637cb5d79171e12690ad0f1b99a3bd2184e23)
---
.../maven/surefire/util/ReflectionUtils.java | 12 ++++
.../maven/surefire/util/ReflectionUtilsTest.java | 29 ++++++++
.../surefire/its/fixture/HelperAssertions.java | 7 ++
...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 ++++++++++-
11 files changed, 302 insertions(+), 1 deletion(-)
diff --git a/surefire-api/src/main/java/org/apache/maven/surefire/util/ReflectionUtils.java b/surefire-api/src/main/java/org/apache/maven/surefire/util/ReflectionUtils.java
index 49f8f09..38c89ce 100644
--- a/surefire-api/src/main/java/org/apache/maven/surefire/util/ReflectionUtils.java
+++ b/surefire-api/src/main/java/org/apache/maven/surefire/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 Object newInstance( Constructor constructor, Object... params )
{
try
diff --git a/surefire-api/src/test/java/org/apache/maven/surefire/util/ReflectionUtilsTest.java b/surefire-api/src/test/java/org/apache/maven/surefire/util/ReflectionUtilsTest.java
index 5440d6e..09beff5 100644
--- a/surefire-api/src/test/java/org/apache/maven/surefire/util/ReflectionUtilsTest.java
+++ b/surefire-api/src/test/java/org/apache/maven/surefire/util/ReflectionUtilsTest.java
@@ -21,6 +21,8 @@ package org.apache.maven.surefire.util;
import org.junit.Test;
+import java.lang.reflect.Constructor;
+
import static org.fest.assertions.Assertions.assertThat;
/**
@@ -31,6 +33,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(expected = RuntimeException.class)
public void shouldNotInvokeStaticMethod()
{
@@ -113,4 +132,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/fixture/HelperAssertions.java b/surefire-its/src/test/java/org/apache/maven/surefire/its/fixture/HelperAssertions.java
index 5bea87e..641ac41 100644
--- a/surefire-its/src/test/java/org/apache/maven/surefire/its/fixture/HelperAssertions.java
+++ b/surefire-its/src/test/java/org/apache/maven/surefire/its/fixture/HelperAssertions.java
@@ -28,6 +28,7 @@ import java.util.ArrayList;
import java.util.List;
import java.util.Locale;
+import static java.lang.Double.parseDouble;
import static junit.framework.Assert.assertEquals;
import static junit.framework.Assert.assertTrue;
import static org.apache.commons.io.Charsets.UTF_8;
@@ -51,6 +52,12 @@ public class HelperAssertions
assertTestSuiteResults( total, errors, failures, skipped, flakes, suite );
}
+ public static void assumeJavaMaxVersion( double expectedMaxVersion )
+ {
+ String thisVersion = System.getProperty( "java.specification.version" );
+ assumeTrue( "java.specification.version: " + thisVersion, parseDouble( thisVersion ) <= expectedMaxVersion );
+ }
+
public static void assertTestSuiteResults( int total, File testDir )
{
IntegrationTestSuiteResults suite = parseTestResults( testDir );
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 0b52c4d..bae5f8b 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
@@ -50,6 +50,10 @@ import java.util.concurrent.atomic.AtomicInteger;
import static org.apache.maven.surefire.cli.CommandLineOption.LOGGING_LEVEL_DEBUG;
import static org.apache.maven.surefire.cli.CommandLineOption.SHOW_ERRORS;
import static org.apache.maven.surefire.util.ReflectionUtils.instantiate;
+import static org.apache.maven.surefire.util.ReflectionUtils.invokeSetter;
+import static org.apache.maven.surefire.util.ReflectionUtils.newInstance;
+import static org.apache.maven.surefire.util.ReflectionUtils.tryGetConstructor;
+import static org.apache.maven.surefire.util.ReflectionUtils.tryGetMethod;
import static org.apache.maven.surefire.util.ReflectionUtils.tryLoadClass;
import static org.apache.maven.surefire.util.internal.ConcurrencyUtils.countDownToZero;
@@ -70,6 +74,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" );
@@ -125,7 +140,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 );
@@ -135,6 +150,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 (XmlClass) 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 );