[GitHub] [maven-surefire] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r854356835
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +36,77 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
+/**
+ * Convert and apply the value of the [threadcount] option.
+ *
+ * @param suite TestNG {@link XmlSuite} object
+ * @param options Surefire plugin configuration options
+ * @throws TestSetFailedException if unable to convert specified
[threadcount] setting
+ */
@Override
-public void configure( XmlSuite suite, Map options )
+protected void configureThreadCount( XmlSuite suite, Map
options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+String threadCount = options.get( THREADCOUNT_PROP );
+// if [threadcount] spec'd
+if ( threadCount != null )
{
-if ( !"methods".equalsIgnoreCase( parallel ) &&
!"classes".equalsIgnoreCase( parallel ) )
+try
{
-throw new TestSetFailedException( "Unsupported TestNG parallel
setting: "
-+ parallel + " ( only METHODS or CLASSES supported )" );
+// convert and apply [threadcount] setting
+suite.setThreadCount( Integer.parseInt( threadCount ) );
Review Comment:
Override of [threadCount] implementation removed. New issue opened here:
https://issues.apache.org/jira/browse/SUREFIRE-2075
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r854356835
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +36,77 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
+/**
+ * Convert and apply the value of the [threadcount] option.
+ *
+ * @param suite TestNG {@link XmlSuite} object
+ * @param options Surefire plugin configuration options
+ * @throws TestSetFailedException if unable to convert specified
[threadcount] setting
+ */
@Override
-public void configure( XmlSuite suite, Map options )
+protected void configureThreadCount( XmlSuite suite, Map
options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+String threadCount = options.get( THREADCOUNT_PROP );
+// if [threadcount] spec'd
+if ( threadCount != null )
{
-if ( !"methods".equalsIgnoreCase( parallel ) &&
!"classes".equalsIgnoreCase( parallel ) )
+try
{
-throw new TestSetFailedException( "Unsupported TestNG parallel
setting: "
-+ parallel + " ( only METHODS or CLASSES supported )" );
+// convert and apply [threadcount] setting
+suite.setThreadCount( Integer.parseInt( threadCount ) );
Review Comment:
New issue opened here: https://issues.apache.org/jira/browse/SUREFIRE-2075
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r854334421
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +36,77 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
+/**
+ * Convert and apply the value of the [threadcount] option.
+ *
+ * @param suite TestNG {@link XmlSuite} object
+ * @param options Surefire plugin configuration options
+ * @throws TestSetFailedException if unable to convert specified
[threadcount] setting
+ */
@Override
-public void configure( XmlSuite suite, Map options )
+protected void configureThreadCount( XmlSuite suite, Map
options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+String threadCount = options.get( THREADCOUNT_PROP );
+// if [threadcount] spec'd
+if ( threadCount != null )
{
-if ( !"methods".equalsIgnoreCase( parallel ) &&
!"classes".equalsIgnoreCase( parallel ) )
+try
{
-throw new TestSetFailedException( "Unsupported TestNG parallel
setting: "
-+ parallel + " ( only METHODS or CLASSES supported )" );
+// convert and apply [threadcount] setting
+suite.setThreadCount( Integer.parseInt( threadCount ) );
Review Comment:
@Tibor17 As you indicated, TestNG provides a default value of 5. As I
pointed out, the Surefire documentation also specifies 5 as the default value.
There is no potential for the thread count to have any value other than 5
unless an explicit assignment is made.
I'll remove the code that overrides the default implementation for the
thread count parameter. I'll open another issue regarding the improper behavior
of the existing implementation (forcing the thread count to 1 in the absence of
an explicit assignment).
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r854334421
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +36,77 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
+/**
+ * Convert and apply the value of the [threadcount] option.
+ *
+ * @param suite TestNG {@link XmlSuite} object
+ * @param options Surefire plugin configuration options
+ * @throws TestSetFailedException if unable to convert specified
[threadcount] setting
+ */
@Override
-public void configure( XmlSuite suite, Map options )
+protected void configureThreadCount( XmlSuite suite, Map
options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+String threadCount = options.get( THREADCOUNT_PROP );
+// if [threadcount] spec'd
+if ( threadCount != null )
{
-if ( !"methods".equalsIgnoreCase( parallel ) &&
!"classes".equalsIgnoreCase( parallel ) )
+try
{
-throw new TestSetFailedException( "Unsupported TestNG parallel
setting: "
-+ parallel + " ( only METHODS or CLASSES supported )" );
+// convert and apply [threadcount] setting
+suite.setThreadCount( Integer.parseInt( threadCount ) );
Review Comment:
@Tibor17 As you indicated, TestNG provides a default value of 5. As I
pointed out, the Surefire documentation also specifies 5 as the default value.
There is no potential for the thread count to have anything other than 5 unless
an explicit assignment is made.
I'll remove the code that overrides the default implementation for the
thread count parameter. I'll open another issue regarding the improper behavior
of the existing implementation (forcing the thread count to 1 in the absence of
an explicit assignment).
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r851818368
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +36,77 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
+/**
+ * Convert and apply the value of the [threadcount] option.
+ *
+ * @param suite TestNG {@link XmlSuite} object
+ * @param options Surefire plugin configuration options
+ * @throws TestSetFailedException if unable to convert specified
[threadcount] setting
+ */
@Override
-public void configure( XmlSuite suite, Map options )
+protected void configureThreadCount( XmlSuite suite, Map
options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+String threadCount = options.get( THREADCOUNT_PROP );
+// if [threadcount] spec'd
+if ( threadCount != null )
{
-if ( !"methods".equalsIgnoreCase( parallel ) &&
!"classes".equalsIgnoreCase( parallel ) )
+try
{
-throw new TestSetFailedException( "Unsupported TestNG parallel
setting: "
-+ parallel + " ( only METHODS or CLASSES supported )" );
+// convert and apply [threadcount] setting
+suite.setThreadCount( Integer.parseInt( threadCount ) );
Review Comment:
I can remove the implementation that bypasses the override of TestNG's
default thread count, but it's unclear why the original code behaves this way.
As it's currently implemented, specifying any form of parallel execution
without explicitly specifying thread count will result in sequential execution.
The documentation
[here](https://maven.apache.org/surefire/maven-surefire-plugin/examples/testng.html#Running_Tests_in_Parallel)
indicates that the default is 5, which would be true if the implementation in
Surefire didn't impose a default value of 1.
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r851694849
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +36,77 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
+/**
+ * Convert and apply the value of the [threadcount] option.
+ *
+ * @param suite TestNG {@link XmlSuite} object
+ * @param options Surefire plugin configuration options
+ * @throws TestSetFailedException if unable to convert specified
[threadcount] setting
+ */
@Override
-public void configure( XmlSuite suite, Map options )
+protected void configureThreadCount( XmlSuite suite, Map
options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+String threadCount = options.get( THREADCOUNT_PROP );
+// if [threadcount] spec'd
+if ( threadCount != null )
{
-if ( !"methods".equalsIgnoreCase( parallel ) &&
!"classes".equalsIgnoreCase( parallel ) )
+try
{
-throw new TestSetFailedException( "Unsupported TestNG parallel
setting: "
-+ parallel + " ( only METHODS or CLASSES supported )" );
+// convert and apply [threadcount] setting
+suite.setThreadCount( Integer.parseInt( threadCount ) );
Review Comment:
If all supported versions of TestNG provide a default for this setting, it
may be best to not inject a default here. Just let TestNG provide its own
default value. That's how the new code for TestNG 7.4+ behaves. I don't know
the history of the original code to understand why this was coded to inject a
default value of 1. (What's the benefit of specifying parallel execution if the
thread count is 1?)
If the default implementation is changed so that it behaves like the new
override, this override can be removed.
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r851697867
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +36,77 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
+/**
+ * Convert and apply the value of the [threadcount] option.
+ *
+ * @param suite TestNG {@link XmlSuite} object
+ * @param options Surefire plugin configuration options
+ * @throws TestSetFailedException if unable to convert specified
[threadcount] setting
+ */
@Override
-public void configure( XmlSuite suite, Map options )
+protected void configureThreadCount( XmlSuite suite, Map
options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+String threadCount = options.get( THREADCOUNT_PROP );
+// if [threadcount] spec'd
+if ( threadCount != null )
{
-if ( !"methods".equalsIgnoreCase( parallel ) &&
!"classes".equalsIgnoreCase( parallel ) )
+try
{
-throw new TestSetFailedException( "Unsupported TestNG parallel
setting: "
-+ parallel + " ( only METHODS or CLASSES supported )" );
+// convert and apply [threadcount] setting
+suite.setThreadCount( Integer.parseInt( threadCount ) );
Review Comment:
Why would we override the default value provided by TestNG? What's the
benefit of specifying parallel execution if the thread count is 1?
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r851694849
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +36,77 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
+/**
+ * Convert and apply the value of the [threadcount] option.
+ *
+ * @param suite TestNG {@link XmlSuite} object
+ * @param options Surefire plugin configuration options
+ * @throws TestSetFailedException if unable to convert specified
[threadcount] setting
+ */
@Override
-public void configure( XmlSuite suite, Map options )
+protected void configureThreadCount( XmlSuite suite, Map
options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+String threadCount = options.get( THREADCOUNT_PROP );
+// if [threadcount] spec'd
+if ( threadCount != null )
{
-if ( !"methods".equalsIgnoreCase( parallel ) &&
!"classes".equalsIgnoreCase( parallel ) )
+try
{
-throw new TestSetFailedException( "Unsupported TestNG parallel
setting: "
-+ parallel + " ( only METHODS or CLASSES supported )" );
+// convert and apply [threadcount] setting
+suite.setThreadCount( Integer.parseInt( threadCount ) );
Review Comment:
If all supported versions of TestNG provide a default for this setting, it
may be best to not inject a default here. Just let TestNG provide its own
default value. That's how the new code for TestNG 7.4+ behaves. I don't know
the history of the original code to understand why this was coded to inject a
default value of 1.
If the default implementation is changed so that it behaves like the new
override, this override can be removed.
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r851694849
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +36,77 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
+/**
+ * Convert and apply the value of the [threadcount] option.
+ *
+ * @param suite TestNG {@link XmlSuite} object
+ * @param options Surefire plugin configuration options
+ * @throws TestSetFailedException if unable to convert specified
[threadcount] setting
+ */
@Override
-public void configure( XmlSuite suite, Map options )
+protected void configureThreadCount( XmlSuite suite, Map
options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+String threadCount = options.get( THREADCOUNT_PROP );
+// if [threadcount] spec'd
+if ( threadCount != null )
{
-if ( !"methods".equalsIgnoreCase( parallel ) &&
!"classes".equalsIgnoreCase( parallel ) )
+try
{
-throw new TestSetFailedException( "Unsupported TestNG parallel
setting: "
-+ parallel + " ( only METHODS or CLASSES supported )" );
+// convert and apply [threadcount] setting
+suite.setThreadCount( Integer.parseInt( threadCount ) );
Review Comment:
If all supported versions of TestNG provide a default for this setting, it
may be best to not inject a default here. Just let TestNG provide its own
default value. That's how the new code for TestNG 7.4+ behaves. I don't know
the history of the original code to understand why this was coded to inject a
default value of 1.
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r847868965
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +36,59 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
@Override
-public void configure( XmlSuite suite, Map options )
+protected void configureThreadCount( XmlSuite suite, Map
options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+String threadCount = options.get( THREADCOUNT_PROP );
+// if [threadcount] spec'd
+if ( threadCount != null )
{
-if ( !"methods".equalsIgnoreCase( parallel ) &&
!"classes".equalsIgnoreCase( parallel ) )
+try
{
-throw new TestSetFailedException( "Unsupported TestNG parallel
setting: "
-+ parallel + " ( only METHODS or CLASSES supported )" );
+// convert and apply [threadcount] setting
+suite.setThreadCount( Integer.parseInt( threadCount ) );
+}
+catch ( NumberFormatException e )
+{
+throw new TestSetFailedException( "Non-integer TestNG
[threadcount] setting: " + threadCount, e );
}
-Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
-Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
-invokeSetter( suite, "setParallel", enumClass, parallelEnum );
}
-
-String dataProviderThreadCount = options.get(
"dataproviderthreadcount" );
-if ( dataProviderThreadCount != null )
+}
+
+@Override
+protected void configureParallel( XmlSuite suite, Map
options )
+throws TestSetFailedException
+{
+String parallel = options.get( PARALLEL_PROP );
+// if [parallel] spec'd
+if ( parallel != null )
{
-suite.setDataProviderThreadCount( Integer.parseInt(
dataProviderThreadCount ) );
+// try to load the [ParallelMode] enumeration
+Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
+// if enumeration loaded
+if ( enumClass != null )
+{
+try
+{
+// convert [parallel] option to corresponding constant
+Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
+// set [XmlSuite] parallel mode to specified value
+invokeSetter( suite, "setParallel", enumClass,
parallelEnum );
Review Comment:
@slawekjaranowski: I added JavaDoc to the configuration methods. The
documentation for the `configureParallel` method explains the rationale behind
this method, specifying the TestNG release that made this necessary.
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r847843603
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +36,59 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
@Override
-public void configure( XmlSuite suite, Map options )
+protected void configureThreadCount( XmlSuite suite, Map
options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+String threadCount = options.get( THREADCOUNT_PROP );
+// if [threadcount] spec'd
+if ( threadCount != null )
{
-if ( !"methods".equalsIgnoreCase( parallel ) &&
!"classes".equalsIgnoreCase( parallel ) )
+try
{
-throw new TestSetFailedException( "Unsupported TestNG parallel
setting: "
-+ parallel + " ( only METHODS or CLASSES supported )" );
+// convert and apply [threadcount] setting
+suite.setThreadCount( Integer.parseInt( threadCount ) );
+}
+catch ( NumberFormatException e )
+{
+throw new TestSetFailedException( "Non-integer TestNG
[threadcount] setting: " + threadCount, e );
}
-Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
-Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
-invokeSetter( suite, "setParallel", enumClass, parallelEnum );
}
-
-String dataProviderThreadCount = options.get(
"dataproviderthreadcount" );
-if ( dataProviderThreadCount != null )
+}
+
+@Override
+protected void configureParallel( XmlSuite suite, Map
options )
+throws TestSetFailedException
+{
+String parallel = options.get( PARALLEL_PROP );
+// if [parallel] spec'd
+if ( parallel != null )
{
-suite.setDataProviderThreadCount( Integer.parseInt(
dataProviderThreadCount ) );
+// try to load the [ParallelMode] enumeration
+Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
+// if enumeration loaded
+if ( enumClass != null )
+{
+try
+{
+// convert [parallel] option to corresponding constant
+Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
+// set [XmlSuite] parallel mode to specified value
+invokeSetter( suite, "setParallel", enumClass,
parallelEnum );
Review Comment:
@Tibor17: Regarding FQN, the **ParallelMode** enumeration is a nested class,
so the name is properly spelt. However, your suggestion would be correct for an
`import` statement. We would need to have the **XmlSuite** class definition
from TestNG 7.4 for that to work, 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
[GitHub] [maven-surefire] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r847840590
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +36,59 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
@Override
-public void configure( XmlSuite suite, Map options )
+protected void configureThreadCount( XmlSuite suite, Map
options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+String threadCount = options.get( THREADCOUNT_PROP );
+// if [threadcount] spec'd
+if ( threadCount != null )
{
-if ( !"methods".equalsIgnoreCase( parallel ) &&
!"classes".equalsIgnoreCase( parallel ) )
+try
{
-throw new TestSetFailedException( "Unsupported TestNG parallel
setting: "
-+ parallel + " ( only METHODS or CLASSES supported )" );
+// convert and apply [threadcount] setting
+suite.setThreadCount( Integer.parseInt( threadCount ) );
+}
+catch ( NumberFormatException e )
+{
+throw new TestSetFailedException( "Non-integer TestNG
[threadcount] setting: " + threadCount, e );
}
-Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
-Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
-invokeSetter( suite, "setParallel", enumClass, parallelEnum );
}
-
-String dataProviderThreadCount = options.get(
"dataproviderthreadcount" );
-if ( dataProviderThreadCount != null )
+}
+
+@Override
+protected void configureParallel( XmlSuite suite, Map
options )
+throws TestSetFailedException
+{
+String parallel = options.get( PARALLEL_PROP );
+// if [parallel] spec'd
+if ( parallel != null )
{
-suite.setDataProviderThreadCount( Integer.parseInt(
dataProviderThreadCount ) );
+// try to load the [ParallelMode] enumeration
+Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
+// if enumeration loaded
+if ( enumClass != null )
+{
+try
+{
+// convert [parallel] option to corresponding constant
+Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
+// set [XmlSuite] parallel mode to specified value
+invokeSetter( suite, "setParallel", enumClass,
parallelEnum );
Review Comment:
@slawekjaranowski: The class header JavaDoc specifies the TestNG version
(`7.4.0`). I suppose this comment could be a bit more explicit, however,
regarding the necessity for this configurator class.
@Tibor17: The inability to load the enumeration will trigger a
clearly-explained failure: `Failed loading TestNG [ParallelMode] enumeration to
convert [parallel] setting`. No need to crack open the code to determine the
cause of a generic NPE (which will be provided as the cause of the
**TestSetFailedException**).
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r847840590
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +36,59 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
@Override
-public void configure( XmlSuite suite, Map options )
+protected void configureThreadCount( XmlSuite suite, Map
options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+String threadCount = options.get( THREADCOUNT_PROP );
+// if [threadcount] spec'd
+if ( threadCount != null )
{
-if ( !"methods".equalsIgnoreCase( parallel ) &&
!"classes".equalsIgnoreCase( parallel ) )
+try
{
-throw new TestSetFailedException( "Unsupported TestNG parallel
setting: "
-+ parallel + " ( only METHODS or CLASSES supported )" );
+// convert and apply [threadcount] setting
+suite.setThreadCount( Integer.parseInt( threadCount ) );
+}
+catch ( NumberFormatException e )
+{
+throw new TestSetFailedException( "Non-integer TestNG
[threadcount] setting: " + threadCount, e );
}
-Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
-Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
-invokeSetter( suite, "setParallel", enumClass, parallelEnum );
}
-
-String dataProviderThreadCount = options.get(
"dataproviderthreadcount" );
-if ( dataProviderThreadCount != null )
+}
+
+@Override
+protected void configureParallel( XmlSuite suite, Map
options )
+throws TestSetFailedException
+{
+String parallel = options.get( PARALLEL_PROP );
+// if [parallel] spec'd
+if ( parallel != null )
{
-suite.setDataProviderThreadCount( Integer.parseInt(
dataProviderThreadCount ) );
+// try to load the [ParallelMode] enumeration
+Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
+// if enumeration loaded
+if ( enumClass != null )
+{
+try
+{
+// convert [parallel] option to corresponding constant
+Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
+// set [XmlSuite] parallel mode to specified value
+invokeSetter( suite, "setParallel", enumClass,
parallelEnum );
Review Comment:
@slawekjaranowski: The class header JavaDoc specifies the TestNG version
(`7.4.0`). I suppose this comment could be a bit more explicit, however,
regarding the necessity for this configurator class.
@Tibor17: The inability to load the enumeration will trigger a
clearly-explained failure: `Failed loading TestNG [ParallelMode] enumeration to
convert [parallel] setting`. No need to crack open the code to determine the
cause of a generic NPE (which will be provided as the cause of the
**TestSetFailedException**.
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r847840590
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +36,59 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
@Override
-public void configure( XmlSuite suite, Map options )
+protected void configureThreadCount( XmlSuite suite, Map
options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+String threadCount = options.get( THREADCOUNT_PROP );
+// if [threadcount] spec'd
+if ( threadCount != null )
{
-if ( !"methods".equalsIgnoreCase( parallel ) &&
!"classes".equalsIgnoreCase( parallel ) )
+try
{
-throw new TestSetFailedException( "Unsupported TestNG parallel
setting: "
-+ parallel + " ( only METHODS or CLASSES supported )" );
+// convert and apply [threadcount] setting
+suite.setThreadCount( Integer.parseInt( threadCount ) );
+}
+catch ( NumberFormatException e )
+{
+throw new TestSetFailedException( "Non-integer TestNG
[threadcount] setting: " + threadCount, e );
}
-Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
-Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
-invokeSetter( suite, "setParallel", enumClass, parallelEnum );
}
-
-String dataProviderThreadCount = options.get(
"dataproviderthreadcount" );
-if ( dataProviderThreadCount != null )
+}
+
+@Override
+protected void configureParallel( XmlSuite suite, Map
options )
+throws TestSetFailedException
+{
+String parallel = options.get( PARALLEL_PROP );
+// if [parallel] spec'd
+if ( parallel != null )
{
-suite.setDataProviderThreadCount( Integer.parseInt(
dataProviderThreadCount ) );
+// try to load the [ParallelMode] enumeration
+Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
+// if enumeration loaded
+if ( enumClass != null )
+{
+try
+{
+// convert [parallel] option to corresponding constant
+Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
+// set [XmlSuite] parallel mode to specified value
+invokeSetter( suite, "setParallel", enumClass,
parallelEnum );
Review Comment:
@slawekjaranowski: The class header JavaDoc specifies the TestNG version
(`7.4.0`). I suppose this comment could be a bit more explicit, however,
regarding the necessity for this configurator class.
@Tibor17: The inability to load the enumeration will trigger a
clearly-explained failure: `Failed loading TestNG [ParallelMode] enumeration to
convert [parallel] setting`. No need to crack open the code to determine the
cause of a generic NPE.
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r847840590
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +36,59 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
@Override
-public void configure( XmlSuite suite, Map options )
+protected void configureThreadCount( XmlSuite suite, Map
options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+String threadCount = options.get( THREADCOUNT_PROP );
+// if [threadcount] spec'd
+if ( threadCount != null )
{
-if ( !"methods".equalsIgnoreCase( parallel ) &&
!"classes".equalsIgnoreCase( parallel ) )
+try
{
-throw new TestSetFailedException( "Unsupported TestNG parallel
setting: "
-+ parallel + " ( only METHODS or CLASSES supported )" );
+// convert and apply [threadcount] setting
+suite.setThreadCount( Integer.parseInt( threadCount ) );
+}
+catch ( NumberFormatException e )
+{
+throw new TestSetFailedException( "Non-integer TestNG
[threadcount] setting: " + threadCount, e );
}
-Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
-Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
-invokeSetter( suite, "setParallel", enumClass, parallelEnum );
}
-
-String dataProviderThreadCount = options.get(
"dataproviderthreadcount" );
-if ( dataProviderThreadCount != null )
+}
+
+@Override
+protected void configureParallel( XmlSuite suite, Map
options )
+throws TestSetFailedException
+{
+String parallel = options.get( PARALLEL_PROP );
+// if [parallel] spec'd
+if ( parallel != null )
{
-suite.setDataProviderThreadCount( Integer.parseInt(
dataProviderThreadCount ) );
+// try to load the [ParallelMode] enumeration
+Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
+// if enumeration loaded
+if ( enumClass != null )
+{
+try
+{
+// convert [parallel] option to corresponding constant
+Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
+// set [XmlSuite] parallel mode to specified value
+invokeSetter( suite, "setParallel", enumClass,
parallelEnum );
Review Comment:
@slawekjaranowski: The class header JavaDoc specifies the TestNG version
(`7.4.0`). I suppose this comment could be a bit more explicit, however,
regarding the necessity for this configurator class.
@Tibor17: The inability to load the enumeration will trigger a
clearly-explained failure: `Failed loading TestNG [ParallelMode] enumeration to
convert [parallel] setting`. No need to crack open the code to determine the
cause of the NPE.
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r847840590
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +36,59 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
@Override
-public void configure( XmlSuite suite, Map options )
+protected void configureThreadCount( XmlSuite suite, Map
options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+String threadCount = options.get( THREADCOUNT_PROP );
+// if [threadcount] spec'd
+if ( threadCount != null )
{
-if ( !"methods".equalsIgnoreCase( parallel ) &&
!"classes".equalsIgnoreCase( parallel ) )
+try
{
-throw new TestSetFailedException( "Unsupported TestNG parallel
setting: "
-+ parallel + " ( only METHODS or CLASSES supported )" );
+// convert and apply [threadcount] setting
+suite.setThreadCount( Integer.parseInt( threadCount ) );
+}
+catch ( NumberFormatException e )
+{
+throw new TestSetFailedException( "Non-integer TestNG
[threadcount] setting: " + threadCount, e );
}
-Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
-Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
-invokeSetter( suite, "setParallel", enumClass, parallelEnum );
}
-
-String dataProviderThreadCount = options.get(
"dataproviderthreadcount" );
-if ( dataProviderThreadCount != null )
+}
+
+@Override
+protected void configureParallel( XmlSuite suite, Map
options )
+throws TestSetFailedException
+{
+String parallel = options.get( PARALLEL_PROP );
+// if [parallel] spec'd
+if ( parallel != null )
{
-suite.setDataProviderThreadCount( Integer.parseInt(
dataProviderThreadCount ) );
+// try to load the [ParallelMode] enumeration
+Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
+// if enumeration loaded
+if ( enumClass != null )
+{
+try
+{
+// convert [parallel] option to corresponding constant
+Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
+// set [XmlSuite] parallel mode to specified value
+invokeSetter( suite, "setParallel", enumClass,
parallelEnum );
Review Comment:
@slawekjaranowski: The class header JavaDoc specifies the TestNG version
(`7.4.0`). I suppose this comment could be a bit more explicit, however,
regarding the necessity for this configurator class.
@Tibor17: The inability to load the enumeration will trigger a
clearly-explained failure: `Failed loading TestNG [ParallelMode] enumeration to
convert [parallel] setting`
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517: URL: https://github.com/apache/maven-surefire/pull/517#discussion_r846684997 ## surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java: ## @@ -37,33 +35,45 @@ * * @since 3.0.0-M6 */ -public class TestNG740Configurator extends TestNG60Configurator +public class TestNG740Configurator +extends TestNG60Configurator Review Comment: This specific revision was a minor format change to match the style used throughout this project. -- 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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517: URL: https://github.com/apache/maven-surefire/pull/517#discussion_r846684997 ## surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java: ## @@ -37,33 +35,45 @@ * * @since 3.0.0-M6 */ -public class TestNG740Configurator extends TestNG60Configurator +public class TestNG740Configurator +extends TestNG60Configurator Review Comment: This specific change was a minor format change to match the style used throughout this project. -- 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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r846676868
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +35,45 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
@Override
public void configure( XmlSuite suite, Map options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+// if [threadcount] option in unspecified
+if ( !options.containsKey( THREADCOUNT_PROP ) )
{
-if ( !"methods".equalsIgnoreCase( parallel ) &&
!"classes".equalsIgnoreCase( parallel ) )
-{
-throw new TestSetFailedException( "Unsupported TestNG parallel
setting: "
-+ parallel + " ( only METHODS or CLASSES supported )" );
-}
-Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
-Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
-invokeSetter( suite, "setParallel", enumClass, parallelEnum );
+// acquire default [threadcount] value to avoid superclass
hardcoding to 1
+options.put( THREADCOUNT_PROP, Integer.toString(
suite.getThreadCount() ) );
}
-String dataProviderThreadCount = options.get(
"dataproviderthreadcount" );
-if ( dataProviderThreadCount != null )
+// if [parallel] option is specified
+if ( options.containsKey( PARALLEL_PROP ) )
{
-suite.setDataProviderThreadCount( Integer.parseInt(
dataProviderThreadCount ) );
+// try to load the [ParallelMode] enumeration
+Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
+// if enumeration loaded
+if ( enumClass != null )
+{
+// extract [parallel] option
+String parallel = options.remove( PARALLEL_PROP );
+try
+{
+// convert [parallel] option to corresponding constant
+Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
+// set [XmlSuite] parallel mode to specified value
+invokeSetter( suite, "setParallel", enumClass,
parallelEnum );
+}
+catch ( IllegalArgumentException e )
+{
+throw new TestSetFailedException( "Unsupported TestNG
parallel setting: " + parallel, e );
+}
+}
}
+
+// invoke superclass handler
+super.configure( suite, options );
Review Comment:
I just committed revisions that use the refactoring approach you
recommended. This eliminates the need for careful management of the options map
and makes the code more readable. It also adds more error detection with
improved reporting of configuration issues.
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r846676868
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +35,45 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
@Override
public void configure( XmlSuite suite, Map options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+// if [threadcount] option in unspecified
+if ( !options.containsKey( THREADCOUNT_PROP ) )
{
-if ( !"methods".equalsIgnoreCase( parallel ) &&
!"classes".equalsIgnoreCase( parallel ) )
-{
-throw new TestSetFailedException( "Unsupported TestNG parallel
setting: "
-+ parallel + " ( only METHODS or CLASSES supported )" );
-}
-Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
-Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
-invokeSetter( suite, "setParallel", enumClass, parallelEnum );
+// acquire default [threadcount] value to avoid superclass
hardcoding to 1
+options.put( THREADCOUNT_PROP, Integer.toString(
suite.getThreadCount() ) );
}
-String dataProviderThreadCount = options.get(
"dataproviderthreadcount" );
-if ( dataProviderThreadCount != null )
+// if [parallel] option is specified
+if ( options.containsKey( PARALLEL_PROP ) )
{
-suite.setDataProviderThreadCount( Integer.parseInt(
dataProviderThreadCount ) );
+// try to load the [ParallelMode] enumeration
+Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
+// if enumeration loaded
+if ( enumClass != null )
+{
+// extract [parallel] option
+String parallel = options.remove( PARALLEL_PROP );
+try
+{
+// convert [parallel] option to corresponding constant
+Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
+// set [XmlSuite] parallel mode to specified value
+invokeSetter( suite, "setParallel", enumClass,
parallelEnum );
+}
+catch ( IllegalArgumentException e )
+{
+throw new TestSetFailedException( "Unsupported TestNG
parallel setting: " + parallel, e );
+}
+}
}
+
+// invoke superclass handler
+super.configure( suite, options );
Review Comment:
I just committed revisions that use the refactoring approach you
recommended. This eliminates the need for careful management of the options map
and makes the code more readable.
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r846669797
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +35,45 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
@Override
public void configure( XmlSuite suite, Map options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+// if [threadcount] option in unspecified
Review Comment:
Handling of the [dataproviderthreadcount] option here was also redundant and
has likewise been removed.
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r846669309
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +35,45 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
@Override
public void configure( XmlSuite suite, Map options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+// if [threadcount] option in unspecified
+if ( !options.containsKey( THREADCOUNT_PROP ) )
{
-if ( !"methods".equalsIgnoreCase( parallel ) &&
!"classes".equalsIgnoreCase( parallel ) )
-{
-throw new TestSetFailedException( "Unsupported TestNG parallel
setting: "
-+ parallel + " ( only METHODS or CLASSES supported )" );
-}
-Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
-Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
-invokeSetter( suite, "setParallel", enumClass, parallelEnum );
+// acquire default [threadcount] value to avoid superclass
hardcoding to 1
+options.put( THREADCOUNT_PROP, Integer.toString(
suite.getThreadCount() ) );
}
-String dataProviderThreadCount = options.get(
"dataproviderthreadcount" );
-if ( dataProviderThreadCount != null )
+// if [parallel] option is specified
+if ( options.containsKey( PARALLEL_PROP ) )
{
-suite.setDataProviderThreadCount( Integer.parseInt(
dataProviderThreadCount ) );
+// try to load the [ParallelMode] enumeration
+Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
+// if enumeration loaded
+if ( enumClass != null )
+{
+// extract [parallel] option
+String parallel = options.remove( PARALLEL_PROP );
+try
+{
+// convert [parallel] option to corresponding constant
+Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
+// set [XmlSuite] parallel mode to specified value
+invokeSetter( suite, "setParallel", enumClass,
parallelEnum );
+}
+catch ( IllegalArgumentException e )
+{
+throw new TestSetFailedException( "Unsupported TestNG
parallel setting: " + parallel, e );
+}
+}
}
+
+// invoke superclass handler
+super.configure( suite, options );
Review Comment:
I could do as you suggest and extract the logic to `setThreadCount` and
`setParallel` methods. My current approach was intended to limit the scope of
revisions to this single class. Would you prefer that I refactor the
implementation? This would reduce the complexity a bit while expanding the
scope of this PR.
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r846669309
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +35,45 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
@Override
public void configure( XmlSuite suite, Map options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+// if [threadcount] option in unspecified
+if ( !options.containsKey( THREADCOUNT_PROP ) )
{
-if ( !"methods".equalsIgnoreCase( parallel ) &&
!"classes".equalsIgnoreCase( parallel ) )
-{
-throw new TestSetFailedException( "Unsupported TestNG parallel
setting: "
-+ parallel + " ( only METHODS or CLASSES supported )" );
-}
-Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
-Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
-invokeSetter( suite, "setParallel", enumClass, parallelEnum );
+// acquire default [threadcount] value to avoid superclass
hardcoding to 1
+options.put( THREADCOUNT_PROP, Integer.toString(
suite.getThreadCount() ) );
}
-String dataProviderThreadCount = options.get(
"dataproviderthreadcount" );
-if ( dataProviderThreadCount != null )
+// if [parallel] option is specified
+if ( options.containsKey( PARALLEL_PROP ) )
{
-suite.setDataProviderThreadCount( Integer.parseInt(
dataProviderThreadCount ) );
+// try to load the [ParallelMode] enumeration
+Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
+// if enumeration loaded
+if ( enumClass != null )
+{
+// extract [parallel] option
+String parallel = options.remove( PARALLEL_PROP );
+try
+{
+// convert [parallel] option to corresponding constant
+Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
+// set [XmlSuite] parallel mode to specified value
+invokeSetter( suite, "setParallel", enumClass,
parallelEnum );
+}
+catch ( IllegalArgumentException e )
+{
+throw new TestSetFailedException( "Unsupported TestNG
parallel setting: " + parallel, e );
+}
+}
}
+
+// invoke superclass handler
+super.configure( suite, options );
Review Comment:
I could do as you suggest and extract the logic to `setThreadCount` and
setParallel` methods. My current approach was intended to limit the scope of
revisions to this single class. Would you prefer that I refactor the
implementation? This would reduce the complexity a bit while expanding the
scope of this PR.
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r846668357
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +35,45 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
@Override
public void configure( XmlSuite suite, Map options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+// if [threadcount] option in unspecified
+if ( !options.containsKey( THREADCOUNT_PROP ) )
{
-if ( !"methods".equalsIgnoreCase( parallel ) &&
!"classes".equalsIgnoreCase( parallel ) )
-{
-throw new TestSetFailedException( "Unsupported TestNG parallel
setting: "
-+ parallel + " ( only METHODS or CLASSES supported )" );
-}
-Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
-Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
-invokeSetter( suite, "setParallel", enumClass, parallelEnum );
+// acquire default [threadcount] value to avoid superclass
hardcoding to 1
+options.put( THREADCOUNT_PROP, Integer.toString(
suite.getThreadCount() ) );
}
-String dataProviderThreadCount = options.get(
"dataproviderthreadcount" );
-if ( dataProviderThreadCount != null )
+// if [parallel] option is specified
+if ( options.containsKey( PARALLEL_PROP ) )
{
-suite.setDataProviderThreadCount( Integer.parseInt(
dataProviderThreadCount ) );
+// try to load the [ParallelMode] enumeration
+Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
+// if enumeration loaded
+if ( enumClass != null )
+{
+// extract [parallel] option
+String parallel = options.remove( PARALLEL_PROP );
+try
+{
+// convert [parallel] option to corresponding constant
+Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
+// set [XmlSuite] parallel mode to specified value
+invokeSetter( suite, "setParallel", enumClass,
parallelEnum );
+}
+catch ( IllegalArgumentException e )
+{
+throw new TestSetFailedException( "Unsupported TestNG
parallel setting: " + parallel, e );
+}
+}
}
+
+// invoke superclass handler
+super.configure( suite, options );
Review Comment:
Look closer... The value of the [parallel] option is being **removed** from
the map. I'm processing the option here, and the removal of the option avoids
double-processing (with the resultant failure) in the superclass implementation.
As I pointed out in my comments, skipping the invocation of the superclass
implementation has the result that all other specified options get ignored.
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r846668357
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +35,45 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
@Override
public void configure( XmlSuite suite, Map options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+// if [threadcount] option in unspecified
+if ( !options.containsKey( THREADCOUNT_PROP ) )
{
-if ( !"methods".equalsIgnoreCase( parallel ) &&
!"classes".equalsIgnoreCase( parallel ) )
-{
-throw new TestSetFailedException( "Unsupported TestNG parallel
setting: "
-+ parallel + " ( only METHODS or CLASSES supported )" );
-}
-Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
-Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
-invokeSetter( suite, "setParallel", enumClass, parallelEnum );
+// acquire default [threadcount] value to avoid superclass
hardcoding to 1
+options.put( THREADCOUNT_PROP, Integer.toString(
suite.getThreadCount() ) );
}
-String dataProviderThreadCount = options.get(
"dataproviderthreadcount" );
-if ( dataProviderThreadCount != null )
+// if [parallel] option is specified
+if ( options.containsKey( PARALLEL_PROP ) )
{
-suite.setDataProviderThreadCount( Integer.parseInt(
dataProviderThreadCount ) );
+// try to load the [ParallelMode] enumeration
+Class enumClass = tryLoadClass( XmlSuite.class.getClassLoader(),
"org.testng.xml.XmlSuite$ParallelMode" );
+// if enumeration loaded
+if ( enumClass != null )
+{
+// extract [parallel] option
+String parallel = options.remove( PARALLEL_PROP );
+try
+{
+// convert [parallel] option to corresponding constant
+Enum parallelEnum = Enum.valueOf( enumClass,
parallel.toUpperCase() );
+// set [XmlSuite] parallel mode to specified value
+invokeSetter( suite, "setParallel", enumClass,
parallelEnum );
+}
+catch ( IllegalArgumentException e )
+{
+throw new TestSetFailedException( "Unsupported TestNG
parallel setting: " + parallel, e );
+}
+}
}
+
+// invoke superclass handler
+super.configure( suite, options );
Review Comment:
Look closer... The value of the [parallel] option is being **removed** from
the map. I'm processing the option here, and the removal of the option avoids
double-processing (with the resultant failure) in the superclass implementation.
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517:
URL: https://github.com/apache/maven-surefire/pull/517#discussion_r846668194
##
surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java:
##
@@ -37,33 +35,45 @@
*
* @since 3.0.0-M6
*/
-public class TestNG740Configurator extends TestNG60Configurator
+public class TestNG740Configurator
+extends TestNG60Configurator
{
@Override
public void configure( XmlSuite suite, Map options )
throws TestSetFailedException
{
-String threadCountAsString = options.get( THREADCOUNT_PROP );
-int threadCount = threadCountAsString == null ? 1 : parseInt(
threadCountAsString );
-suite.setThreadCount( threadCount );
-
-String parallel = options.get( PARALLEL_PROP );
-if ( parallel != null )
+// if [threadcount] option in unspecified
Review Comment:
This code is redundant. The superclass implementation already processes the
[threadcount] option. The reason I'm acquiring the default value when the
option is unspecified is that the superclass implementation assigns a default
value of 1, whereas TestNG currently assigns a default value of 5.
--
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] sbabcoc commented on a diff in pull request #517: [SUREFIRE-2064] Allow all supported values of [parallel] option
sbabcoc commented on code in PR #517: URL: https://github.com/apache/maven-surefire/pull/517#discussion_r846510472 ## surefire-providers/surefire-testng/src/main/java/org/apache/maven/surefire/testng/conf/TestNG740Configurator.java: ## @@ -37,33 +35,45 @@ * * @since 3.0.0-M6 */ -public class TestNG740Configurator extends TestNG60Configurator +public class TestNG740Configurator +extends TestNG60Configurator Review Comment: This is the style employed by **all** of the other classes in this project. -- 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
