Remove TestListResolver#optionallyWildcardFilter #499

slawekjaranowski · 2022-03-25T07:59:17Z

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.

slawekjaranowski · 2022-03-25T08:00:13Z

@Tibor17

build pass
code is more readable for me
after review issue will be created

Tibor17 · 2022-03-25T13:47:05Z

surefire-api/src/test/java/org/apache/maven/surefire/api/testset/TestListResolverTest.java

@@ -267,13 +270,16 @@ public void testShouldRunAny()
    public void testClassFilter()
    {
        TestListResolver resolver = new TestListResolver( "#test" );
-        assertTrue( resolver.shouldRun( "pkg/MyTest.class", null ) );
+        assertTrue( resolver.shouldRun( "pkg/MyTest.class", "test" ) );


Don't change the assertions.
It makes whole PR not trusty.

try:

resolver = new TestListResolver( "#abc" ); assertTrue( resolver.shouldRun( "pkg/MyTest.class", null ) ); resolver = new TestListResolver( "#test" ); assertTrue( resolver.shouldRun( "pkg/Abc.class", null ) );

what we verify here?

It means that whatever class should run.
This verifies ParentRunner in JUnit4.

If you type resolver.shouldRun( "pkg/Abc.class", "otherMethod" ), it returns false because it verifies the child of ParentRunner. Methods do not match * = Abc but abc !=otherMethod

If shouldRun has null method then we compare classes => ParentRunner, and in this case * matches Abc => true
If shouldRun has non-null method, we evaluate test method.

Tibor17 · 2022-03-25T13:49:26Z

surefire-api/src/test/java/org/apache/maven/surefire/api/testset/TestListResolverTest.java

@@ -401,19 +406,6 @@ public void testInclusiveWithDefaultExclusivePattern()
        assertTrue( runnable );
    }

-    public void testWildcard()


This use case is still valid and the test should not be removed.

but I removed ... #optionallyWildcardFilter so what I should check?

@slawekjaranowski
This test would be too simple if we have rewrite optionallyWildcardFilter to **/*.class.

...common-junit48/src/test/java/org/apache/maven/surefire/common/junit48/FilterFactoryTest.java

Tibor17 · 2022-03-25T14:10:00Z

surefire-api/src/main/java/org/apache/maven/surefire/api/testset/TestListResolver.java

-        return EMPTY;
-    }
-
-    public final boolean isWildcard()


This is still a valid case.
If TLR is empty (nothing) or a wildcard (everything), the provider would ignore TLR. But both corner cases may happen in the config.

only one case is true for isWildcard:

tlr = new TestListResolver( "*.class" ); assertTrue( tlr.isWildcard() );

Tibor17 · 2022-03-25T14:12:29Z

...providers/surefire-junit4/src/main/java/org/apache/maven/surefire/junit4/JUnit4Provider.java

@@ -432,7 +430,6 @@ private static boolean hasFilteredOutAllChildren( Description description )

    private Filter createMethodFilter()
    {
-        TestListResolver methodFilter = optionallyWildcardFilter( testResolver );
-        return methodFilter.isEmpty() || methodFilter.isWildcard() ? null : new TestResolverFilter( methodFilter );
+        return testResolver != null && testResolver.hasMethodPatterns() ? new TestResolverFilter( testResolver ) : null;


We are missing methodFilter.isWildcard() in this PR.

Wildcard class and wildcard method (i.e. *#*, or **/*#*) returns true from hasMethodPatterns()? If it is any wildcard, even this special case, the filter should be returned as null.

I removed isWildcard also ...

Test for master code:

tlr = new TestListResolver( "*.class" ); assertTrue( tlr.isWildcard() ); assertFalse( tlr.hasMethodPatterns() ); tlr = new TestListResolver( "*#*" ); assertFalse( tlr.isWildcard() ); assertTrue( tlr.hasMethodPatterns() ); tlr = new TestListResolver( "**/*#*" ); assertFalse( tlr.isWildcard() ); assertTrue( tlr.hasMethodPatterns() );

isWildicard() return true only for *.class

In expresion:

TestListResolver methodFilter = optionallyWildcardFilter( testResolver ); return methodFilter.isEmpty() || methodFilter.isWildcard() ? null : new TestResolverFilter( methodFilter );

methodFilter.isEmpty() always return false

methodFilter.isWildcard() != methodFilter.hasMethodPatterns() is always true

@slawekjaranowski
I know what you mean but the problem is that the expression
methodFilter.isWildcard() != methodFilter.hasMethodPatterns()
talks about implementation behavior.
The whole problem is that we adapt the tests for implementation behavior to pass because we trust the current behavior more than the tests.
Maybe the wildcard was not very properly implemented. Maybe it was, I do not remember, but if we remove some test or method we should compensate it with such strong tests and assertions where we would prove your logical expression. If hasMethodPatterns() is enough, why do not you write unit test asserting isWildcard() != hasMethodPatterns() for wildcards. That's the reason why I started the discussion.
The filter is useless and should be null or should not be used for wildcards and empty method patterns.
Why should we slow down the JUnit or TestNG with wildcard filters or why should we let the filtering mechanism to open some unknown bug?

tlr = new TestListResolver( "*#*" ); assertFalse( tlr.isWildcard() ); assertTrue( tlr.hasMethodPatterns() );

Tibor17 · 2022-04-19T13:28:02Z

Hi @slawekjaranowski ,

I will have time today late evening.

slawekjaranowski · 2022-05-04T22:04:19Z

Kindly reminder @Tibor17 or someone else ... 😄

slawekjaranowski requested a review from Tibor17 March 25, 2022 07:59

slawekjaranowski mentioned this pull request Mar 25, 2022

[SUREFIRE-2040] No tests executed with junit-platform-suite and -Dtest=TestSuite #494

Merged

8 tasks

Tibor17 reviewed Mar 25, 2022

View reviewed changes

...common-junit48/src/test/java/org/apache/maven/surefire/common/junit48/FilterFactoryTest.java Show resolved Hide resolved

Tibor17 reviewed Mar 25, 2022

View reviewed changes

slawekjaranowski force-pushed the rm-optionally-wildcard branch from d9d914e to b1315fe Compare April 4, 2022 19:37

Remove TestListResolver#optionallyWildcardFilter

d27a848

slawekjaranowski force-pushed the rm-optionally-wildcard branch from b1315fe to d27a848 Compare April 4, 2022 19:40

slawekjaranowski requested a review from Tibor17 April 19, 2022 11:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove TestListResolver#optionallyWildcardFilter #499

Remove TestListResolver#optionallyWildcardFilter #499

slawekjaranowski commented Mar 25, 2022

slawekjaranowski commented Mar 25, 2022

Tibor17 Mar 25, 2022 •

edited

slawekjaranowski Mar 25, 2022

Tibor17 Mar 25, 2022

Tibor17 Mar 25, 2022

slawekjaranowski Mar 25, 2022

Tibor17 Mar 28, 2022

Tibor17 Mar 25, 2022

slawekjaranowski Mar 25, 2022

Tibor17 Mar 25, 2022

Tibor17 Mar 25, 2022 •

edited

slawekjaranowski Mar 25, 2022

slawekjaranowski Mar 25, 2022

Tibor17 Mar 28, 2022

Tibor17 commented Apr 19, 2022

slawekjaranowski commented May 4, 2022

Remove TestListResolver#optionallyWildcardFilter #499

Are you sure you want to change the base?

Remove TestListResolver#optionallyWildcardFilter #499

Conversation

slawekjaranowski commented Mar 25, 2022

slawekjaranowski commented Mar 25, 2022

Tibor17 Mar 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tibor17 Mar 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tibor17 commented Apr 19, 2022

slawekjaranowski commented May 4, 2022

Tibor17 Mar 25, 2022 •

edited

Tibor17 Mar 25, 2022 •

edited