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

Tibor17 · 2022-01-12T22:53:51Z

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.

Tibor17 · 2022-01-13T01:42:52Z

@imonteroperez
@slawekjaranowski
Let's see this as well.
The ASM class used getTest() for the tests filter. We want to extend it with inc/excludedFiles param but it is not exactly the same list of patterns used by directory/dependency scanner. Additionally, the tests filter should not have the default inc/exc patterns because they should be only in the directory/dependency filters.
This way the behavior would be backwards compatible and your feature would present new.

Tibor17 · 2022-01-13T02:03:12Z

The docs and IT does not exist in here.

slawekjaranowski

Does not impact on any ITs tests?

slawekjaranowski · 2022-01-13T07:08:09Z

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

-                                                        + pattern );
-                }
+                throw new MojoFailureException( "Method filter prohibited in "
+                    + "includes|excludes|includesFile|excludesFile parameter: "


includesFile|excludesFile should not be supported?

good catch!
The commit 6e06d2a did not employ any ITs in 2015.

imonteroperez · 2022-01-14T14:59:19Z

Great! Do you mind including some new ITs? like the ones provided in: https://github.com/apache/maven-surefire/pull/400/files#diff-03a936fa7cdbcd585a7539659e12039d761e549c55b3f98183b2eea56860990c

Tibor17 · 2022-01-19T21:27:39Z

Hi @imonteroperez ;-)
Yes of course we will use your ITs.
I am glad that you have joined us again!
We also need to have updates of the documentation (Javadoc) of the config parameters. Do you have it as well in your commits? Can we use it?

imonteroperez · 2022-01-26T08:29:57Z

Hi @imonteroperez ;-) Yes of course we will use your ITs. I am glad that you have joined us again! We also need to have updates of the documentation (Javadoc) of the config parameters. Do you have it as well in your commits? Can we use it?

Hi @Tibor17 sorry for the late response, I got infected by COVID last week.

I did not perform any update on Javadoc in #400, so feel free to update this yourself. Only commit that you can use is the one about the ITs that I was mentioning in the previous comment. I saw that you updated the negativeTest but I would recommend also to include a new one to test the method filtering by itself.

Something like:

public void shouldNotRunExcludedTestMethodByExcludesFile() 
    {
        assumeThat( getSettings().getConfiguration(), is( INCLUDES_EXCLUDES_FILE ) );
        String pattern = "!TestFive#testSuccessOne+testSuccessThree";
        prepare( pattern )
            .executeTest()
            .verifyErrorFree( 1 )
            .verifyErrorFreeLog();
    }

FTR as side note, I early test this approach internally with +30K tests and everything working as expected 🚀

Tibor17 · 2022-01-26T23:43:50Z

@imonteroperez
Thx for the unit test. I will use it. We have not covered this code with unit test yet so this would be worth to do it now.
I have my IT already in progress, so I will commit the IT as well.
T

olamy

very nice feature to have.
@Tibor17 definitely ship it!

Tibor17 · 2022-02-06T11:52:41Z

Hi @imonteroperez
As we spoke before, pls try to finish this with adding Javadoc or extending the existing one for the config parameter, and add the unit tests since the ITs execute some scenario. I guess you do not have permissions to commit into this branch, so feel free to fork it to you own branch. I am currently working on the issue SUREFIRE-1860. Thx

imonteroperez · 2022-02-08T17:15:26Z

Hi @imonteroperez As we spoke before, pls try to finish this with adding Javadoc or extending the existing one for the config parameter, and add the unit tests since the ITs execute some scenario. I guess you do not have permissions to commit into this branch, so feel free to fork it to you own branch. I am currently working on the issue SUREFIRE-1860. Thx

Work in progress in #460

…udesFile

Tibor17 · 2022-03-02T21:14:09Z

@slawekjaranowski
Would you pls make the review? Thx

olamy · 2022-03-03T00:33:51Z

does this include the commits from #460?

imonteroperez · 2022-03-03T09:06:47Z

does this include the commits from #460?

Yes @olamy, the content from #460 has been included here

olamy · 2022-03-03T09:12:29Z

does this include the commits from #460?

Yes @olamy, the content from #460 has been included here

ok good know I was a bit confused as I could not see any reference to you in the commit.

imonteroperez · 2022-03-03T09:20:13Z

does this include the commits from #460?

Yes @olamy, the content from #460 has been included here

ok good know I was a bit confused as I could not see any reference to you in the commit.

Yes, I got confused also, seems all the content from my PR was force-pushed and my mention as a contributor get lost 😞

olamy · 2022-03-03T09:24:25Z

does this include the commits from #460?

Yes @olamy, the content from #460 has been included here

ok good know I was a bit confused as I could not see any reference to you in the commit.

Yes, I got confused also, seems all the content from my PR was force-pushed and my mention as a contributor get lost 😞

Agree you deserve some credits for the work you did.
@Tibor17 can you please fix that as @imonteroperez did a fair amount of work here and deserve to have some credits. Thanks

Tibor17 · 2022-03-04T01:50:43Z

@imonteroperez
It's the same.
I used both commits 499dc83 and 1b721a9
I have cherry picked the commits, shifted to the common module into the right test class where is mock already and finally squashed together in one.
I have only shifted the changes to the right module.
The work from @imonteroperez is here, nothing is lost, really! The Javadoc, both unit tests from @imonteroperez. I added third.

olamy · 2022-03-04T03:32:00Z

@imonteroperez It's the same. I used both commits 499dc83 and 1b721a9 I have cherry picked the commits, shifted to the common module into the right test class where is mock already and finally squashed together in one. I have only shifted the changes to the right module. The work from @imonteroperez is here, nothing is lost, really! The Javadoc, both unit tests from @imonteroperez. I added third.

yes but the problem is you didn't preserve the author id and so @imonteroperez doesn't get the credit he deserve!

Tibor17 · 2022-03-04T15:36:13Z

@olamy
I don't have time to fight with someone about words. I told the guys that it should be shifted to the right module but you won't understand it and you are basically a speaker of your colleague @imonteroperez . You both were informed.
Everybody is happy if I take the commit and simplify his time, so pls appreciate it!

olamy · 2022-03-04T23:06:28Z

@olamy I don't have time to fight with someone about words. I told the guys that it should be shifted to the right module but you won't understand it and you are basically a speaker of your colleague @imonteroperez . You both were informed. Everybody is happy if I take the commit and simplify his time, so pls appreciate it!

please try to understand, there are potentially 2 problems here:

as you completely remove the original author from the git history, it's social/community issue. We should respect our contributors and give them credit.
if you used commits from someone else. The author should be part of the history somewhere.
This is breaking IP history. it's a legal problem!

Tibor17 · 2022-03-07T16:11:25Z

@olamy
I can repeat myself.
The implementation of tests that @imonteroperez was made in a wrong module. Many contributors have made the unit tests in right module and @olamy as a speaker of @imonteroperez requires opposite which was never seen before and this attitude postpones the release. I made everything with Slawomir and we need to understand @olamy that we need to reach the consensus. Thx @olamy for your understanding!!!

imonteroperez · 2022-03-07T16:53:02Z

Sorry for being late in this discussion. Not sure to follow your concerns/ statements @Tibor17

The implementation of tests that @imonteroperez was made in a wrong module. Many contributors have made the unit tests in right module

I asked about it on #460 (comment) and the only answer I got was #460 (comment). I assumed based on your latest comment that everything was ok from your side and you will cherry-pick my changes here (in order to prevent who is the author of the changes)

@olamy as a speaker of @imonteroperez requires opposite which was never seen before and this attitude postpones the release

No idea about previous releases or discussions around this topic (or whatever) BUT (1) @olamy is not my speaker (2) I would say exactly the same that he said in terms of respect contributors credit in whatever project.

As mentioned in #440 (comment), I'm confused about this situation.

I made everything with Slawomir and we need to understand @olamy that we need to reach the consensus

Yes, it seems you took my code from SurefirePluginTest 1b721a9 and moved it inside a different test/module. It is ok, BUT I would prefer to get an answer when I asked about it as mentioned before.

Given said that, please feel free to do whatever you want with this. I will let you decide.

Tibor17 · 2022-03-07T17:49:49Z

@imonteroperez
Thank You for your reply, I am very glad. I want to cut two release versions 3.0.0-M5 and 2.22.3, including your feature request to make the users happy.

There was 6 days between two messages #460 (comment) and #460 (comment) and with the like 👍 so it was a signal for me that we can shift the changes to the target module immediately.

@imonteroperez
Thank you for contributing. Glad to see you!

imonteroperez · 2022-03-08T08:26:24Z

@imonteroperez Thank You for your reply, I am very glad. I want to cut two release versions 3.0.0-M5 and 2.22.3, including your feature request to make the users happy.

There was 6 days between two messages #460 (comment) and #460 (comment) and with the like +1 so it was a signal for me that we can shift the changes to the target module immediately.

@imonteroperez Thank you for contributing. Glad to see you!

Now this new feature has been merged, I want to drop here my last comment and thoughts about what happened here:

First of all, thank you all for your support and help in making this happen. The final main goal here is to deliver a new feature to users to make it feasible to perform method filtering on excludesFile and includesFile which is great.
Although main goal was reached, FMPOV as newbie contributor in the plugin I would say I'm still shocked by how things went in terms of keeping author credit, and for sure, this is totally outside what my brain model says about contributing to OSS projects is about. In addition, it makes me more sad to see that comments like this are totally ignored. I did not expect this behaviour here TBH.

Given said that, please fix this internal situation as soon as possible. This is a bad symptom for Apache community health and contribution culture.

jira-importer · 2025-07-04T05:23:07Z

Resolve #2883

Tibor17 changed the title ~~Included Excluded Files as patterns~~ [SUREFIRE-1964] Support for method filtering on excludesFile and includesFile Jan 13, 2022

Tibor17 force-pushed the inc-exc-files branch 2 times, most recently from 6f0fe45 to be7a443 Compare January 13, 2022 01:25

Tibor17 requested a review from slawekjaranowski January 13, 2022 01:44

Tibor17 mentioned this pull request Jan 13, 2022

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

Closed

8 tasks

slawekjaranowski reviewed Jan 13, 2022

View reviewed changes

Tibor17 force-pushed the inc-exc-files branch from 1a3cf0e to 768bb15 Compare January 19, 2022 21:24

Tibor17 requested a review from eolivelli January 20, 2022 00:06

Tibor17 mentioned this pull request Jan 27, 2022

Enabling Surefire to run test classes and test methods in any order specified by a new runOrder #348

Open

olamy approved these changes Feb 5, 2022

View reviewed changes

Tibor17 force-pushed the inc-exc-files branch from 768bb15 to 505d862 Compare February 5, 2022 15:35

Tibor17 assigned imonteroperez Feb 6, 2022

imonteroperez mentioned this pull request Feb 21, 2022

Add some javadoc and unit tests to includesFile and excludesFile #460

Closed

8 tasks

olamy added the enhancement New feature or request label Feb 28, 2022

asfgit force-pushed the inc-exc-files branch from 9c7c0d7 to f2db6ba Compare March 2, 2022 20:25

[SUREFIRE-1964] Support for method filtering on excludesFile and incl…

8add83a

…udesFile

asfgit force-pushed the inc-exc-files branch from f2db6ba to 8add83a Compare March 2, 2022 21:09

olamy closed this Mar 4, 2022

olamy reopened this Mar 4, 2022

Tibor17 merged commit 342ff2b into master Mar 7, 2022

asfgit deleted the inc-exc-files branch March 27, 2022 23:14

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

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

Uh oh!

Conversation

Tibor17 commented Jan 12, 2022

Uh oh!

Tibor17 commented Jan 13, 2022

Uh oh!

Tibor17 commented Jan 13, 2022

Uh oh!

slawekjaranowski left a comment

Choose a reason for hiding this comment

Uh oh!

slawekjaranowski Jan 13, 2022

Choose a reason for hiding this comment

Uh oh!

Tibor17 Jan 14, 2022

Choose a reason for hiding this comment

Uh oh!

imonteroperez commented Jan 14, 2022

Uh oh!

Tibor17 commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imonteroperez commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tibor17 commented Jan 26, 2022

Uh oh!

olamy left a comment

Choose a reason for hiding this comment

Uh oh!

Tibor17 commented Feb 6, 2022

Uh oh!

imonteroperez commented Feb 8, 2022

Uh oh!

Tibor17 commented Mar 2, 2022

Uh oh!

olamy commented Mar 3, 2022

Uh oh!

imonteroperez commented Mar 3, 2022

Uh oh!

olamy commented Mar 3, 2022

Uh oh!

imonteroperez commented Mar 3, 2022

Uh oh!

olamy commented Mar 3, 2022

Uh oh!

Tibor17 commented Mar 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olamy commented Mar 4, 2022

Uh oh!

Tibor17 commented Mar 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olamy commented Mar 4, 2022

Uh oh!

Tibor17 commented Mar 7, 2022

Uh oh!

imonteroperez commented Mar 7, 2022

Uh oh!

Tibor17 commented Mar 7, 2022

Uh oh!

imonteroperez commented Mar 8, 2022

Uh oh!

jira-importer commented Jul 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Tibor17 commented Jan 19, 2022 •

edited

Loading

imonteroperez commented Jan 26, 2022 •

edited

Loading

Tibor17 commented Mar 4, 2022 •

edited

Loading

Tibor17 commented Mar 4, 2022 •

edited

Loading