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 48b4032338..27821d7e87 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 static Constructor getConstructor( Class clazz, Class... argumen } } + public static Constructor tryGetConstructor( Class clazz, Class... arguments ) + { + try + { + return clazz.getConstructor( arguments ); + } + catch ( NoSuchMethodException e ) + { + return null; + } + } + public static T newInstance( Constructor constructor, Object... params ) { try 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 0000000000..f7174bd14f --- /dev/null +++ b/surefire-its/src/test/java/org/apache/maven/surefire/its/jiras/Surefire1967CheckTestNgMethodParallelOrderingIT.java @@ -0,0 +1,90 @@ +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 java.util.regex.Matcher; +import java.util.regex.Pattern; + +import static org.junit.Assert.assertTrue; +import static org.junit.Assume.assumeTrue; + +/** + * 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() + { + // TODO Replace Pattern with java.lang.Runtime.Version once Java 8 support is dropped + 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 ); + + 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 0000000000..f71d97c598 --- /dev/null +++ b/surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/pom.xml @@ -0,0 +1,60 @@ + + + + + 4.0.0 + + + org.apache.maven.surefire + it-parent + 1.0 + ../pom.xml + + + org.apache.maven.plugins.surefire + surefire-1967-testng-method-parallel-ordering + 1.0-SNAPSHOT + TestNG parallel ordering + + + + org.testng + testng + ${testNgVersion} + + + + + + + org.apache.maven.plugins + maven-surefire-plugin + ${surefire.version} + + 2 + methods + + + + + + 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 0000000000..147852d338 --- /dev/null +++ b/surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/src/test/java/testng/parallelOrdering/Base.java @@ -0,0 +1,59 @@ +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() + { + sleepShortly(); + } + + @Test + public void test2() + { + sleepShortly(); + } + + @Test + public void test3() + { + sleepShortly(); + } + + // Sleep random time to let tests interleave. Keep sleep short not to extend tests duration too much. + private void sleepShortly() + { + try { + Thread.sleep(ThreadLocalRandom.current().nextInt(3)); + } + catch (InterruptedException e) { + throw new RuntimeException(e); + } + } +} 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 0000000000..34c524ca67 --- /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 0000000000..8b11761ded --- /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 0000000000..b5ea787d24 --- /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 0000000000..51b9fc51ac --- /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 56fc9d5987..c0799f7a54 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 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 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> 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> 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 ); + } + return new XmlClass( testClassName ); + } + private static boolean isCliDebugOrShowErrors( List mainCliOptions ) { return mainCliOptions.contains( LOGGING_LEVEL_DEBUG ) || mainCliOptions.contains( SHOW_ERRORS );