[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344: URL: https://github.com/apache/maven-surefire/pull/344#discussion_r608118433 ## File path: maven-surefire-plugin/src/site/apt/examples/junit-platform.apt.vm ## @@ -526,6 +526,9 @@ else * To exclude <<>>, use <<>>. + Be aware that this feature reserve system properties <<>> and <<>> Review comment: `reserve` should become `reserves`, 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344: URL: https://github.com/apache/maven-surefire/pull/344#discussion_r608076370 ## File path: maven-failsafe-plugin/src/main/java/org/apache/maven/plugin/failsafe/IntegrationTestMojo.java ## @@ -476,10 +476,16 @@ @Parameter( property = "failsafe.systemPropertiesFile" ) private File systemPropertiesFile; -@Parameter( property = "junitIncludeEngine" ) +/** + * Provide the ID/s of an JUnit engine to be included in the test run. Review comment: We have very similar problem with `listener` in `testng.apt.vm`. You should think of users and us, because if we do not mention that, the users will report a bug against us that the Provider fails. They would not notice that we also use the same property. -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344: URL: https://github.com/apache/maven-surefire/pull/344#discussion_r607978413 ## File path: maven-failsafe-plugin/src/main/java/org/apache/maven/plugin/failsafe/IntegrationTestMojo.java ## @@ -476,10 +476,16 @@ @Parameter( property = "failsafe.systemPropertiesFile" ) private File systemPropertiesFile; -@Parameter( property = "junitIncludeEngine" ) +/** + * Provide the ID/s of an JUnit engine to be included in the test run. Review comment: This looks much better now. The only problem is the version we are missing. We should say that this property is available since the version 3.0.0-M6. And the system property `junitincludeengine` is reserved keyword. Similar with the next one. Thx -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r607975505
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -1568,16 +1568,30 @@ private void convertJunitEngineParameters()
{
if ( getIncludeJUnit5Engines() != null )
{
-String includeJUnit5Engines = Arrays.toString(
getIncludeJUnit5Engines() ).replaceAll( "\\[\\]", "" );
-getProperties().setProperty( INCLUDE_JUNIT5_ENGINES_PROP,
includeJUnit5Engines );
+getProperties().setProperty( INCLUDE_JUNIT5_ENGINES_PROP,
Review comment:
Pls move the Lines 23 and 24 below the line 156. It is the static
import. Thx
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r607851990
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -1571,13 +1566,15 @@ private void convertGroupParameters()
private void convertJunitEngineParameters()
{
-if ( isNotBlank( getJunitIncludeEngine() ) )
+if ( getIncludeJUnit5Engines() != null )
{
-getProperties().setProperty( JUNIT_INCLUDE_ENGINE_PROP,
getJunitIncludeEngine() );
+String includeJUnit5Engines = Arrays.toString(
getIncludeJUnit5Engines() ).replaceAll( "\\[\\]", "" );
Review comment:
Pls do not use `Arrays.toString`. Nobody does this way. Use a typical
loop. In Java 8 we will have string joinrer but now pls use a loop and
StringBuilder.
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344: URL: https://github.com/apache/maven-surefire/pull/344#discussion_r606372077 ## File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java ## @@ -501,6 +504,12 @@ @Parameter( property = "excludedGroups" ) private String excludedGroups; +@Parameter( property = "junitIncludeEngine" ) Review comment: I expected to finish it today but ok I will wait for you till Tuesday. -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344: URL: https://github.com/apache/maven-surefire/pull/344#discussion_r606320356 ## File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java ## @@ -501,6 +504,12 @@ @Parameter( property = "excludedGroups" ) private String excludedGroups; +@Parameter( property = "junitIncludeEngine" ) Review comment: yes, the trick is called an Abstcation. Let's jump to the class `SurefirePlugin`, and consider the field [printSummary](https://github.com/apache/maven-surefire/blob/master/maven-surefire-plugin/src/main/java/org/apache/maven/plugin/surefire/SurefirePlugin.java#L100) and its [setter](https://github.com/apache/maven-surefire/blob/master/maven-surefire-plugin/src/main/java/org/apache/maven/plugin/surefire/SurefirePlugin.java#L702) and [getter](https://github.com/apache/maven-surefire/blob/master/maven-surefire-plugin/src/main/java/org/apache/maven/plugin/surefire/SurefirePlugin.java#L696). Now jump to the class `IntegrationTestMojo`. The field with name [printSummary](https://github.com/apache/maven-surefire/blob/master/maven-failsafe-plugin/src/main/java/org/apache/maven/plugin/failsafe/IntegrationTestMojo.java#L117), [setter](https://github.com/apache/maven-surefire/blob/master/maven-failsafe-plugin/src/main/java/org/apache/maven/plugin/failsafe/IntegrationTestMojo.java#L692) and [getter](https://github.com/apache/maven-surefire/blob/master/maven-failsafe-plugin/src/main/java/org/apache/maven/plugin/failsafe/IntegrationTestMojo.java#L686). These were concrete implementations. But notice that both classes implement the same interface through extension of the abstract class `AbstractSurefireMojo`, see [this](https://github.com/apache/maven-surefire/blob/master/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java#L162). So the [interface](https://github.com/apache/maven-surefire/blob/master/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireExecutionParameters.java) declares abstract methods I spoke before, the [setter](https://github.com/apache/maven-surefire/blob/master/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireExecutionParameters.java#L83) and [getter](https://github.com/apache/maven-surefire/blob/master/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/SurefireExecutionParameters.java#L81). The task for you is to rename your fields to `includeJUnit5Engines` and `excludeJUnit5Engines`. Then change the type to `String[]`. The IntelliJ IDEA will automatically update the getter and setter. Write both abstract getter and setter in the interface `SurefireExecutionParameters`. Implement them in `SurefirePlugin` and `IntegrationTestMojo`. Move both fields [this](https://github.com/apache/maven-surefire/pull/344/commits/e8ef4f5153d5c3f82639e4ec8cd1b1364202dad8#diff-38e379eb63d7dcd2deb45902b4a494517e4ce54c29913bcf74e7d9243ebb7011R507) and [this](https://github.com/apache/maven-surefire/pull/344/commits/e8ef4f5153d5c3f82639e4ec8cd1b1364202dad8#diff-38e379eb63d7dcd2deb45902b4a494517e4ce54c29913bcf74e7d9243ebb7011R511) to `SurefirePlugin` and `IntegrationTestMojo`. Then remove the getters and setters in `AbstractSurefireMojo from Line 3815 to 3836. Try to compile the 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344: URL: https://github.com/apache/maven-surefire/pull/344#discussion_r606227151 ## File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java ## @@ -501,6 +504,12 @@ @Parameter( property = "excludedGroups" ) private String excludedGroups; +@Parameter( property = "junitIncludeEngine" ) Review comment: I know it is quite a lot of work with the PR but it is always with a new feature. Pls add a Javadoc on the top of these fields and see how we use to write the Javadoc in another parts. Additionally every feature has to be documented. Regarding JUnit5 add a new chanper with your feature [here](https://github.com/apache/maven-surefire/blob/master/maven-surefire-plugin/src/site/apt/examples/junit-platform.apt.vm). -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344: URL: https://github.com/apache/maven-surefire/pull/344#discussion_r606194405 ## File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java ## @@ -501,6 +504,12 @@ @Parameter( property = "excludedGroups" ) private String excludedGroups; +@Parameter( property = "junitIncludeEngine" ) Review comment: Both instance fileds are in wrong class. We have two plugins in this project, so you have to generate getter and setter for these fields. Both must be in `SurefirPlugin` class, see [this](https://github.com/apache/maven-surefire/blob/master/maven-surefire-plugin/src/main/java/org/apache/maven/plugin/surefire/SurefirePlugin.java#L99) and also in [here](https://github.com/apache/maven-surefire/blob/master/maven-failsafe-plugin/src/main/java/org/apache/maven/plugin/failsafe/IntegrationTestMojo.java#L116). The reson is the prefix in the property. Due to AbstractSurefireMojo is an abstract class, you have to generate getters and setters and use them instead of fields in AbstractSurefireMojo. Regarding the naming conventions pls start the name of fields and properties with "include" and "exclude", and rename junit to "junit5" to make it clear that we are not aiming for JUnit4 and 3. Additionally both should be String[] because the history shows us that people want to have more than one value in our config properties, pls see [the tutorial for MOJO](https://maven.apache.org/guides/plugin/guide-java-plugin-development.html#parameter-types-with-multiple-values). -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344: URL: https://github.com/apache/maven-surefire/pull/344#discussion_r606194405 ## File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java ## @@ -501,6 +504,12 @@ @Parameter( property = "excludedGroups" ) private String excludedGroups; +@Parameter( property = "junitIncludeEngine" ) Review comment: Both instance fileds are in wrong class. We have two plugins in this project, so you have to generate getter and setter for these fields. Both must be in `SurefirPlugin` class, see [this](https://github.com/apache/maven-surefire/blob/master/maven-surefire-plugin/src/main/java/org/apache/maven/plugin/surefire/SurefirePlugin.java#L99) and also in [here](https://github.com/apache/maven-surefire/blob/master/maven-failsafe-plugin/src/main/java/org/apache/maven/plugin/failsafe/IntegrationTestMojo.java#L116). The reson is the prefix in the property. Due to AbstractSurefireMojo is an abstract class, you have to generate getters and setters and use them instead of fields in AbstractSurefireMojo. Regarding the naming conventions pls start the name of fields and properties with "include" and "exclude", and rename junit to "junit5" to make it clear that we are aiming for not JUnit4 and 3. Additionally both should be String[] because the history shows us that people want to have more than one value in our config properties, pls see [the tutorial for MOJO](https://maven.apache.org/guides/plugin/guide-java-plugin-development.html#parameter-types-with-multiple-values). -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344: URL: https://github.com/apache/maven-surefire/pull/344#discussion_r606194405 ## File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java ## @@ -501,6 +504,12 @@ @Parameter( property = "excludedGroups" ) private String excludedGroups; +@Parameter( property = "junitIncludeEngine" ) Review comment: Both instance fileds are in wrong class. We have two plugins in this project, so you have to generate getter and setter for these fields. Both must be in `SurefirPlugin` class, see [this](https://github.com/apache/maven-surefire/blob/master/maven-surefire-plugin/src/main/java/org/apache/maven/plugin/surefire/SurefirePlugin.java#L99) and also in [here](https://github.com/apache/maven-surefire/blob/master/maven-failsafe-plugin/src/main/java/org/apache/maven/plugin/failsafe/IntegrationTestMojo.java#L116). The reson is the prefix in the property. Due to AbstractSurefireMojo is an abstract class, you have to generate getters and setters and use them instead of fields in AbstractSurefireMojo. Regarding the naming conventions pls start the name of fields and properties with "include" and "exclude", and raname junit to "junit5" to make it clear that we are aiming for not JUnit4 and 3. Additionally both should be String[] because the history shows us that people want to have more than one value in our config properties, pls see [the tutorial for MOJO](https://maven.apache.org/guides/plugin/guide-java-plugin-development.html#parameter-types-with-multiple-values). -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344: URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605924419 ## File path: surefire-providers/surefire-junit-platform/src/test/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProviderTest.java ## @@ -788,7 +792,10 @@ public void bothJunitIncludeAndExcludeEngineAreAllowed() JUnitPlatformProvider provider = new JUnitPlatformProvider( providerParameters ); assertThat( provider.getFilters() ).hasSize( 2 ); -assertThat( provider.getFilters() ).allMatch( x -> x instanceof EngineFilter ); +assertThat( Arrays.stream( provider.getFilters() ).collect( Collectors.toList()) ) Review comment: thx, I 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344: URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605924120 ## File path: surefire-providers/surefire-junit-platform/src/test/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProviderTest.java ## @@ -772,7 +775,8 @@ public void filtersWithEmptyJunitEngineAreNotRegistered() JUnitPlatformProvider provider = new JUnitPlatformProvider( providerParameters ); assertThat( provider.getFilters() ).hasSize( 1 ); -assertThat( provider.getFilters() ).allMatch( x -> x instanceof EngineFilter ); +assertThat( Arrays.stream( provider.getFilters() ).collect( Collectors.toList()) ) Review comment: ok, then better do it without the Java Stream like this `assertThat( provider.getFilters()[0] ).isInstanceOf( EngineFilter.class )` -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344: URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605883578 ## File path: surefire-providers/surefire-junit-platform/src/test/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProviderTest.java ## @@ -788,7 +792,10 @@ public void bothJunitIncludeAndExcludeEngineAreAllowed() JUnitPlatformProvider provider = new JUnitPlatformProvider( providerParameters ); assertThat( provider.getFilters() ).hasSize( 2 ); -assertThat( provider.getFilters() ).allMatch( x -> x instanceof EngineFilter ); +assertThat( Arrays.stream( provider.getFilters() ).collect( Collectors.toList()) ) Review comment: `assertThat( provider.getFilters() ).asList().element( 0 ).isInstanceOf( EngineFilter.class )` `assertThat( provider.getFilters() ).asList().element( 1 ).isInstanceOf( EngineFilter.class )` -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344: URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605882860 ## File path: surefire-providers/surefire-junit-platform/src/test/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProviderTest.java ## @@ -772,7 +775,8 @@ public void filtersWithEmptyJunitEngineAreNotRegistered() JUnitPlatformProvider provider = new JUnitPlatformProvider( providerParameters ); assertThat( provider.getFilters() ).hasSize( 1 ); -assertThat( provider.getFilters() ).allMatch( x -> x instanceof EngineFilter ); +assertThat( Arrays.stream( provider.getFilters() ).collect( Collectors.toList()) ) Review comment: This is unnecessarily complicated. I have checked it out in my code, and this works for me: `assertThat( provider.getFilters() ).asList().element( 0 ).isInstanceOf( EngineFilter.class )` Does it work for you too? -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344: URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605882185 ## File path: surefire-providers/surefire-junit-platform/src/test/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProviderTest.java ## @@ -743,7 +745,8 @@ public void onlyJunitExcludeEngineIsDeclared() JUnitPlatformProvider provider = new JUnitPlatformProvider( providerParameters ); assertThat( provider.getFilters() ).hasSize( 1 ); -assertThat( provider.getFilters() ).allMatch( x -> x instanceof EngineFilter ); +assertThat( Arrays.stream( provider.getFilters() ).collect( Collectors.toList()) ) Review comment: This is unnecessarily complicated. I have checked it out in my code, and this works for me: `assertThat( provider.getFilters() ).asList().element( 0 ).isInstanceOf( EngineFilter.class )` Does it work for you too? -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344: URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605881464 ## File path: surefire-providers/surefire-junit-platform/src/test/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProviderTest.java ## @@ -729,7 +730,8 @@ public void onlyJunitIncludeEngineIsDeclared() JUnitPlatformProvider provider = new JUnitPlatformProvider( providerParameters ); assertThat( provider.getFilters() ).hasSize( 1 ); -assertThat( provider.getFilters() ).allMatch( x -> x instanceof EngineFilter ); +assertThat( Arrays.stream( provider.getFilters() ).collect( Collectors.toList()) ) Review comment: This is unnecessarily complicated. I have checked it out in my code, and this works for me: `assertThat( provider.getFilters() ).asList().element( 0 ).isInstanceOf( EngineFilter.class )` Does it work for you too? -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344: URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605881464 ## File path: surefire-providers/surefire-junit-platform/src/test/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProviderTest.java ## @@ -729,7 +730,8 @@ public void onlyJunitIncludeEngineIsDeclared() JUnitPlatformProvider provider = new JUnitPlatformProvider( providerParameters ); assertThat( provider.getFilters() ).hasSize( 1 ); -assertThat( provider.getFilters() ).allMatch( x -> x instanceof EngineFilter ); +assertThat( Arrays.stream( provider.getFilters() ).collect( Collectors.toList()) ) Review comment: This is unnecessarily complicated. I have checked it out in my code, and this works for me: `assertThat( provider.getFilters() ).asList().element( 0 ).isInstanceOf( TestEngine.class )` Does it work for you too? -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605548725
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -1568,13 +1572,13 @@ private void convertGroupParameters()
private void convertJunitEngineParameters()
{
-if ( this.getJunitIncludeEngine() != null )
+if ( StringUtils.isNotBlank( getJunitIncludeEngine()) )
Review comment:
After you removed the Line 65, you can simply type:
`if ( isNotBlank( getJunitIncludeEngine() ) )`
notice that I added a white space between the end-brackets to confirm the
checkstyle rule.
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -1568,13 +1572,13 @@ private void convertGroupParameters()
private void convertJunitEngineParameters()
{
-if ( this.getJunitIncludeEngine() != null )
+if ( StringUtils.isNotBlank( getJunitIncludeEngine()) )
{
-getProperties().setProperty(
ProviderParameterNames.JUNIT_INCLUDE_ENGINE_PROP, this.getJunitIncludeEngine()
);
+getProperties().setProperty( JUNIT_INCLUDE_ENGINE_PROP,
getJunitIncludeEngine() );
}
-if ( this.getJunitExcludeEngine() != null )
+if ( StringUtils.isNotBlank( getJunitExcludeEngine()) )
Review comment:
same here
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344: URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605547899 ## File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java ## @@ -59,6 +62,7 @@ import org.apache.maven.project.MavenProject; import org.apache.maven.shared.artifact.filter.PatternIncludesArtifactFilter; import org.apache.maven.shared.transfer.dependencies.resolve.DependencyResolver; +import org.apache.maven.surefire.shared.utils.StringUtils; Review comment: Pls remove this line, because there already was this import line: `import static org.apache.maven.surefire.shared.utils.StringUtils.isNotBlank;` -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605533224
##
File path:
surefire-providers/surefire-junit-platform/src/test/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProviderTest.java
##
@@ -766,6 +768,72 @@ public void
tagExpressionsAreSupportedForIncludeTagsContainingAmpersand()
assertEquals( 2, provider.getFilters().length );
}
+@Test
+public void onlyJunitIncludeEngineIsDeclared()
+{
+Map properties = singletonMap(
JUNIT_INCLUDE_ENGINE_PROP, "engine-one, engine-two" );
+
+ProviderParameters providerParameters = providerParametersMock(
TestClass1.class );
+when( providerParameters.getProviderProperties() ).thenReturn(
properties );
+
+JUnitPlatformProvider provider = new JUnitPlatformProvider(
providerParameters );
+
+assertEquals( 1, provider.getFilters().length );
Review comment:
`instanceOf` would be fine too.
Dont use assertEquals, you have more modern style, see `import static
org.assertj.core.api.Assertions.assertThat`
Example:
`assertThat( provider.getFilters() ).hasSize( 1 )`
`assertThat( provider.getFilters() ).element( 0 ).isInstanceOf(
TestEngine.class )`
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605492071
##
File path:
surefire-providers/surefire-junit-platform/src/test/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProviderTest.java
##
@@ -766,6 +768,72 @@ public void
tagExpressionsAreSupportedForIncludeTagsContainingAmpersand()
assertEquals( 2, provider.getFilters().length );
}
+@Test
+public void onlyJunitIncludeEngineIsDeclared()
+{
+Map properties = singletonMap(
JUNIT_INCLUDE_ENGINE_PROP, "engine-one, engine-two" );
+
+ProviderParameters providerParameters = providerParametersMock(
TestClass1.class );
+when( providerParameters.getProviderProperties() ).thenReturn(
properties );
+
+JUnitPlatformProvider provider = new JUnitPlatformProvider(
providerParameters );
+
+assertEquals( 1, provider.getFilters().length );
Review comment:
Can you examine the content of the filter with the assertion statement?
Assert that the filter contains two inclusive filters "engine-one,
engine-two".
Is it possible, and also in the next tests?
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605492071
##
File path:
surefire-providers/surefire-junit-platform/src/test/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProviderTest.java
##
@@ -766,6 +768,72 @@ public void
tagExpressionsAreSupportedForIncludeTagsContainingAmpersand()
assertEquals( 2, provider.getFilters().length );
}
+@Test
+public void onlyJunitIncludeEngineIsDeclared()
+{
+Map properties = singletonMap(
JUNIT_INCLUDE_ENGINE_PROP, "engine-one, engine-two" );
+
+ProviderParameters providerParameters = providerParametersMock(
TestClass1.class );
+when( providerParameters.getProviderProperties() ).thenReturn(
properties );
+
+JUnitPlatformProvider provider = new JUnitPlatformProvider(
providerParameters );
+
+assertEquals( 1, provider.getFilters().length );
Review comment:
Can you examine the content of the filter with the assertion statement?
Assert that the filter contains two inclusive filters "engine-one,
engine-two".
Is it possibel, and also in the next tests?
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605488089
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -1560,6 +1566,18 @@ private void convertGroupParameters()
}
}
+private void convertJunitEngineParameters()
+{
+if ( this.getJunitIncludeEngine() != null )
Review comment:
I think this class is using `isNotBlank` somewhere too. It is in one of
the classes `StringUtils`. It is very useful because it has the same meaning
for `null` and empty strings or strings having white spaces, since they do not
cover any reasonable value, and the method returns `false` in these cases.
Same in the second IF below.
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605488089
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -1560,6 +1566,18 @@ private void convertGroupParameters()
}
}
+private void convertJunitEngineParameters()
+{
+if ( this.getJunitIncludeEngine() != null )
Review comment:
I think this class is using `isNotBlank` somewhere too. It is in one of
the classes `StringUtils`/ It is very useful because it has the same meaning
for `not` and empty strings or strings having white spaces, since they do not
cover any reasonable value, and the method returns `false` in these cases.
Same in the second IF below.
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605484137
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -1560,6 +1566,18 @@ private void convertGroupParameters()
}
}
+private void convertJunitEngineParameters()
+{
+if ( this.getJunitIncludeEngine() != null )
+{
+getProperties().setProperty(
ProviderParameterNames.JUNIT_INCLUDE_ENGINE_PROP, this.getJunitIncludeEngine()
);
+}
+if ( this.getJunitExcludeEngine() != null )
+{
+getProperties().setProperty(
ProviderParameterNames.JUNIT_EXCLUDE_ENGINE_PROP, this.getJunitExcludeEngine()
);
Review comment:
Here can be the static import for the constant JUNIT_EXCLUDE_ENGINE_PROP
and the line would become shorter.
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605484013
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -1560,6 +1566,18 @@ private void convertGroupParameters()
}
}
+private void convertJunitEngineParameters()
+{
+if ( this.getJunitIncludeEngine() != null )
+{
+getProperties().setProperty(
ProviderParameterNames.JUNIT_INCLUDE_ENGINE_PROP, this.getJunitIncludeEngine()
);
Review comment:
Here can be the static import for JUNIT_INCLUDE_ENGINE_PROP and the line
would become shorter.
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605484013
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -1560,6 +1566,18 @@ private void convertGroupParameters()
}
}
+private void convertJunitEngineParameters()
+{
+if ( this.getJunitIncludeEngine() != null )
+{
+getProperties().setProperty(
ProviderParameterNames.JUNIT_INCLUDE_ENGINE_PROP, this.getJunitIncludeEngine()
);
Review comment:
Here can be the static import for JUNIT_EXCLUDE_ENGINE_PROP and the line
would become shorter.
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605484013
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -1560,6 +1566,18 @@ private void convertGroupParameters()
}
}
+private void convertJunitEngineParameters()
+{
+if ( this.getJunitIncludeEngine() != null )
+{
+getProperties().setProperty(
ProviderParameterNames.JUNIT_INCLUDE_ENGINE_PROP, this.getJunitIncludeEngine()
);
Review comment:
Here can be the static import and the line would become shorter.
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -1560,6 +1566,18 @@ private void convertGroupParameters()
}
}
+private void convertJunitEngineParameters()
+{
+if ( this.getJunitIncludeEngine() != null )
+{
+getProperties().setProperty(
ProviderParameterNames.JUNIT_INCLUDE_ENGINE_PROP, this.getJunitIncludeEngine()
);
+}
+if ( this.getJunitExcludeEngine() != null )
+{
+getProperties().setProperty(
ProviderParameterNames.JUNIT_EXCLUDE_ENGINE_PROP, this.getJunitExcludeEngine()
);
Review comment:
Here can be the static import and the line would become shorter.
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605482069
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -1560,6 +1566,18 @@ private void convertGroupParameters()
}
}
+private void convertJunitEngineParameters()
+{
+if ( this.getJunitIncludeEngine() != null )
+{
+getProperties().setProperty(
ProviderParameterNames.JUNIT_INCLUDE_ENGINE_PROP, this.getJunitIncludeEngine()
);
Review comment:
Remove "this".
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -1560,6 +1566,18 @@ private void convertGroupParameters()
}
}
+private void convertJunitEngineParameters()
+{
+if ( this.getJunitIncludeEngine() != null )
+{
+getProperties().setProperty(
ProviderParameterNames.JUNIT_INCLUDE_ENGINE_PROP, this.getJunitIncludeEngine()
);
+}
+if ( this.getJunitExcludeEngine() != null )
+{
+getProperties().setProperty(
ProviderParameterNames.JUNIT_EXCLUDE_ENGINE_PROP, this.getJunitExcludeEngine()
);
Review comment:
Remove "this".
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605481968
##
File path:
maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##
@@ -1560,6 +1566,18 @@ private void convertGroupParameters()
}
}
+private void convertJunitEngineParameters()
+{
+if ( this.getJunitIncludeEngine() != null )
Review comment:
Pls use `getJunitIncludeEngine()` instead of
`this.getJunitIncludeEngine()`.
Remove "this".
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344: URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605305509 ## File path: surefire-providers/surefire-junit-platform/src/test/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProviderTest.java ## @@ -22,8 +22,8 @@ import static java.util.Collections.emptyMap; import static java.util.Collections.singletonMap; import static java.util.stream.Collectors.toSet; -import static org.apache.maven.surefire.api.booter.ProviderParameterNames.TESTNG_EXCLUDEDGROUPS_PROP; -import static org.apache.maven.surefire.api.booter.ProviderParameterNames.TESTNG_GROUPS_PROP; +import static org.apache.maven.surefire.api.booter.ProviderParameterNames.*; Review comment: again star here -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine
Tibor17 commented on a change in pull request #344: URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605305237 ## File path: surefire-providers/surefire-junit-platform/src/main/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProvider.java ## @@ -60,10 +60,7 @@ import org.apache.maven.surefire.shared.utils.StringUtils; import org.junit.platform.engine.DiscoverySelector; import org.junit.platform.engine.Filter; -import org.junit.platform.launcher.Launcher; -import org.junit.platform.launcher.LauncherDiscoveryRequest; -import org.junit.platform.launcher.TagFilter; -import org.junit.platform.launcher.TestIdentifier; +import org.junit.platform.launcher.*; Review comment: star is not legal -- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
