[SUREFIRE-1967] Fix parallel test ordering to prevent high resource c…

…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.

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 <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

surefire-api/src/test/java/org/apache/maven/surefire/api/util/ReflectionUtilsTest.java

@@ -21,6 +21,8 @@
 
 import org.junit.Test;
 
+import java.lang.reflect.Constructor;
+
 import static org.fest.assertions.Assertions.assertThat;
 
 /**
@@ -31,6 +33,23 @@
  */
 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
     {
@@ -121,4 +140,14 @@ public long pid()
             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 )
+        {
+        }
+    }
 }

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 );
+    }
+}

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>

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));
+    }
+}

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 {}

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 {}

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 {}

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 {}

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<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 );
+        }
+        return new XmlClass( testClassName );
+    }
+
     private static boolean isCliDebugOrShowErrors( List<CommandLineOption> mainCliOptions )
     {
         return mainCliOptions.contains( LOGGING_LEVEL_DEBUG ) || mainCliOptions.contains( SHOW_ERRORS );