[GitHub] maven-surefire pull request #173: [SUREFIRE-1004] Support full gavtc format ...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire pull request #173: [SUREFIRE-1004] Support full gavtc format ...

bradflood
GitHub user bindul opened a pull request:

    https://github.com/apache/maven-surefire/pull/173

    [SUREFIRE-1004] Support full gavtc format for dependenciesToScan

    Added support for specifying dependencies to scan for tests using full `groupId:artifactId[:version[:type[:classifier]]]` format. Has unit and integration tests and updates to documentation.
   
    Related Issues:
    - [SUREFIRE-1004](https://issues.apache.org/jira/browse/SUREFIRE-1004)

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/bindul/maven-surefire SUREFIRE-1004_dependenciesToScan

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/maven-surefire/pull/173.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #173
   
----
commit 447537ccbe54ae64868a507624f4af7a2edf0d3a
Author: Bindul Bhowmik <bindulbhowmik@...>
Date:   2018-01-11T17:56:38Z

    [SUREFIRE-1004] Support full gavtc format for dependenciesToScan
   
    Submitted by: Bindul Bhowmik
   
    Code, unit tests and integration test to support full
    groupId:artifactId[:version[:type[:classifier]]] format when specifying
    additional dependencies to scan for test classes.
   
    Updated plugin parameter and site documentation.
   
    Related Issues:
    - [SUREFIRE-1004] Enhance pattern/wildcard capabilities for
    dependenciesToScan to GAVT

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire issue #173: [SUREFIRE-1004] Support full gavtc format for dep...

bradflood
Github user Tibor17 commented on the issue:

    https://github.com/apache/maven-surefire/pull/173
 
    The comments like this `// Check version` are not needed because it's obvious from code.
    You may remove them or extract that part to private static methods. It's up to you.
    If you change something please `git --ammend commit`. There should be single commit per PR.
    Good work!


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire issue #173: [SUREFIRE-1004] Support full gavtc format for dep...

bradflood
In reply to this post by bradflood
Github user bindul commented on the issue:

    https://github.com/apache/maven-surefire/pull/173
 
    @Tibor17 Thank you for your review.
   
    I agree with you that `version` check is not really useful in this use case, I put it in there as the JIRA ticket called for GAVT support and also to keep the format consistent with the other places maven defines gavtc.
   
    I am happy to remove the version check. If I do so, do you want the input configuration format to still be `groupId:artifactId[:version[:type[:classifier]]]` or would you rather it be `groupId:artifactId[:type[:classifier]]`.
   
    I will amend the commit based on your response.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire issue #173: [SUREFIRE-1004] Support full gavtc format for dep...

bradflood
In reply to this post by bradflood
Github user Tibor17 commented on the issue:

    https://github.com/apache/maven-surefire/pull/173
 
    I don't want you to remove code. I will try to explain in code.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire pull request #173: [SUREFIRE-1004] Support full gavtc format ...

bradflood
In reply to this post by bradflood
Github user Tibor17 commented on a diff in the pull request:

    https://github.com/apache/maven-surefire/pull/173#discussion_r161331717
 
    --- Diff: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/util/DependencyScanner.java ---
    @@ -111,19 +112,46 @@ private static void scanArtifact( File artifact, TestFilter<String, String> filt
             {
                 for ( String groups : groupArtifactIds )
                 {
    +                // groupId:artifactId[:version[:type[:classifier]]]
    --- End diff --
   
    Maybe you could add `groupId:artifactId[:version[:type[:classifier]]]` to a regular JavaDoc on the top of Class or method and then this comment can be removed. The users will see it in public API JavaDoc for their better understanding.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire pull request #173: [SUREFIRE-1004] Support full gavtc format ...

bradflood
In reply to this post by bradflood
Github user Tibor17 commented on a diff in the pull request:

    https://github.com/apache/maven-surefire/pull/173#discussion_r161332171
 
    --- Diff: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/util/DependencyScanner.java ---
    @@ -111,19 +112,46 @@ private static void scanArtifact( File artifact, TestFilter<String, String> filt
             {
                 for ( String groups : groupArtifactIds )
                 {
    +                // groupId:artifactId[:version[:type[:classifier]]]
                     String[] groupArtifact = groups.split( ":" );
    -                if ( groupArtifact.length != 2 )
    +                if ( groupArtifact.length < 2 || groupArtifact.length > 5 )
                     {
                         throw new IllegalArgumentException( "dependencyToScan argument should be in format"
    -                        + " 'groupid:artifactid': " + groups );
    +                        + " 'groupid:artifactid[:version[:type[:classifier]]]': " + groups );
                     }
    -                if ( artifact.getGroupId().matches( groupArtifact[0] )
    -                    && artifact.getArtifactId().matches( groupArtifact[1] ) )
    +                if ( artifactMatchesGavtc( artifact, groupArtifact ) )
                     {
                         matches.add( artifact.getFile() );
                     }
                 }
             }
             return matches;
         }
    +    
    +    private static boolean artifactMatchesGavtc( Artifact artifact, String[] gavtc )
    --- End diff --
   
    In my next comment I use "groups" as a parameter because the upper part has local variable "groups". You can rename it as you like.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire pull request #173: [SUREFIRE-1004] Support full gavtc format ...

bradflood
In reply to this post by bradflood
Github user Tibor17 commented on a diff in the pull request:

    https://github.com/apache/maven-surefire/pull/173#discussion_r161333402
 
    --- Diff: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/util/DependencyScanner.java ---
    @@ -111,19 +112,46 @@ private static void scanArtifact( File artifact, TestFilter<String, String> filt
             {
                 for ( String groups : groupArtifactIds )
                 {
    +                // groupId:artifactId[:version[:type[:classifier]]]
                     String[] groupArtifact = groups.split( ":" );
    -                if ( groupArtifact.length != 2 )
    +                if ( groupArtifact.length < 2 || groupArtifact.length > 5 )
                     {
                         throw new IllegalArgumentException( "dependencyToScan argument should be in format"
    -                        + " 'groupid:artifactid': " + groups );
    +                        + " 'groupid:artifactid[:version[:type[:classifier]]]': " + groups );
                     }
    -                if ( artifact.getGroupId().matches( groupArtifact[0] )
    -                    && artifact.getArtifactId().matches( groupArtifact[1] ) )
    +                if ( artifactMatchesGavtc( artifact, groupArtifact ) )
                     {
                         matches.add( artifact.getFile() );
                     }
                 }
             }
             return matches;
         }
    +    
    +    private static boolean artifactMatchesGavtc( Artifact artifact, String[] gavtc )
    +    {
    +        boolean match = false;
    +        if ( artifact.getGroupId().matches( gavtc[0] ) && artifact.getArtifactId().matches( gavtc[1] ) )
    +        {
    +            match = true;
    +            // Check version
    --- End diff --
   
    I mean only these comments. For me it is quite obvious what you mean in code and thus the comment is useless. If you want to put a meaning on the IF statement, you can do something like this:
    `if ( hasGroupAndArtifactId( groups ) )`
    ...
    ```private static boolean hasGroupAndArtifactId( String groups )
        return StringUtils.countMatches( groups, ':' ) >= 2;
    ```
    The project knows several `StringUtils` but I mean the one from library `commons-lang3`.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire pull request #173: [SUREFIRE-1004] Support full gavtc format ...

bradflood
In reply to this post by bradflood
Github user Tibor17 commented on a diff in the pull request:

    https://github.com/apache/maven-surefire/pull/173#discussion_r161333588
 
    --- Diff: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/util/DependencyScanner.java ---
    @@ -111,19 +112,46 @@ private static void scanArtifact( File artifact, TestFilter<String, String> filt
             {
                 for ( String groups : groupArtifactIds )
                 {
    +                // groupId:artifactId[:version[:type[:classifier]]]
                     String[] groupArtifact = groups.split( ":" );
    -                if ( groupArtifact.length != 2 )
    +                if ( groupArtifact.length < 2 || groupArtifact.length > 5 )
                     {
                         throw new IllegalArgumentException( "dependencyToScan argument should be in format"
    -                        + " 'groupid:artifactid': " + groups );
    +                        + " 'groupid:artifactid[:version[:type[:classifier]]]': " + groups );
                     }
    -                if ( artifact.getGroupId().matches( groupArtifact[0] )
    -                    && artifact.getArtifactId().matches( groupArtifact[1] ) )
    +                if ( artifactMatchesGavtc( artifact, groupArtifact ) )
                     {
                         matches.add( artifact.getFile() );
                     }
                 }
             }
             return matches;
         }
    +    
    +    private static boolean artifactMatchesGavtc( Artifact artifact, String[] gavtc )
    +    {
    +        boolean match = false;
    +        if ( artifact.getGroupId().matches( gavtc[0] ) && artifact.getArtifactId().matches( gavtc[1] ) )
    +        {
    +            match = true;
    +            // Check version
    +            if ( match && gavtc.length > 2 )
    --- End diff --
   
    Did you mean `>=`?


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire pull request #173: [SUREFIRE-1004] Support full gavtc format ...

bradflood
In reply to this post by bradflood
Github user Tibor17 commented on a diff in the pull request:

    https://github.com/apache/maven-surefire/pull/173#discussion_r161333957
 
    --- Diff: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/util/DependencyScanner.java ---
    @@ -111,19 +112,46 @@ private static void scanArtifact( File artifact, TestFilter<String, String> filt
             {
                 for ( String groups : groupArtifactIds )
                 {
    +                // groupId:artifactId[:version[:type[:classifier]]]
                     String[] groupArtifact = groups.split( ":" );
    -                if ( groupArtifact.length != 2 )
    +                if ( groupArtifact.length < 2 || groupArtifact.length > 5 )
                     {
                         throw new IllegalArgumentException( "dependencyToScan argument should be in format"
    -                        + " 'groupid:artifactid': " + groups );
    +                        + " 'groupid:artifactid[:version[:type[:classifier]]]': " + groups );
                     }
    -                if ( artifact.getGroupId().matches( groupArtifact[0] )
    -                    && artifact.getArtifactId().matches( groupArtifact[1] ) )
    +                if ( artifactMatchesGavtc( artifact, groupArtifact ) )
                     {
                         matches.add( artifact.getFile() );
                     }
                 }
             }
             return matches;
         }
    +    
    +    private static boolean artifactMatchesGavtc( Artifact artifact, String[] gavtc )
    +    {
    +        boolean match = false;
    +        if ( artifact.getGroupId().matches( gavtc[0] ) && artifact.getArtifactId().matches( gavtc[1] ) )
    +        {
    +            match = true;
    +            // Check version
    +            if ( match && gavtc.length > 2 )
    +            {
    --- End diff --
   
    Because I have proposed this method has only one parameter `( String groups )` you can use our utility:
    `String[] gavtc = StringUtils.split( groups, ':' );`


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire pull request #173: [SUREFIRE-1004] Support full gavtc format ...

bradflood
In reply to this post by bradflood
Github user Tibor17 commented on a diff in the pull request:

    https://github.com/apache/maven-surefire/pull/173#discussion_r161334621
 
    --- Diff: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/util/DependencyScanner.java ---
    @@ -111,19 +112,46 @@ private static void scanArtifact( File artifact, TestFilter<String, String> filt
             {
                 for ( String groups : groupArtifactIds )
                 {
    +                // groupId:artifactId[:version[:type[:classifier]]]
                     String[] groupArtifact = groups.split( ":" );
    -                if ( groupArtifact.length != 2 )
    +                if ( groupArtifact.length < 2 || groupArtifact.length > 5 )
                     {
                         throw new IllegalArgumentException( "dependencyToScan argument should be in format"
    -                        + " 'groupid:artifactid': " + groups );
    +                        + " 'groupid:artifactid[:version[:type[:classifier]]]': " + groups );
                     }
    -                if ( artifact.getGroupId().matches( groupArtifact[0] )
    -                    && artifact.getArtifactId().matches( groupArtifact[1] ) )
    +                if ( artifactMatchesGavtc( artifact, groupArtifact ) )
                     {
                         matches.add( artifact.getFile() );
                     }
                 }
             }
             return matches;
         }
    +    
    +    private static boolean artifactMatchesGavtc( Artifact artifact, String[] gavtc )
    +    {
    +        boolean match = false;
    +        if ( artifact.getGroupId().matches( gavtc[0] ) && artifact.getArtifactId().matches( gavtc[1] ) )
    +        {
    +            match = true;
    +            // Check version
    +            if ( match && gavtc.length > 2 )
    +            {
    +                match = StringUtils.isBlank( gavtc[2] ) || artifact.getVersion().equals( gavtc[2] );
    --- End diff --
   
    Then `match = StringUtils.isBlank( gavtc[2] )` might be verbose with calling new private static method `match = hasVersion( groups );`
    private static boolean hasVersion( String... gavtc )
    {
        return StringUtils.isBlank( gavtc[2] );
    }
    Basically the same principle with T and C and the code will be verbose.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire pull request #173: [SUREFIRE-1004] Support full gavtc format ...

bradflood
In reply to this post by bradflood
Github user Tibor17 commented on a diff in the pull request:

    https://github.com/apache/maven-surefire/pull/173#discussion_r161335367
 
    --- Diff: maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/util/DependenciesScannerTest.java ---
    @@ -19,12 +19,17 @@
      * under the License.
      */
     
    -import junit.framework.TestCase;
    +import static org.junit.Assert.*;
    --- End diff --
   
    Not sure if checkstyle will break the build because of using star `*` in this import.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire pull request #173: [SUREFIRE-1004] Support full gavtc format ...

bradflood
In reply to this post by bradflood
Github user Tibor17 commented on a diff in the pull request:

    https://github.com/apache/maven-surefire/pull/173#discussion_r161336057
 
    --- Diff: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java ---
    @@ -732,7 +732,8 @@
          * The child elements of this element must be &lt;dependency&gt; elements, and the
          * contents of each of these elements must be a string which follows the format:
          *
    -     * <i>groupId:artifactId</i>. For example: <i>org.acme:project-a</i>.
    +     * <i>groupId:artifactId[:version[:type[:classifier]]]</i>. For example: <i>org.acme:project-a</i>,
    +     * <i>org.acme:project-a::test-jar</i>, <i>org.acme:project-a:::tests-jdk15</i>
    --- End diff --
   
    Did the previous pattern `groupId:artifactId` support `*` wildcard. Does your algorithm support `*`? I know that other plugins usually support it with `*`. If both, empty character between :: and `*`, would work, even better.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire pull request #173: [SUREFIRE-1004] Support full gavtc format ...

bradflood
In reply to this post by bradflood
Github user Tibor17 commented on a diff in the pull request:

    https://github.com/apache/maven-surefire/pull/173#discussion_r161336480
 
    --- Diff: maven-surefire-common/src/test/java/org/apache/maven/plugin/surefire/util/DependenciesScannerTest.java ---
    @@ -69,19 +78,141 @@ public void testLocateTestClasses()
             assertEquals( 1, props.size() );
         }
     
    -    private File writeTestFile()
    +    /**
    +     * Test for artifact with classifier
    +     */
    +    @Test
    +    public void testLocateTestClassesFromArtifactWithClassifier()
    +        throws Exception
    +    {
    +        File testJarFile = writeTestFile( "DependenciesScannerTest2-1.0-tests-jdk15.jar", "org/test/TestA.class",
    +                                          "org/test/other/TestAA.class", "org/test/TestB.class" );
    +        Artifact testArtifact =
    +            new DefaultArtifact( "org.surefire.dependency", "dependent-artifact2",
    +                                 VersionRange.createFromVersion( "1.0" ), "test", "jar", "tests-jdk15", null );
    +        testArtifact.setFile( testJarFile );
    +
    +        List<String> scanDependencies = new ArrayList<String>();
    +        scanDependencies.add( "org.surefire.dependency:dependent-artifact2:::tests-jdk15" );
    +
    +        List<String> include = new ArrayList<String>();
    +        include.add( "**/*A.java" );
    +        List<String> exclude = new ArrayList<String>();
    +
    +        DependencyScanner scanner =
    +            new DependencyScanner( DependencyScanner.filter( Collections.singletonList( testArtifact ),
    +                                                             scanDependencies ),
    +                                   new TestListResolver( include, exclude ) );
    +
    +        ScanResult classNames = scanner.scan();
    +        assertNotNull( classNames );
    +        assertEquals( 2, classNames.size() );
    +
    +        Map<String, String> props = new HashMap<String, String>();
    +        classNames.writeTo( props );
    +        assertEquals( 2, props.size() );
    +    }
    +
    +    /**
    +     * Test with type when two artifacts are present, should only find the class in jar with correct type
    +     */
    +    @Test
    +    public void testLocateTestClassesFromMultipleArtifactsWithType()
    +        throws Exception
    +    {
    +        File jarFile =
    +            writeTestFile( "DependenciesScannerTest3-1.0.jar", "org/test/ClassA.class", "org/test/ClassB.class" );
    +        Artifact mainArtifact = new DefaultArtifact( "org.surefire.dependency", "dependent-artifact3",
    +                                                     VersionRange.createFromVersion( "1.0" ), "test", "jar", null,
    +                                                     new DefaultArtifactHandler() );
    +        mainArtifact.setFile( jarFile );
    +
    +        File testJarFile =
    +            writeTestFile( "DependenciesScannerTest3-1.0-tests.jar", "org/test/TestA.class", "org/test/TestB.class" );
    +        Artifact testArtifact = new DefaultArtifact( "org.surefire.dependency", "dependent-artifact3",
    +                                                     VersionRange.createFromVersion( "1.0" ), "test", "test-jar", null,
    +                                                     new DefaultArtifactHandler() );
    +        testArtifact.setFile( testJarFile );
    +
    +        List<String> scanDependencies = new ArrayList<String>();
    +        scanDependencies.add( "org.surefire.dependency:dependent-artifact3::test-jar" );
    +
    +        List<String> include = new ArrayList<String>();
    +        include.add( "**/*A.java" );
    +        List<String> exclude = new ArrayList<String>();
    +
    +        DependencyScanner scanner =
    +            new DependencyScanner( DependencyScanner.filter( Arrays.asList( mainArtifact, testArtifact ),
    --- End diff --
   
    Try to make the constructor call more compact using e.g. static imports or extracting to another local variables.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire pull request #173: [SUREFIRE-1004] Support full gavtc format ...

bradflood
In reply to this post by bradflood
Github user Tibor17 commented on a diff in the pull request:

    https://github.com/apache/maven-surefire/pull/173#discussion_r161336602
 
    --- Diff: surefire-integration-tests/src/test/java/org/apache/maven/surefire/its/jiras/Surefire1004RunTestFromDependencyJarsTypeAndClassifierIT.java ---
    @@ -0,0 +1,57 @@
    +package org.apache.maven.surefire.its.jiras;
    +
    +/*
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + */
    +
    +import static org.junit.Assert.*;
    --- End diff --
   
    Same here.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire pull request #173: [SUREFIRE-1004] Support full gavtc format ...

bradflood
In reply to this post by bradflood
Github user Tibor17 commented on a diff in the pull request:

    https://github.com/apache/maven-surefire/pull/173#discussion_r161337637
 
    --- Diff: maven-surefire-plugin/src/site/apt/examples/inclusion-exclusion.apt.vm ---
    @@ -209,4 +209,41 @@ Inclusions and Exclusions of Tests
       Multiple formats can be additionally combined.
       This syntax can be used in parameters: <<<test>>>, <<<includes>>>, <<<excludes>>>, <<<includesFile>>>, <<<excludesFile>>>.
     
    +* Tests from dependencies
    +
    +  By default, ${thisPlugin} will only scan for test classes to execute in the configured <<<testSourceDirectory>>>. To
    --- End diff --
   
    Here you should say that the feature VTC was introduced in version 2.21.0: `Since 2.21.0 ...`


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire issue #173: [SUREFIRE-1004] Support full gavtc format for dep...

bradflood
In reply to this post by bradflood
Github user Tibor17 commented on the issue:

    https://github.com/apache/maven-surefire/pull/173
 
    @binduk
    We have modify the maven coordinates to `groupId:artifactId:packaging:classifier:version`, see the docs:
    https://maven.apache.org/pom.html#Maven_Coordinates
    Basically, only the version would be determined last.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire issue #173: [SUREFIRE-1004] Support full gavtc format for dep...

bradflood
In reply to this post by bradflood
Github user Tibor17 commented on the issue:

    https://github.com/apache/maven-surefire/pull/173
 
    @bindul
    Then we have to support also other alternatives:
    `groupId:artifactId:version`, and
    `groupId:artifactId:packaging:version`.
    This means you have to, first of all, count `:` and call different private method/matcher for count=2, 3, 4 or 5.


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] maven-surefire issue #173: [SUREFIRE-1004] Support full gavtc format for dep...

bradflood
In reply to this post by bradflood
Github user bindul commented on the issue:

    https://github.com/apache/maven-surefire/pull/173
 
    @Tibor17,
   
    Sorry I misunderstood your first comment. Thank you again for taking the time to review the changes and for your patience.
   
    I will rework the pull request using your [latest comment](https://github.com/apache/maven-surefire/pull/173#issuecomment-357434597) and the inline review comments.
   
    I will get back to you soon.
   
    Bindul


---