You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2021/03/31 13:21:48 UTC

[GitHub] [maven-surefire] adam11grafik opened a new pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

adam11grafik opened a new pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344


   Support include/exclude junit test engine


-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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<String, String> 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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-812472479


   The build is succesful now. Pls squash all commits in one, I would like to see it as one commit and make the last review. 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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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<String, String> 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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r608059224



##########
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:
       Hmm I did not found example with marking some keyword as reserved system property description
   Can you provide some example?




-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik closed pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik closed pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344


   


-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605494931



##########
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:
       I used the same format like it was for TESTNG_GROUPS_PROP but if you want I can follow your approach?




-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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<String, String> 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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-814378922


   ok, this looks very well.
   Pls squash it and I will finish it.
   Thx for your work!


-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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 <<<engines>>>, use <<<excludeJUnit5Engines>>>.
 
+   Be aware that this feature reserve system properties <<<includejunit5engines>>> and <<<excludejunit5engines>>>

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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik commented on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik commented on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-811936118


   @Tibor17, I got errors here now:
    Error:  src\test\java\org\apache\maven\surefire\junitplatform\JUnitPlatformProviderTest.java:[892] (sizes) LineLength: Line is longer than 120 characters (found 142).
   Error:  src\test\java\org\apache\maven\surefire\junitplatform\JUnitPlatformProviderTest.java:[925] (sizes) LineLength: Line is longer than 120 characters (found 122).
   Error:  src\test\java\org\apache\maven\surefire\junitplatform\JUnitPlatformProviderTest.java:[930] (sizes) LineLength: Line is longer than 120 characters (found 147).
   Error:  src\test\java\org\apache\maven\surefire\junitplatform\JUnitPlatformProviderTest.java:[1010] (sizes) LineLength: Line is longer than 120 characters (found 122).
   But I did not change those lines of code :/ It was there previously.
   Should I refactor this to pass build?


-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r606343723



##########
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:
       Ok, thank you. I will try to do this on 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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik commented on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik commented on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-815836192


   Hi, but will it be part of 3.0.0-M6 version or 2.x ?
   I see that PR is closed, not merged...


-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik edited a comment on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik edited a comment on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-811936118


   @Tibor17, I got errors here now:
    Error:  src\test\java\org\apache\maven\surefire\junitplatform\JUnitPlatformProviderTest.java:[892] (sizes) LineLength: Line is longer than 120 characters (found 142).
   Error:  src\test\java\org\apache\maven\surefire\junitplatform\JUnitPlatformProviderTest.java:[925] (sizes) LineLength: Line is longer than 120 characters (found 122).
   Error:  src\test\java\org\apache\maven\surefire\junitplatform\JUnitPlatformProviderTest.java:[930] (sizes) LineLength: Line is longer than 120 characters (found 147).
   Error:  src\test\java\org\apache\maven\surefire\junitplatform\JUnitPlatformProviderTest.java:[1010] (sizes) LineLength: Line is longer than 120 characters (found 122).
   But I did not change those lines of code :/ It was there previously.
   Should I refactor this to pass build?
   
   Ok, my bad. I modified this again :)
   Will fix this and revert as it was.


-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-815849901


   @adam11grafik 
   See the master branch. Yes it will be in M6. 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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605894867



##########
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:
       For me assertThat( provider.getFilters() ).asList() throw exception as getFilters() return array but expected was list :/




-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik commented on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik commented on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-811729840


   Ahh sorry, maybe I click wrong button :)


-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik commented on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik commented on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-812078484


   I check and there is no method: .element()
   assertThat( provider.getFilters() ).element( 0 )
   getFilters() return array so maybe this is the reason


-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-815826275


   I added few unit tests and ushed.
   Thx for contributing!


-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-814238087


   Additionally write an example with POM in the documentation file `junit-platform.apt.vm`. Our users are reading it and they want to see a new features there. 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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605492296



##########
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:
       Ok, it was done previously by IDE formatting but I will change 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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik commented on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik commented on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-811720519


   > 
   > 
   > yes, you have modified the biggest class AbstractSurefireMojo. The problem is that I am not able to recognize real changes within the code format you changed. I guess you pressed CRTL+L in your IDE and reformatted the code. Pls do not do that. That;s the reason why the build is not successful. You should place only chages without reformating all the code around.
   
   


-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-811729884


   Do you use IntelliJ IDEA?
   You can configure it so that the IDE will be according to the checkstyle plugin, see this:
   https://maven.apache.org/developers/conventions/code.html
   ```
   Download maven-idea-codestyle.xml and import it into IDEA using File > Settings > Editor > Code Style > Gear icon > Import Scheme > IntelliJ IDEA Code Style XML
   ```


-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik commented on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik commented on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-811354712


   Hi @Tibor17, can you help me with build issue?
   maven-checkstyle-plugin throw a lot of error s but any formatter was not triggered for my branch  when I was building project locally.
   So how can I keep all those standards?
   BR,
   Adam


-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik commented on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik commented on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-814452411


   Squash done.
   Thank you also for your support!
   It was new and interesting experience for me! :)


-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik commented on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik commented on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-811722247


   @Tibor17, hmm why maven repository does not use maven formatter plugin? :)
   Should I fix this on the same branch and create new PR?
   I do not know why PR was closed :/
   Or should I create new branch?
   BR,
   Adam


-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r607843333



##########
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:
       Hi, I made most of changes but there is conversion of String[] to String which does not look so good but I need this to pass values to properties.
   Regarding that include and exclude engines should be provided in array I agree but please, remember that most of usage of this feature will base probably on use of one value only as junit as default have only one engine (one for JUnit5 and one for JUnit Vintage).
   I will push my changes in next minutes




-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-811555475


   yes, you have modified the biggest class AbstractSurefireMojo. The problem is that I am not able to recognize real changes within the code format    you changed. I guess you pressed CRTL+L in your IDE and reformatted the code. Pls do not do that. That;s the reason why the build is not successful. You should place only chages without reformating all the code around.


-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-812086654


   This would work
   `assertThat( provider.getFilters() ).asList().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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik closed pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik closed pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344


   


-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r608088711



##########
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:
       I agree with you :) but still I am learning ways of working in your community and not all is know for me :)




-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605676523



##########
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<String, String> 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:
       Hmm there was no method: ".element()" so I used something else :)




-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 closed pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
Tibor17 closed pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344


   


-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r606255188



##########
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:
       Hmm I follow simillar approach like for "groups" but now I am a little confused regarding those fields... 
   Logic to load values for those fields is in AbstractSurefireMojo but if I will not define them here, so how I can get values for them?
   Can you provide me some example field with simillar flow which you expect?
   




-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-811555475


   yes, you have modified the biggest class AbstractSurefireMojo. The problem is that I am not able to recognize real changes and the code format. I guess you pressed CRTL+L in your IDE and reformatted the code. Pls do not do that. That;s the reason why the build is not successful. You should place only chages without reformating all the code around.


-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-811727484


   I can see that you closed your PR but I have reopened it.
   Please continue 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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605894867



##########
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:
       For me assertThat( provider.getFilters() ).asList() throw exception as getFilters() return array but expected was list :/
   java.lang.AssertionError: 
   Expecting:
     <[EngineFilter that includes engines with IDs [engine-one, engine-two]]>
   to be an instance of:
     <java.util.List>
   but was instance of:
     <org.junit.platform.engine.Filter[]>




-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605520269



##########
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<String, String> 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:
       Hmm I checked and to verify ids I need to do probably some conversion of junit classes etc.
   There is no direct method to get those ids values but only method to apply if TestEngine is included or excluded by filters.
   This logic is more inside Junit itself so maybe enough will be to check only if expected filter is instance of EngineFilter?




-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r605894387



##########
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() throw exception as getFilters() return array but expected was list :/




-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r608193226



##########
File path: maven-surefire-plugin/src/site/apt/examples/junit-platform.apt.vm
##########
@@ -526,6 +526,9 @@ else
 
     * To exclude <<<engines>>>, use <<<excludeJUnit5Engines>>>.
 
+   Be aware that this feature reserve system properties <<<includejunit5engines>>> and <<<excludejunit5engines>>>

Review comment:
       Done




-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-811727484


   I can see that zou closed your PR but I have reopened it.
   Please continue 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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] adam11grafik commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
adam11grafik commented on a change in pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#discussion_r608098238



##########
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:
       Ok, now I know which properties do you have in mind :)
   Not @Parameters but system properties used internally to provide values to JUnitPlatformProvider.
   Is it ok now?




-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-812087636


   If the assertion statement fails, we would see a complete array in the log, the index of element and types which do not match. So it will be very verbose report. That's the most important difference against assertEquals or assertTrue.


-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-812034002


   Now this looks much better. Pls fix the line length.
   Are you going to use the `isInstanceOf` in several 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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #344:
URL: https://github.com/apache/maven-surefire/pull/344#issuecomment-811738125


   My advice for you is to copy the class and revert it. Then apply only code changes.


-- 
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:
users@infra.apache.org

[GitHub] [maven-surefire] Tibor17 commented on a change in pull request #344: SUREFIRE-1854 Support include/exclude junit test engine

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org