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

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

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

bentatham
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...

bentatham
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...

bentatham
In reply to this post by bentatham
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...

bentatham
In reply to this post by bentatham
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 ...

bentatham
In reply to this post by bentatham
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 ...

bentatham
In reply to this post by bentatham
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 ...

bentatham
In reply to this post by bentatham
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 ...

bentatham
In reply to this post by bentatham
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 ...

bentatham
In reply to this post by bentatham
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 ...

bentatham
In reply to this post by bentatham
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 ...

bentatham
In reply to this post by bentatham
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 ...

bentatham
In reply to this post by bentatham
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 ...

bentatham
In reply to this post by bentatham
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 ...

bentatham
In reply to this post by bentatham
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 ...

bentatham
In reply to this post by bentatham
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...

bentatham
In reply to this post by bentatham
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...

bentatham
In reply to this post by bentatham
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...

bentatham
In reply to this post by bentatham
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


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

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

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

    https://github.com/apache/maven-surefire/pull/173
 
    @Tibor17
   
    I have reviewed the Maven Coordinates documentation you mentioned, and I can switch the order of elements in the parameter easily. However, I think I would disagree with requiring the version to be last element in the coordinates in this use case. As the functionality stands, the `dependenciesToScan` configuration does not add additional dependencies to the test scope of the project, it filters dependencies already added to the test scope in the project to allow for scanning test classes to run. If someone wants to add say a classfier or a type/packaging, requiring them them to also mention the version of the dependency would just make maintainers life harder by having another location to keep the version of the dependency in sync.
   
    As such I propose, we keep the version as optional and support the following variations of dependencies to scan:
   
    - `groupId:artifactId`
    - `groupId:artifactId:packaging/type`
    - `groupId:artifactId:packaging/type::version`
    - `groupId:artifactId:packaging/type:classifier`
    - `groupId:artifactId::classifier`
    - `groupId:artifactId::classifier:version`
    - `groupId:artifactId:packaging/type:classifier:version`
   
    It still maintains the same order of elements as the Maven Coordinates documentation in the POM, just makes the version not required to be the last element, or rather makes version optional.
   
    Thoughts?


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

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

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

    https://github.com/apache/maven-surefire/pull/173
 
    Thx for your work. I will get back to you soon. Right now I am working on
    branch SUREFIRE-1463 which contains few fixes. I plan to finish this,
    including build script, and then I would push my changes. I will make a
    code review in this PR as I do in every other before accepting. Please wait
    for me meanwhile.
   
    On Sat, Feb 10, 2018 at 1:42 AM, Bindul Bhowmik <[hidden email]>
    wrote:
   
    > @Tibor17 <https://github.com/tibor17>
    >
    > I have reviewed the Maven Coordinates documentation you mentioned, and I
    > can switch the order of elements in the parameter easily. However, I think
    > I would disagree with requiring the version to be last element in the
    > coordinates in this use case. As the functionality stands, the
    > dependenciesToScan configuration does not add additional dependencies to
    > the test scope of the project, it filters dependencies already added to the
    > test scope in the project to allow for scanning test classes to run. If
    > someone wants to add say a classfier or a type/packaging, requiring them
    > them to also mention the version of the dependency would just make
    > maintainers life harder by having another location to keep the version of
    > the dependency in sync.
    >
    > As such I propose, we keep the version as optional and support the
    > following variations of dependencies to scan:
    >
    >    - groupId:artifactId
    >    - groupId:artifactId:packaging/type
    >    - groupId:artifactId:packaging/type::version
    >    - groupId:artifactId:packaging/type:classifier
    >    - groupId:artifactId::classifier
    >    - groupId:artifactId::classifier:version
    >    - groupId:artifactId:packaging/type:classifier:version
    >
    > It still maintains the same order of elements as the Maven Coordinates
    > documentation in the POM, just makes the version not required to be the
    > last element, or rather makes version optional.
    >
    > Thoughts?
    >
    > —
    > You are receiving this because you were mentioned.
    > Reply to this email directly, view it on GitHub
    > <https://github.com/apache/maven-surefire/pull/173#issuecomment-364609503>,
    > or mute the thread
    > <https://github.com/notifications/unsubscribe-auth/AA_yR2UI0i46PG43W3OWYyJ0NwaY_X9Pks5tTOYHgaJpZM4RcCcQ>
    > .
    >
   
   
   
    --
    Cheers
    Tibor



---