[GitHub] [maven-surefire] findepi commented on a change in pull request #407: [SUREFIRE-1967] Fix parallel test ordering to prevent high resource consumption
findepi commented on a change in pull request #407: URL: https://github.com/apache/maven-surefire/pull/407#discussion_r783434179 ## File path: surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGExecutor.java ## @@ -137,6 +152,31 @@ static void run( Iterable> testClasses, String testSourceDirectory, testng.run(); } +private static void addXmlClass( List 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. You mean this one: _The code is compiled against TestNG 5.10 and that version doesn't have the `index` field in `XmlClass`, hence use of reflection._ ? I wouldn't want to put exact TestNG 5.10 version in the source, as it's subject to change, but i added explanatory comment above XML_CLASS_SET_INDEX constant: ``` // 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. ``` i tried to make it convey same meaning: that we have to use the reflection. -- 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-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [maven-surefire] findepi commented on a change in pull request #407: [SUREFIRE-1967] Fix parallel test ordering to prevent high resource consumption
findepi commented on a change in pull request #407: URL: https://github.com/apache/maven-surefire/pull/407#discussion_r783424283 ## File path: surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGExecutor.java ## @@ -137,6 +152,31 @@ static void run( Iterable> testClasses, String testSourceDirectory, testng.run(); } +private static void addXmlClass( List 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: @Tibor17 it's possible to select and invoke one of the constructors, but i don't think the added complexity is worth the effort (i am mostly concerned about future code readability and maintainability). Note that after all, we're solving a problem that exists in TestNG >= 6.3, and TestNG 5.x is not going to change. The m_index field is used in TestNG 5.x when `preserve-order` is set, but it's not possible to set it when invoking tests via surefire. More precisely, `preserve-order` seems to require a suite file, but then TestNG will populate the m_index, not us. -- 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-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [maven-surefire] findepi commented on a change in pull request #407: [SUREFIRE-1967] Fix parallel test ordering to prevent high resource consumption
findepi commented on a change in pull request #407: URL: https://github.com/apache/maven-surefire/pull/407#discussion_r776713600 ## File path: surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/pom.xml ## @@ -0,0 +1,60 @@ + + + +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";> + 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} + Review comment: I understand that parameterizing threadCount would allow me to run tests with different threadCount values. Would we actually want to run the test more times? What additional value would it bring? -- 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-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [maven-surefire] findepi commented on a change in pull request #407: [SUREFIRE-1967] Fix parallel test ordering to prevent high resource consumption
findepi commented on a change in pull request #407: URL: https://github.com/apache/maven-surefire/pull/407#discussion_r776708306 ## File path: surefire-its/src/test/resources/surefire-1967-testng-method-parallel-ordering/pom.xml ## @@ -0,0 +1,60 @@ + + + +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";> + 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} + Review comment: > The main advantage in adding this cosmetic change is that you could easily test the behaviour with multiple values for the `threadCount` i don't see advantage of doing this. Please help me understand. -- 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-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [maven-surefire] findepi commented on a change in pull request #407: [SUREFIRE-1967] Fix parallel test ordering to prevent high resource consumption
findepi commented on a change in pull request #407: URL: https://github.com/apache/maven-surefire/pull/407#discussion_r769037716 ## File path: surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGExecutor.java ## @@ -137,6 +152,31 @@ static void run( Iterable> testClasses, String testSourceDirectory, testng.run(); } +private static void addXmlClass( List 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: The code is compiled against TestNG 5.10 and that version doesn't have the `index` field in `XmlClass`, hence use of reflection. > Instead of setting the m_index via reflection we could use the appropriate constructor to set it: Yes, I can lookup a constructor taking `(String.class, int.class)` arguments. It's not as robust or future-proof as finding a `setIndex` method though. -- 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-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org