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/12/03 17:56:39 UTC
[GitHub] [maven-surefire] imonteroperez opened a new pull request #400: Support for method filtering on excludesFile
imonteroperez opened a new pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400
### Highlights
- This PR provides support for method filtering on `excludesFile`.
- If we perform `mvn test -Dsurefire.excludesFile=/foo/bar/file` and it is using method filtering like:
```
test.SomeTest#test1
test.SomeTest#test2
test.SomeTest#test3
test.SomeTest#test4
test.SomeTest#test5
test.SomeTest#test6
```
- Before this PR then we obtain `Method filter prohibited in includes|excludes|includesFile|excludesFile parameter`
- After this PR method filter is applied, so we obtain:
```
[INFO] --- maven-surefire-plugin:3.0.0-M6-SNAPSHOT:test (default-test) @ simple-maven-project-with-tests ---
[INFO]
[INFO] -------------------------------------------------------
[INFO] T E S T S
[INFO] -------------------------------------------------------
[INFO] Running test.SomeTest
[INFO] Tests run: 0, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.035 s - in test.SomeTest
[INFO] Running test.OtherTest
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.001 s - in test.OtherTest
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 1.808 s
[INFO] Finished at: 2021-12-03T18:00:37+01:00
[INFO] ------------------------------------------------------------------------
```
Following this checklist to help us incorporate your
contribution quickly and easily:
- [ ] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/SUREFIRE) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [ ] Each commit in the pull request should have a meaningful subject line and body.
- [ ] Format the pull request title like `[SUREFIRE-XXX] - Fixes bug in ApproximateQuantiles`,
where you replace `SUREFIRE-XXX` with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
- [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [ ] Run `mvn clean install` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [ ] You have run the integration tests successfully (`mvn -Prun-its clean install`).
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [ ] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] slawekjaranowski commented on a change in pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on a change in pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#discussion_r773294587
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -1839,14 +1842,38 @@ private PluginFailureReason getEffectiveFailIfNoTests()
{
if ( isSpecificTestSpecified() )
{
- return getFailIfNoSpecifiedTests() ? COULD_NOT_RUN_SPECIFIED_TESTS : NONE;
+ if ( getFailIfNoSpecifiedTests()
+ && isTestToIncludeExcludeNoSpecified() )
+ {
+ return COULD_NOT_RUN_SPECIFIED_TESTS;
+ }
+ else
+ {
+ return NONE;
+ }
}
else
{
return getFailIfNoTests() ? COULD_NOT_RUN_DEFAULT_TESTS : NONE;
}
}
+ private boolean isTestToIncludeExcludeNoSpecified()
+ {
+ return getTestToIncludeExclude().isEmpty();
+ }
+
+ private void setTestToIncludeExclude( String test )
+ {
+ this.testToIncludeExclude = test;
+ setTest( test );
+ }
Review comment:
mixed responsibility ..
when I see a call some setter method I expect that one property will be set ...
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -2282,32 +2336,113 @@ private void maybeAppendList( List<String> base, List<String> list )
return filterNulls( includes );
}
- private void checkMethodFilterInIncludesExcludes( Iterable<String> patterns )
- throws MojoFailureException
+ private List<String> getMethodFilterInIncludesExcludes( Iterable<String> patterns )
{
+ List<String> methodFilterInIncludesExcludes = new LinkedList<>();
if ( patterns != null )
{
for ( String pattern : patterns )
{
if ( pattern != null && pattern.contains( "#" ) )
{
- throw new MojoFailureException( "Method filter prohibited in "
- + "includes|excludes|includesFile|excludesFile parameter: "
- + pattern );
+ methodFilterInIncludesExcludes.add( pattern );
}
}
}
+ return methodFilterInIncludesExcludes;
}
private TestListResolver getIncludedAndExcludedTests()
throws MojoFailureException
{
if ( includedExcludedTests == null )
{
- includedExcludedTests = new TestListResolver( getIncludeList(), getExcludeList() );
+ StringBuilder sb = new StringBuilder();
+ List<String> includeList = getIncludeList();
+ for ( String include: includeList )
+ {
+ sb.append( "," ).append( include );
+ }
+ IncludeExcludeList excludeList = getExcludeList();
+ List<String> excludedTestMethods = excludeList.getTestMethods();
+ if ( !excludedTestMethods.isEmpty() )
+ {
+ includeList.clear();
Review comment:
`includeList` is return value from `getIncludeList` should be immutable ...
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -2271,7 +2324,8 @@ private void maybeAppendList( List<String> base, List<String> list )
maybeAppendList( includes, getIncludes() );
}
- checkMethodFilterInIncludesExcludes( includes );
+ List<String> methodFilterInIncludes = getMethodFilterInIncludesExcludes( includes );
+ addAll( includes, methodFilterInIncludes.toArray( new String[0] ) );
Review comment:
`includes` can be reference to anything - should be immutable
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -2282,32 +2336,113 @@ private void maybeAppendList( List<String> base, List<String> list )
return filterNulls( includes );
}
- private void checkMethodFilterInIncludesExcludes( Iterable<String> patterns )
- throws MojoFailureException
+ private List<String> getMethodFilterInIncludesExcludes( Iterable<String> patterns )
{
+ List<String> methodFilterInIncludesExcludes = new LinkedList<>();
if ( patterns != null )
{
for ( String pattern : patterns )
{
if ( pattern != null && pattern.contains( "#" ) )
{
- throw new MojoFailureException( "Method filter prohibited in "
- + "includes|excludes|includesFile|excludesFile parameter: "
- + pattern );
+ methodFilterInIncludesExcludes.add( pattern );
}
}
}
+ return methodFilterInIncludesExcludes;
}
private TestListResolver getIncludedAndExcludedTests()
throws MojoFailureException
{
if ( includedExcludedTests == null )
{
- includedExcludedTests = new TestListResolver( getIncludeList(), getExcludeList() );
+ StringBuilder sb = new StringBuilder();
+ List<String> includeList = getIncludeList();
+ for ( String include: includeList )
+ {
+ sb.append( "," ).append( include );
+ }
+ IncludeExcludeList excludeList = getExcludeList();
+ List<String> excludedTestMethods = excludeList.getTestMethods();
+ if ( !excludedTestMethods.isEmpty() )
+ {
+ includeList.clear();
+ includeList.addAll( excludedTestMethods );
+ for ( String method : excludedTestMethods )
+ {
+ sb.append( "," ).append( method );
+ }
+ }
+ if ( !Arrays.equals(
+ Collections.singletonList( getDefaultExcludes() )
+ .toArray( new String[0] ),
+ excludeList.getTestClasses().toArray( new String[0] ) ) )
+ {
+ for ( String clazz : excludeList.getTestClasses() )
+ {
+ sb.append( "," ).append( clazz );
+ }
+ }
+
+ if ( sb.length() == 0 )
+ {
+ includedExcludedTests = new TestListResolver(
+ asList ( getDefaultIncludes() ),
+ Collections.singletonList( getDefaultExcludes() ) );
+ }
+ else
+ {
+ String testToIncludeExclude = sb.deleteCharAt( 0 ).toString( );
+ if ( !Arrays.equals( getDefaultIncludes(), split( testToIncludeExclude, "," ) ) )
+ {
+ setTestToIncludeExclude( testToIncludeExclude );
+ }
+ includedExcludedTests = new TestListResolver( includeList, excludeList.getTestClasses() );
+ }
}
return includedExcludedTests;
}
+
+ class IncludeExcludeList
+ {
+
+ private List<String> testClasses = new LinkedList<>();
+
+ private List<String> testMethods = new LinkedList<>();
+
+ IncludeExcludeList( List<String> testClasses, List<String> testMethods )
+ {
+ super();
Review comment:
needed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#issuecomment-1011657874
@imonteroperez
@slawekjaranowski
Pls let a have a look into #440 as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#issuecomment-1010198845
@imonteroperez
I did not mean to put the change on this branch. Here are too many and unnecessary changes.
The fix should be quite simple. The lists are merged in those two methods we know and no other parts of the code should be modified.
Pls try to modify the methods `getIncludeList` and `getExcludeList` on the master. There is no reason why it should not work with minimum change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] imonteroperez commented on pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
imonteroperez commented on pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#issuecomment-1009866925
> @imonteroperez @slawekjaranowski @olamy
>
> The includes|excludes, includesFile|excludesFile end up within the TestListResolver. There is no need to modify the implementation of Provider, TestListResolver and other stuff. All we could do is to compromise the sanity check `Method filter prohibited in includes|excludes|includesFile|excludesFile parameter`.
>
> Below you can see the original method for includesFile. And then the sanity check is shifted in the modified code, this is my proposal.
>
> ```
> private List<String> getIncludeList()
> throws MojoFailureException
> {
> List<String> includes = null;
> if ( isSpecificTestSpecified() )
> {
> includes = new ArrayList<>();
> addAll( includes, split( getTest(), "," ) );
> }
> else
> {
> if ( getIncludesFile() != null )
> {
> includes = readListFromFile( getIncludesFile() );
> }
>
> if ( includes == null )
> {
> includes = getIncludes();
> }
> else
> {
> maybeAppendList( includes, getIncludes() );
> }
>
> checkMethodFilterInIncludesExcludes( includes );
>
> if ( includes == null || includes.isEmpty() )
> {
> includes = asList( getDefaultIncludes() );
> }
> }
>
> return filterNulls( includes );
> }
> ```
>
> changed to
>
> ```
> private List<String> getIncludeList()
> throws MojoFailureException
> {
> List<String> includes = null;
> if ( isSpecificTestSpecified() )
> {
> includes = new ArrayList<>();
> addAll( includes, split( getTest(), "," ) );
> }
> else
> {
> includes = getIncludes();
>
> checkMethodFilterInIncludesExcludes( includes );
>
> if ( getIncludesFile() != null )
> {
> if ( includes == null )
> {
> includes = new ArrayList<>();
> }
> addAll( includes, readListFromFile( getIncludesFile() ) );
> }
>
> if ( includes == null || includes.isEmpty() )
> {
> includes = asList( getDefaultIncludes() );
> }
> }
>
> return filterNulls( includes );
> }
> ```
>
> Similar with the excludesFile.
>
> @imonteroperez Would this help you? Of course we do not know the IT results yet. They may fail.., not sure! The JavaDoc of both parameters should say that the pattern allows using #method, and the site documentation too.
I think this is not going to work because IIUC then is going to be delegated to a FileScanner to evaluate which test classes should be executed without considering the method filtering. See: https://github.com/apache/maven-surefire/blob/master/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/util/FileScanner.java#L74 where method is `null`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#issuecomment-1003142907
@imonteroperez
@slawekjaranowski
@olamy
The includes|excludes, includesFile|excludesFile end up within the TestListResolver.
There is no need to modify the implementation of Provider, TestListResolver and other stuff.
All we could do is to compromise the sanity check `Method filter prohibited in includes|excludes|includesFile|excludesFile parameter`.
Below you can see the original method for includesFile. And then the sanity check is shifted in the modified code, this is my proposal.
```
private List<String> getIncludeList()
throws MojoFailureException
{
List<String> includes = null;
if ( isSpecificTestSpecified() )
{
includes = new ArrayList<>();
addAll( includes, split( getTest(), "," ) );
}
else
{
if ( getIncludesFile() != null )
{
includes = readListFromFile( getIncludesFile() );
}
if ( includes == null )
{
includes = getIncludes();
}
else
{
maybeAppendList( includes, getIncludes() );
}
checkMethodFilterInIncludesExcludes( includes );
if ( includes == null || includes.isEmpty() )
{
includes = asList( getDefaultIncludes() );
}
}
return filterNulls( includes );
}
```
changed to
```
private List<String> getIncludeList()
throws MojoFailureException
{
List<String> includes = null;
if ( isSpecificTestSpecified() )
{
includes = new ArrayList<>();
addAll( includes, split( getTest(), "," ) );
}
else
{
includes = getIncludes();
checkMethodFilterInIncludesExcludes( includes );
if ( getIncludesFile() != null )
{
includes = readListFromFile( getIncludesFile() );
}
if ( includes == null || includes.isEmpty() )
{
includes = asList( getDefaultIncludes() );
}
}
return filterNulls( includes );
}
```
Similar with the excludesFile.
@imonteroperez
Would this help you?
Of course we do not know the IT results yet. They may fail.., not sure!
The JavaDoc of both parameters should say that the pattern allows using #method, and the site documentation 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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] slawekjaranowski commented on a change in pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on a change in pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#discussion_r775224480
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -1839,7 +1841,7 @@ private PluginFailureReason getEffectiveFailIfNoTests()
{
if ( isSpecificTestSpecified() )
{
- return getFailIfNoSpecifiedTests() ? COULD_NOT_RUN_SPECIFIED_TESTS : NONE;
+ return getFailIfNoSpecifiedTests() ? COULD_NOT_RUN_SPECIFIED_TESTS : NONE;
Review comment:
It will be ok for checkstyle?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#issuecomment-1003142907
@imonteroperez
@slawekjaranowski
@olamy
The includes|excludes, includesFile|excludesFile end up within the TestListResolver.
There is no need to modify the implementation of Provider, TestListResolver and other stuff.
All we could do is to compromise the sanity check `Method filter prohibited in includes|excludes|includesFile|excludesFile parameter`.
Below you can see the original method for includesFile. And then the sanity check is shifted in the modified code, this is my proposal.
```
private List<String> getIncludeList()
throws MojoFailureException
{
List<String> includes = null;
if ( isSpecificTestSpecified() )
{
includes = new ArrayList<>();
addAll( includes, split( getTest(), "," ) );
}
else
{
if ( getIncludesFile() != null )
{
includes = readListFromFile( getIncludesFile() );
}
if ( includes == null )
{
includes = getIncludes();
}
else
{
maybeAppendList( includes, getIncludes() );
}
checkMethodFilterInIncludesExcludes( includes );
if ( includes == null || includes.isEmpty() )
{
includes = asList( getDefaultIncludes() );
}
}
return filterNulls( includes );
}
```
changed to
```
private List<String> getIncludeList()
throws MojoFailureException
{
List<String> includes = null;
if ( isSpecificTestSpecified() )
{
includes = new ArrayList<>();
addAll( includes, split( getTest(), "," ) );
}
else
{
includes = getIncludes();
checkMethodFilterInIncludesExcludes( includes );
if ( getIncludesFile() != null )
{
if ( includes == null )
{
includes = new ArrayList<>();
}
addAll( includes, readListFromFile( getIncludesFile() ) );
}
if ( includes == null || includes.isEmpty() )
{
includes = asList( getDefaultIncludes() );
}
}
return filterNulls( includes );
}
```
Similar with the excludesFile.
@imonteroperez
Would this help you?
Of course we do not know the IT results yet. They may fail.., not sure!
The JavaDoc of both parameters should say that the pattern allows using #method, and the site documentation 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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] imonteroperez commented on pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
imonteroperez commented on pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#issuecomment-1009899120
> > @imonteroperez @slawekjaranowski @olamy
> > The includes|excludes, includesFile|excludesFile end up within the TestListResolver. There is no need to modify the implementation of Provider, TestListResolver and other stuff. All we could do is to compromise the sanity check `Method filter prohibited in includes|excludes|includesFile|excludesFile parameter`.
> > Below you can see the original method for includesFile. And then the sanity check is shifted in the modified code, this is my proposal.
> > ```
> > private List<String> getIncludeList()
> > throws MojoFailureException
> > {
> > List<String> includes = null;
> > if ( isSpecificTestSpecified() )
> > {
> > includes = new ArrayList<>();
> > addAll( includes, split( getTest(), "," ) );
> > }
> > else
> > {
> > if ( getIncludesFile() != null )
> > {
> > includes = readListFromFile( getIncludesFile() );
> > }
> >
> > if ( includes == null )
> > {
> > includes = getIncludes();
> > }
> > else
> > {
> > maybeAppendList( includes, getIncludes() );
> > }
> >
> > checkMethodFilterInIncludesExcludes( includes );
> >
> > if ( includes == null || includes.isEmpty() )
> > {
> > includes = asList( getDefaultIncludes() );
> > }
> > }
> >
> > return filterNulls( includes );
> > }
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > changed to
> > ```
> > private List<String> getIncludeList()
> > throws MojoFailureException
> > {
> > List<String> includes = null;
> > if ( isSpecificTestSpecified() )
> > {
> > includes = new ArrayList<>();
> > addAll( includes, split( getTest(), "," ) );
> > }
> > else
> > {
> > includes = getIncludes();
> >
> > checkMethodFilterInIncludesExcludes( includes );
> >
> > if ( getIncludesFile() != null )
> > {
> > if ( includes == null )
> > {
> > includes = new ArrayList<>();
> > }
> > addAll( includes, readListFromFile( getIncludesFile() ) );
> > }
> >
> > if ( includes == null || includes.isEmpty() )
> > {
> > includes = asList( getDefaultIncludes() );
> > }
> > }
> >
> > return filterNulls( includes );
> > }
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > Similar with the excludesFile.
> > @imonteroperez Would this help you? Of course we do not know the IT results yet. They may fail.., not sure! The JavaDoc of both parameters should say that the pattern allows using #method, and the site documentation too.
>
> I think this is not going to work because IIUC then is going to be delegated to a FileScanner to evaluate which test classes should be executed without considering the method filtering. See: https://github.com/apache/maven-surefire/blob/master/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/util/FileScanner.java#L74 where method is `null`
Applied your proposal with a combination of mine to specify the tests to run based on identifying if there is method filtering applied or not on the `TestListResolver`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] imonteroperez commented on pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
imonteroperez commented on pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#issuecomment-1022165752
Closed in favor of https://github.com/apache/maven-surefire/pull/440
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] slawekjaranowski commented on a change in pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on a change in pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#discussion_r772573695
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -2282,32 +2296,113 @@ private void maybeAppendList( List<String> base, List<String> list )
return filterNulls( includes );
}
- private void checkMethodFilterInIncludesExcludes( Iterable<String> patterns )
- throws MojoFailureException
+ private List<String> getMethodFilterInIncludesExcludes( Iterable<String> patterns )
{
+ List<String> methodFilterInIncludesExcludes = new LinkedList<>();
if ( patterns != null )
{
for ( String pattern : patterns )
{
if ( pattern != null && pattern.contains( "#" ) )
{
- throw new MojoFailureException( "Method filter prohibited in "
- + "includes|excludes|includesFile|excludesFile parameter: "
- + pattern );
+ methodFilterInIncludesExcludes.add( pattern );
}
}
}
+ return methodFilterInIncludesExcludes;
}
private TestListResolver getIncludedAndExcludedTests()
throws MojoFailureException
{
if ( includedExcludedTests == null )
{
- includedExcludedTests = new TestListResolver( getIncludeList(), getExcludeList() );
+ StringBuilder sb = new StringBuilder();
+ List<String> includeList = getIncludeList();
+ for ( String include: includeList )
+ {
+ sb.append( "," ).append( include );
+ }
+ IncludeExcludeList excludeList = getExcludeList();
+ List<String> excludedTestMethods = excludeList.getTestMethods();
+ if ( !excludedTestMethods.isEmpty() )
+ {
+ includeList.clear();
+ includeList.addAll( excludedTestMethods );
+ for ( String method : excludedTestMethods )
+ {
+ sb.append( "," ).append( method );
+ }
+ }
+ if ( !Arrays.equals(
+ Collections.singletonList( getDefaultExcludes() )
+ .toArray( new String[0] ),
+ excludeList.getTestClasses().toArray( new String[0] ) ) )
+ {
+ for ( String clazz : excludeList.getTestClasses() )
+ {
+ sb.append( "," ).append( clazz );
+ }
+ }
+
+ if ( sb.length() == 0 )
+ {
+ includedExcludedTests = new TestListResolver(
+ asList ( getDefaultIncludes() ),
+ Collections.singletonList( getDefaultExcludes() ) );
+ }
+ else
+ {
+ String testToIncludeExclude = sb.deleteCharAt( 0 ).toString( );
+ if ( !Arrays.equals( getDefaultIncludes(), split( testToIncludeExclude, "," ) ) )
+ {
+ setTest( testToIncludeExclude );
+ }
+ includedExcludedTests = new TestListResolver( includeList, excludeList.getTestClasses() );
+ }
Review comment:
There are many decision branches here.
Maybe it will be possible put this logic in `TestListResolver` class and prepare unit test for it to cover all branches.
Such tests can show which input params impact to result 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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] slawekjaranowski commented on pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#issuecomment-1010137098
code looks ok
Please add documentation to Mojo params and to site with examples.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] imonteroperez commented on pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
imonteroperez commented on pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#issuecomment-1001524984
> @imonteroperez We already discussed this feature request with @olamy in ASF. This feature brings chaos to the meaning of the config parameter and a duplicate to filter `test` which have the space for the method filtering. Naturally, `includesFile` cannot be about filtering methods. Instead of those parameters pls use `<test>*Test#test*One+testTwo?????</test>`. Why you do not use the config parameter `test`? Do you want to override or merge this parameter with system property `test`?
Hi @Tibor17 I know I can use `-Dtest`, but I was looking for a solution to expand the method filtering capabilities to be used on `surefire.excludesFile` and (additionally) on `surefire.includesFile`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] imonteroperez commented on pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
imonteroperez commented on pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#issuecomment-1001526015
Moving this PR to draft status to achieve some of the feedback provided and to include more testing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#issuecomment-1003142907
@imonteroperez
@slawekjaranowski
@olamy
The includes|excludes, includesFile|excludesFile end up within the TestListResolver.
There is no need to modify the implementation of Provider, TestListResolver and other stuff.
All we could do is to compromise the sanity check `Method filter prohibited in includes|excludes|includesFile|excludesFile parameter`.
Below you can see the original method for includesFile. And then the sanity check is shifted in the modified code, this is my proposal.
```
private List<String> getIncludeList()
throws MojoFailureException
{
List<String> includes = null;
if ( isSpecificTestSpecified() )
{
includes = new ArrayList<>();
addAll( includes, split( getTest(), "," ) );
}
else
{
if ( getIncludesFile() != null )
{
includes = readListFromFile( getIncludesFile() );
}
if ( includes == null )
{
includes = getIncludes();
}
else
{
maybeAppendList( includes, getIncludes() );
}
checkMethodFilterInIncludesExcludes( includes );
if ( includes == null || includes.isEmpty() )
{
includes = asList( getDefaultIncludes() );
}
}
return filterNulls( includes );
}
```
changed to
```
private List<String> getIncludeList()
throws MojoFailureException
{
List<String> includes = null;
if ( isSpecificTestSpecified() )
{
includes = new ArrayList<>();
addAll( includes, split( getTest(), "," ) );
}
else
{
includes = getIncludes();
checkMethodFilterInIncludesExcludes( includes );
if ( getIncludesFile() != null )
{
includes = readListFromFile( getIncludesFile() );
}
if ( includes == null || includes.isEmpty() )
{
includes = asList( getDefaultIncludes() );
}
}
return filterNulls( includes );
}
```
Similar with the excludesFile.
@imonteroperez
Would this help you?
Of course we do not know the IT results yet. They may fail.., not sure!
Of course the JavaDoc of both parameters should say that the pattern allows using #method, and the site documentation 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.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] imonteroperez closed pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
imonteroperez closed pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] imonteroperez commented on pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
imonteroperez commented on pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#issuecomment-997705698
> @imonteroperez please rebase and resolve conflicts
Updated on https://github.com/apache/maven-surefire/pull/400/commits/abfd73f25e29798a47454c5dc643ff2967e6bc86 and no conflicts obtained
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] olamy commented on pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
olamy commented on pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#issuecomment-997720857
@slawekjaranowski I cannot see any conflicts and btw we can squash when merging..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#issuecomment-1001225444
@imonteroperez
We already discussed this feature request with @olamy in ASF.
This feature brings chaos to the meaning of the config parameter and a duplicate to filter `test` which have the space for the method filtering. Naturally, `includesFile` cannot be about filtering methods.
Instead of those parameters pls use `<test>*Test#test*One+testTwo?????</test>`.
Why you do not use the config parameter `test`? Do you want to override or merge this parameter with system property `test`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] olamy commented on pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
olamy commented on pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#issuecomment-1002483985
> @imonteroperez We already discussed this feature request with @olamy in ASF. This feature brings chaos to the meaning of the config parameter and a duplicate to filter `test` which have the space for the method filtering. Naturally, `includesFile` cannot be about filtering methods. Instead of those parameters pls use `<test>*Test#test*One+testTwo?????
</test>`. Why you do not use the config parameter `test`? Do you want to override or merge this parameter with system property `test`?
`Naturally`? the parameter is called `includesFile` and not `includesClassesFile` so naturally it would make sense to include methods filtering as well. the parameter `test` does `naturally`?
At the moment it's not consistent not being able to have method filtering when using files.
Naturally it's a good idea to have same level of filtering support for both parameter and it will be more consistent.
very long command line parameter are not easy to use.
@Tibor17 I don't see any problem make parameters consistent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] imonteroperez commented on a change in pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
imonteroperez commented on a change in pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#discussion_r782305865
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -2292,23 +2291,52 @@ private void checkMethodFilterInIncludesExcludes( Iterable<String> patterns )
if ( pattern != null && pattern.contains( "#" ) )
{
throw new MojoFailureException( "Method filter prohibited in "
- + "includes|excludes|includesFile|excludesFile parameter: "
+ + "includes|excludes parameter: "
+ pattern );
}
}
}
}
- private TestListResolver getIncludedAndExcludedTests()
+ private TestListResolver solveIncludedAndExcludedTests()
Review comment:
Updated in https://github.com/apache/maven-surefire/pull/400/commits/bc8768bccda866ee3edbb7c12a8f439ff351bcdd
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] imonteroperez edited a comment on pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
imonteroperez edited a comment on pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#issuecomment-1009899120
> I think this is not going to work because IIUC then is going to be delegated to a FileScanner to evaluate which test classes should be executed without considering the method filtering. See: https://github.com/apache/maven-surefire/blob/master/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/util/FileScanner.java#L74 where method is `null`
Applied in https://github.com/apache/maven-surefire/pull/400/commits/20d880d11eba91063aba160139d7a69a0c07d478 your proposal with a combination of mine to specify the tests to run based on identifying if there is method filtering applied or not on the `TestListResolver`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] slawekjaranowski commented on pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#issuecomment-997432621
@imonteroperez please rebase and resolve conflicts
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] slawekjaranowski commented on pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#issuecomment-997718157
Please also squash to one finish commit
conflicts still exist ... `This branch cannot be rebased due to conflicts`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] imonteroperez edited a comment on pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
imonteroperez edited a comment on pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#issuecomment-1009899120
> I think this is not going to work because IIUC then is going to be delegated to a FileScanner to evaluate which test classes should be executed without considering the method filtering. See: https://github.com/apache/maven-surefire/blob/master/maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/util/FileScanner.java#L74 where method is `null`
Applied in https://github.com/apache/maven-surefire/pull/400/commits/199184f9ace1e7e38fb814b9b8936321bf999ded your proposal with a combination of mine to specify the tests to run based on identifying if there is method filtering applied or not on the `TestListResolver`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] slawekjaranowski commented on a change in pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
slawekjaranowski commented on a change in pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#discussion_r782221367
##########
File path: maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
##########
@@ -2292,23 +2291,52 @@ private void checkMethodFilterInIncludesExcludes( Iterable<String> patterns )
if ( pattern != null && pattern.contains( "#" ) )
{
throw new MojoFailureException( "Method filter prohibited in "
- + "includes|excludes|includesFile|excludesFile parameter: "
+ + "includes|excludes parameter: "
+ pattern );
}
}
}
}
- private TestListResolver getIncludedAndExcludedTests()
+ private TestListResolver solveIncludedAndExcludedTests()
Review comment:
Method returns two values ....
One by standard `return` statement and second by call `setTest(...)` which changes `Mojo` state and can cause impact to other place ...
I didn't analyze flow where `solveIncludedAndExcludedTests` and `getTest` are used and what order are.
Try to describe what does this method do ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 edited a comment on pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
Tibor17 edited a comment on pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#issuecomment-1001225444
@imonteroperez
We already discussed this feature request with @olamy in ASF.
This feature bring chaos to the meaning of the config parameter and a duplicate to inclusion filter which have the space for the method filtering. Naturally, `includesFile` cannot be about filtering methods.
Instead of those parameters pls use `<test>*Test#test*One+testTwo?????</test>`.
Why you do not use the config parameter `test`? Do you want to override or merge this parameter with system property `test`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-surefire] Tibor17 commented on pull request #400: [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile
Posted by GitBox <gi...@apache.org>.
Tibor17 commented on pull request #400:
URL: https://github.com/apache/maven-surefire/pull/400#issuecomment-1001225444
@imonteroperez
We already discussed this feature request with @olamy in ASF.
This feature bring chaos to the meaning of the config parameter and a duplicate to inclusion filter which have the space for the method filtering. Naturally, `includesFile` cannot be about filtering methods.
Instead of those parameters pls use `<test>*Test#test*One+testTwo?????</test>`.
Why you do not use the config parameter `test`? Do you want to override or merge this parameter with system property `test`?
TestListResolver
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org