You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2022/01/12 16:16:16 UTC
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #407: [SUREFIRE-1967] Fix parallel test ordering to prevent high resource consumption
Tibor17 commented on a change in pull request #407:
URL: https://github.com/apache/maven-surefire/pull/407#discussion_r774992573
##########
File path: 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));
Review comment:
Feel free to add the `throws` clause in the method signature and in the tests. Catching exceptions in tests should not be used in tests because it leads to situation where developers are tolerant to hiding errors.
##########
File path: surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGExecutor.java
##########
@@ -137,6 +152,31 @@ 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 )
+ {
+ try
+ {
+ // 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.
+ XML_CLASS_SET_INDEX.invoke( xmlClass, xmlClasses.size() );
+ }
+ catch ( ReflectiveOperationException e )
+ {
+ throw new RuntimeException( e );
+ }
+ }
Review comment:
These two constructors handle the same information across versions
`XmlClass(String className, int index, boolean loadClasses)`
`XmlClass(String name, Boolean declaredClass, int index)`
@juherr Can you ensure that the constructors, at least two, would not be removed in the future? Difficult question for IT development, I know. :-)
##########
File path: 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>
Review comment:
If the CPU is overloaded in Jenkins, this hypothesis may be broken and parallel threads turn to serial. Don;t rely on parallel execution even if you run parallel tests. Let's see the Jenkins test result especially on Windows...
##########
File path: surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGExecutor.java
##########
@@ -137,6 +152,31 @@ 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 )
+ {
+ try
+ {
+ // 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.
+ XML_CLASS_SET_INDEX.invoke( xmlClass, xmlClasses.size() );
+ }
+ catch ( ReflectiveOperationException e )
+ {
+ throw new RuntimeException( e );
+ }
+ }
Review comment:
Pls add a line with a comment regarding TestNG 5.10. It was very good point.
##########
File path: surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGExecutor.java
##########
@@ -137,6 +152,31 @@ 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 )
+ {
+ try
+ {
+ // 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.
+ XML_CLASS_SET_INDEX.invoke( xmlClass, xmlClasses.size() );
+ }
+ catch ( ReflectiveOperationException e )
+ {
+ throw new RuntimeException( e );
+ }
+ }
Review comment:
@findepi It would be an ideal situation to ensure same behavior in both versions. So in that case the constructor would help us `XmlClass(String className, int index, boolean loadClasses)`. Am I right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org