[SUREFIRE-1964] Support for method filtering on excludesFile and includesFile #400

imonteroperez · 2021-12-03T17:56:26Z

Highlights

See SUREFIRE-1964
This PR provides support for method filtering on excludesFile and includesFile.
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 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 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
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
In any other case, please file an Apache Individual Contributor License Agreement.

…excludesFile

slawekjaranowski · 2021-12-19T17:46:50Z

@imonteroperez please rebase and resolve conflicts

…excludesFile

imonteroperez · 2021-12-20T08:31:39Z

@imonteroperez please rebase and resolve conflicts

Updated on abfd73f and no conflicts obtained

slawekjaranowski · 2021-12-20T08:49:15Z

Please also squash to one finish commit
conflicts still exist ... This branch cannot be rebased due to conflicts

olamy · 2021-12-20T08:53:14Z

@slawekjaranowski I cannot see any conflicts and btw we can squash when merging..

slawekjaranowski

Documentation for new feature should be also updated ...

eg. on page https://maven.apache.org/surefire/maven-surefire-plugin/examples/inclusion-exclusion.html or create another one page.

on plugin params includesFile, excludesFile put link to page.

In other case no one will know about it.

maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java

Tibor17 · 2021-12-26T18:31:15Z

@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?

imonteroperez · 2021-12-27T11:37:06Z

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

imonteroperez · 2021-12-27T11:38:06Z

Moving this PR to draft status to achieve some of the feedback provided and to include more testing.

olamy · 2021-12-29T09:36:17Z

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

Tibor17 · 2021-12-30T18:44:56Z

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

imonteroperez · 2022-01-11T11:22:53Z

@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

imonteroperez · 2022-01-11T12:04:26Z

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 20d880d 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

maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java

slawekjaranowski · 2022-01-11T16:24:55Z

code looks ok

Please add documentation to Mojo params and to site with examples.

Tibor17 · 2022-01-11T17:33:45Z

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

Tibor17 · 2022-01-13T01:50:53Z

@imonteroperez
@slawekjaranowski
Pls let a have a look into #440 as well.

imonteroperez · 2022-01-26T12:46:36Z

Closed in favor of #440

Support for method filtering on excludesFile

29efe8b

imonteroperez force-pushed the method-filtering-excludesFile branch from c0233c9 to 67edb77 Compare December 9, 2021 17:17

Merge remote-tracking branch 'upstream/master' into method-filtering-…

00f1ba8

…excludesFile

imonteroperez force-pushed the method-filtering-excludesFile branch from 67edb77 to 00f1ba8 Compare December 10, 2021 10:25

imonteroperez changed the title ~~Support for method filtering on excludesFile~~ Support for method filtering on excludesFile and includesFile Dec 10, 2021

imonteroperez changed the title ~~Support for method filtering on excludesFile and includesFile~~ [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile Dec 10, 2021

imonteroperez marked this pull request as ready for review December 10, 2021 10:38

Merge remote-tracking branch 'upstream/master' into method-filtering-…

abfd73f

…excludesFile

Fix detected bug on setting up the includes

85d0c72

slawekjaranowski requested changes Dec 20, 2021

View reviewed changes

maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java Outdated Show resolved Hide resolved

Fix on filtering test classes

4e33bf1

imonteroperez force-pushed the method-filtering-excludesFile branch from 3d723f5 to 4e33bf1 Compare December 21, 2021 12:36

Fix on dealing with specified tests

1adf848

slawekjaranowski requested changes Dec 21, 2021

View reviewed changes

Remove extra field

2717853

slawekjaranowski reviewed Dec 26, 2021

View reviewed changes

maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java Outdated Show resolved Hide resolved

imonteroperez marked this pull request as draft December 27, 2021 11:38

imonteroperez force-pushed the method-filtering-excludesFile branch from 199184f to 20d880d Compare January 11, 2022 12:15

imonteroperez force-pushed the method-filtering-excludesFile branch 4 times, most recently from 687be88 to af199d1 Compare January 11, 2022 13:17

Refactor based on some feedback

37cf5fc

imonteroperez force-pushed the method-filtering-excludesFile branch from af199d1 to 37cf5fc Compare January 11, 2022 14:13

slawekjaranowski reviewed Jan 11, 2022

View reviewed changes

maven-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java Outdated Show resolved Hide resolved

Refactor solve method and provide description

bc8768b

imonteroperez force-pushed the method-filtering-excludesFile branch from 12e509c to bc8768b Compare January 11, 2022 15:41

Tibor17 self-requested a review January 11, 2022 17:24

imonteroperez force-pushed the method-filtering-excludesFile branch from 160b7bb to bc8768b Compare January 14, 2022 10:29

imonteroperez mentioned this pull request Jan 26, 2022

[SUREFIRE-1964] Support for method filtering on excludesFile and includesFile #440

Merged

8 tasks

imonteroperez closed this Jan 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SUREFIRE-1964] Support for method filtering on excludesFile and includesFile #400

[SUREFIRE-1964] Support for method filtering on excludesFile and includesFile #400

imonteroperez commented Dec 3, 2021 •

edited

Loading

slawekjaranowski commented Dec 19, 2021

imonteroperez commented Dec 20, 2021

slawekjaranowski commented Dec 20, 2021

olamy commented Dec 20, 2021

slawekjaranowski left a comment

Tibor17 commented Dec 26, 2021 •

edited

Loading

imonteroperez commented Dec 27, 2021

imonteroperez commented Dec 27, 2021

olamy commented Dec 29, 2021

Tibor17 commented Dec 30, 2021 •

edited

Loading

imonteroperez commented Jan 11, 2022

imonteroperez commented Jan 11, 2022 •

edited

Loading

slawekjaranowski commented Jan 11, 2022

Tibor17 commented Jan 11, 2022

Tibor17 commented Jan 13, 2022

imonteroperez commented Jan 26, 2022

[SUREFIRE-1964] Support for method filtering on excludesFile and includesFile #400

[SUREFIRE-1964] Support for method filtering on excludesFile and includesFile #400

Conversation

imonteroperez commented Dec 3, 2021 • edited Loading

Highlights

slawekjaranowski commented Dec 19, 2021

imonteroperez commented Dec 20, 2021

slawekjaranowski commented Dec 20, 2021

olamy commented Dec 20, 2021

slawekjaranowski left a comment

Choose a reason for hiding this comment

Tibor17 commented Dec 26, 2021 • edited Loading

imonteroperez commented Dec 27, 2021

imonteroperez commented Dec 27, 2021

olamy commented Dec 29, 2021

Tibor17 commented Dec 30, 2021 • edited Loading

imonteroperez commented Jan 11, 2022

imonteroperez commented Jan 11, 2022 • edited Loading

slawekjaranowski commented Jan 11, 2022

Tibor17 commented Jan 11, 2022

Tibor17 commented Jan 13, 2022

imonteroperez commented Jan 26, 2022

imonteroperez commented Dec 3, 2021 •

edited

Loading

Tibor17 commented Dec 26, 2021 •

edited

Loading

Tibor17 commented Dec 30, 2021 •

edited

Loading

imonteroperez commented Jan 11, 2022 •

edited

Loading