[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-its/src/test/java/org/apache/maven/surefire/its/jiras/Surefire1967CheckTestNgMethodParallelOrderingIT.java

@@ -0,0 +1,52 @@
+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;
+
+/**
+ * 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 testNgParallelOrderingWithOldVersion()
+    {
+        unpack( "surefire-1967-testng-method-parallel-ordering" )
+                .sysProp( "testNgVersion", "6.10" )
+                .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,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);
+        }
+    }
+}

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,8 @@
 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.tryGetMethod;
 import static org.apache.maven.surefire.api.util.ReflectionUtils.tryLoadClass;
 import static org.apache.maven.surefire.api.util.internal.ConcurrencyUtils.countDownToZero;
 
@@ -72,6 +74,11 @@ 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 );
+
     private TestNGExecutor()
     {
         throw new IllegalStateException( "not instantiable constructor" );
@@ -127,7 +134,7 @@ static void run( Iterable<Class<?>> testClasses, String testSourceDirectory,
                 suiteAndNamedTests.testNameToTest.put( metadata.testName, xmlTest );
             }
 
-            xmlTest.getXmlClasses().add( new XmlClass( testClass.getName() ) );
+            addXmlClass( xmlTest.getXmlClasses(), testClass.getName() );
         }
 
         testng.setXmlSuites( xmlSuites );
@@ -137,6 +144,24 @@ static void run( Iterable<Class<?>> testClasses, String testSourceDirectory,
         testng.run();
     }
 
+    private static void addXmlClass( List<XmlClass> xmlClasses, String testClassName )
+    {
+        XmlClass xmlClass = new XmlClass( testClassName );
+        if ( XML_CLASS_SET_INDEX != null )
+        {
+            // 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.
+            invokeSetter( xmlClass, XML_CLASS_SET_INDEX, xmlClasses.size() );
+        }
+        xmlClasses.add( xmlClass );
+    }
+
     private static boolean isCliDebugOrShowErrors( List<CommandLineOption> mainCliOptions )
     {
         return mainCliOptions.contains( LOGGING_LEVEL_DEBUG ) || mainCliOptions.contains( SHOW_ERRORS );