Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove TestListResolver#optionallyWildcardFilter #499

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove TestListResolver#optionallyWildcardFilter #499

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 1 addition & 26 deletions surefire-api/src/main/java/org/apache/maven/surefire/api/testset/TestListResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,6 @@ public class TestListResolver
{
private static final String JAVA_CLASS_FILE_EXTENSION = ".class";

private static final TestListResolver WILDCARD = new TestListResolver( "*" + JAVA_CLASS_FILE_EXTENSION );

private static final TestListResolver EMPTY = new TestListResolver( "" );

private final Set<ResolvedTest> includedPatterns;

private final Set<ResolvedTest> excludedPatterns;
Expand Down Expand Up @@ -135,27 +131,6 @@ public boolean hasMethodPatterns()
return hasIncludedMethodPatterns() || hasExcludedMethodPatterns();
}

/**
*
* @param resolver filter possibly having method patterns
* @return {@code resolver} if {@link TestListResolver#hasMethodPatterns() resolver.hasMethodPatterns()}
* returns {@code true}; Otherwise wildcard filter {@code *.class} is returned.
*/
public static TestListResolver optionallyWildcardFilter( TestListResolver resolver )
{
return resolver.hasMethodPatterns() ? resolver : WILDCARD;
}

public static TestListResolver getEmptyTestListResolver()
{
return EMPTY;
}

public final boolean isWildcard()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only one case is true for isWildcard:

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

{
return equals( WILDCARD );
}

public TestFilter<String, String> and( final TestListResolver another )
{
return new TestFilter<String, String>()
Expand Down Expand Up @@ -238,7 +213,7 @@ public boolean shouldRun( String testClassFile, String methodName )
@Override
public boolean isEmpty()
{
return equals( EMPTY );
return includedPatterns.isEmpty() && excludedPatterns.isEmpty();
}

@Override
Expand Down
26 changes: 6 additions & 20 deletions surefire-api/src/test/java/org/apache/maven/surefire/api/testset/TestListResolverTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@
* under the License.
*/

import junit.framework.TestCase;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
Expand All @@ -29,12 +27,14 @@
import java.util.LinkedHashSet;
import java.util.Set;

import static java.util.Collections.addAll;
import static org.apache.maven.surefire.api.testset.TestListResolver.newTestListResolver;
import static org.apache.maven.surefire.api.testset.ResolvedTest.Type.CLASS;
import junit.framework.TestCase;

import static java.util.Arrays.asList;
import static java.util.Collections.addAll;
import static java.util.Collections.emptySet;
import static java.util.Collections.singleton;
import static org.apache.maven.surefire.api.testset.ResolvedTest.Type.CLASS;
import static org.apache.maven.surefire.api.testset.TestListResolver.newTestListResolver;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;

Expand Down Expand Up @@ -257,7 +257,7 @@ public void testShouldRunSuiteWithIncludedMethods()

public void testShouldRunAny()
{
TestListResolver resolver = TestListResolver.getEmptyTestListResolver();
TestListResolver resolver = new TestListResolver( "" );
assertTrue( resolver.shouldRun( "pkg/MyTest.class", null ) );

resolver = new TestListResolver( Collections.<String>emptySet() );
Expand Down Expand Up @@ -365,7 +365,6 @@ public void testTestListResolverWithMethods()
assertTrue( resolver.shouldRun( "BTest.class", null ) );
assertFalse( resolver.shouldRun( "BTest.class", "failedTest" ) );
assertTrue( resolver.shouldRun( "CTest.class", null ) );
assertFalse( TestListResolver.optionallyWildcardFilter( resolver ).isEmpty() );
}

private static Set<ResolvedTest> toSet( ResolvedTest... patterns )
Expand Down Expand Up @@ -401,19 +400,6 @@ public void testInclusiveWithDefaultExclusivePattern()
assertTrue( runnable );
}

public void testWildcard()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
TestListResolver tlr = TestListResolver.optionallyWildcardFilter( new TestListResolver( (String) null ) );
assertThat( tlr, is( new TestListResolver( "**/*.class" ) ) );
assertThat( tlr.isWildcard(), is( true ) );
assertThat( tlr.isEmpty(), is( false ) );

tlr = TestListResolver.optionallyWildcardFilter( new TestListResolver( "**/**/MethodLessPattern.class" ) );
assertThat( tlr, is( new TestListResolver( "**/*.class" ) ) );
assertThat( tlr.isWildcard(), is( true ) );
assertThat( tlr.isEmpty(), is( false ) );
}

public void testRegexRuleViolationQuotedHashMark()
{
try
Expand Down
5 changes: 1 addition & 4 deletions ...mon-junit48/src/test/java/org/apache/maven/surefire/common/junit48/FilterFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -933,8 +933,7 @@ public void testWithJUnitCoreFirstClassAndSingleMethod()
@Test
public void testShouldRunSuite()
{
TestListResolver filter = new TestListResolver( "Su?te" );
filter = TestListResolver.optionallyWildcardFilter( filter );
TestListResolver filter = new TestListResolver( "*.class" );
slawekjaranowski marked this conversation as resolved.
Show resolved Hide resolved
JUnitCore core = new JUnitCore();
Result result = core.run( Request.classes( Suite.class ).filterWith( new MethodFilter( filter ) ) );
assertTrue( result.wasSuccessful() );
Expand All @@ -951,7 +950,6 @@ public void testShouldRunParameterized()
+ "PT#testC*, "
+ "!PT.java#testCY[?],"
+ "%regex[.*.tests.pt.PT.class#w.*|x.*T.*]" );
filter = TestListResolver.optionallyWildcardFilter( filter );
JUnitCore core = new JUnitCore();
Result result = core.run( Request.classes( PT.class ).filterWith( new MethodFilter( filter ) ) );
assertTrue( result.wasSuccessful() );
Expand All @@ -970,7 +968,6 @@ public void testShouldRunParameterizedWithPlusDelimiter()
// x12T35[0], x12T35[1]
TestListResolver filter =
new TestListResolver( "%regex[.*.PT.* # w.*|x(\\d+)T(\\d+)\\[(\\d+)\\]]" );
filter = TestListResolver.optionallyWildcardFilter( filter );
JUnitCore core = new JUnitCore();
Result result = core.run( Request.classes( PT.class ).filterWith( new MethodFilter( filter ) ) );
assertTrue( result.wasSuccessful() );
Expand Down
12 changes: 6 additions & 6 deletions ...platform/src/main/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import static org.apache.maven.surefire.api.report.ConsoleOutputCapture.startCapture;
import static org.apache.maven.surefire.api.report.RunMode.NORMAL_RUN;
import static org.apache.maven.surefire.api.report.RunMode.RERUN_TEST_AFTER_FAILURE;
import static org.apache.maven.surefire.api.testset.TestListResolver.optionallyWildcardFilter;
import static org.apache.maven.surefire.api.util.TestsToRun.fromClass;
import static org.apache.maven.surefire.shared.utils.StringUtils.isBlank;
import static org.junit.platform.engine.discovery.DiscoverySelectors.selectClass;
Expand All @@ -56,6 +55,7 @@
import org.apache.maven.surefire.api.report.ReporterException;
import org.apache.maven.surefire.api.report.ReporterFactory;
import org.apache.maven.surefire.api.suite.RunResult;
import org.apache.maven.surefire.api.testset.TestListResolver;
import org.apache.maven.surefire.api.testset.TestSetFailedException;
import org.apache.maven.surefire.api.util.ScanResult;
import org.apache.maven.surefire.api.util.SurefireReflectionException;
Expand Down Expand Up @@ -275,11 +275,11 @@ private Filter<?>[] newFilters()
.map( TagFilter::excludeTags )
.ifPresent( filters::add );

of( optionallyWildcardFilter( parameters.getTestRequest().getTestListResolver() ) )
.filter( f -> !f.isEmpty() )
.filter( f -> !f.isWildcard() )
.map( TestMethodFilter::new )
.ifPresent( filters::add );
TestListResolver resolver = parameters.getTestRequest().getTestListResolver();
if ( resolver.hasMethodPatterns() )
{
filters.add( new TestMethodFilter( resolver ) );
}

getPropertiesList( INCLUDE_JUNIT5_ENGINES_PROP )
.map( EngineFilter::includeEngines )
Expand Down
3 changes: 0 additions & 3 deletions ...form/src/test/java/org/apache/maven/surefire/junitplatform/JUnitPlatformProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -899,7 +899,6 @@ public void shouldFilterTestMethod()
ProviderParameters providerParameters = providerParametersMock();
TestListResolver testListResolver = new TestListResolver( "**/*Test#test*" );
assertFalse( testListResolver.isEmpty() );
assertFalse( testListResolver.isWildcard() );
TestRequest request = new TestRequest( null, null, testListResolver, 0 );
when( providerParameters.getTestRequest() ).thenReturn( request );

Expand All @@ -926,7 +925,6 @@ public void shouldNotFilterEmpty()
ProviderParameters providerParameters = providerParametersMock();
TestListResolver testListResolver = new TestListResolver( "" );
assertTrue( testListResolver.isEmpty() );
assertFalse( testListResolver.isWildcard() );
TestRequest request = new TestRequest( null, null, testListResolver, 0 );
when( providerParameters.getTestRequest() ).thenReturn( request );

Expand All @@ -941,7 +939,6 @@ public void shouldNotFilterWildcard()
{
ProviderParameters providerParameters = providerParametersMock();
TestListResolver testListResolver = new TestListResolver( "*.java" );
assertTrue( testListResolver.isWildcard() );
assertFalse( testListResolver.isEmpty() );
TestRequest request = new TestRequest( null, null, testListResolver, 0 );
when( providerParameters.getTestRequest() ).thenReturn( request );
Expand Down
7 changes: 2 additions & 5 deletions ...viders/surefire-junit4/src/main/java/org/apache/maven/surefire/junit4/JUnit4Provider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@
import static org.apache.maven.surefire.common.junit4.Notifier.pureNotifier;
import static org.apache.maven.surefire.api.report.ConsoleOutputCapture.startCapture;
import static org.apache.maven.surefire.api.report.SimpleReportEntry.withException;
import static org.apache.maven.surefire.api.testset.TestListResolver.optionallyWildcardFilter;
import static org.apache.maven.surefire.api.util.TestsToRun.fromClass;
import static org.apache.maven.surefire.api.util.internal.ObjectUtils.systemProps;
import static org.junit.runner.Request.aClass;
Expand Down Expand Up @@ -274,14 +273,13 @@ private void executeWithRerun( Class<?> clazz, Notifier notifier, RunModeSetter
{
JUnitTestFailureListener failureListener = new JUnitTestFailureListener();
notifier.addListener( failureListener );
boolean hasMethodFilter = testResolver != null && testResolver.hasMethodPatterns();

try
{
try
{
notifier.asFailFast( isFailFast() );
execute( clazz, notifier, hasMethodFilter ? createMethodFilter() : null );
execute( clazz, notifier, createMethodFilter() );
}
finally
{
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@Tibor17 Tibor17 Mar 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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() );

}
}
11 changes: 4 additions & 7 deletions ...surefire-junit47/src/main/java/org/apache/maven/surefire/junitcore/JUnitCoreProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
import static org.apache.maven.surefire.api.report.ConsoleOutputCapture.startCapture;
import static org.apache.maven.surefire.api.report.RunMode.NORMAL_RUN;
import static org.apache.maven.surefire.api.report.RunMode.RERUN_TEST_AFTER_FAILURE;
import static org.apache.maven.surefire.api.testset.TestListResolver.optionallyWildcardFilter;
import static org.apache.maven.surefire.api.util.TestsToRun.fromClass;
import static org.apache.maven.surefire.common.junit4.JUnit4ProviderUtil.generateFailingTestDescriptions;
import static org.apache.maven.surefire.common.junit4.JUnit4RunListenerFactory.createCustomListeners;
Expand Down Expand Up @@ -276,16 +275,14 @@ private Filter createJUnit48Filter()
final FilterFactory factory = new FilterFactory( testClassLoader );
Map<String, String> props = providerParameters.getProviderProperties();
Filter groupFilter = factory.canCreateGroupFilter( props ) ? factory.createGroupFilter( props ) : null;
TestListResolver methodFilter = optionallyWildcardFilter( testResolver );
boolean onlyGroups = methodFilter.isEmpty() || methodFilter.isWildcard();
if ( onlyGroups )
if ( testResolver.hasMethodPatterns() )
{
return groupFilter;
Filter jUnitMethodFilter = factory.createMethodFilter( testResolver );
return groupFilter == null ? jUnitMethodFilter : factory.and( groupFilter, jUnitMethodFilter );
}
else
{
Filter jUnitMethodFilter = factory.createMethodFilter( methodFilter );
return groupFilter == null ? jUnitMethodFilter : factory.and( groupFilter, jUnitMethodFilter );
return groupFilter;
}
}

Expand Down
12 changes: 5 additions & 7 deletions ...viders/surefire-testng/src/main/java/org/apache/maven/surefire/testng/TestNGProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@

import static org.apache.maven.surefire.api.report.ConsoleOutputCapture.startCapture;
import static org.apache.maven.surefire.api.report.RunMode.NORMAL_RUN;
import static org.apache.maven.surefire.api.testset.TestListResolver.getEmptyTestListResolver;
import static org.apache.maven.surefire.api.testset.TestListResolver.optionallyWildcardFilter;
import static org.apache.maven.surefire.api.util.TestsToRun.fromClass;

/**
Expand Down Expand Up @@ -205,7 +203,7 @@ public void update( Command command )
private TestNGDirectoryTestSuite newDirectorySuite()
{
return new TestNGDirectoryTestSuite( testRequest.getTestSourceDirectory().toString(), providerProperties,
reporterConfiguration.getReportsDirectory(), getTestFilter(),
reporterConfiguration.getReportsDirectory(), getTestMethodFilterFilter(),
mainCliOptions, getSkipAfterFailureCount() );
}

Expand Down Expand Up @@ -247,13 +245,13 @@ private TestsToRun scanClassPath()
private boolean hasSpecificTests()
{
TestListResolver specificTestPatterns = testRequest.getTestListResolver();
return !specificTestPatterns.isEmpty() && !specificTestPatterns.isWildcard();
return !specificTestPatterns.isEmpty();
}

private TestListResolver getTestFilter()
private TestListResolver getTestMethodFilterFilter()
{
TestListResolver filter = optionallyWildcardFilter( testRequest.getTestListResolver() );
return filter.isWildcard() ? getEmptyTestListResolver() : filter;
TestListResolver filter = testRequest.getTestListResolver();
return filter.hasMethodPatterns() ? filter : null;
}

// If we have access to IResultListener, return a ConfigurationAwareTestNGReporter.
Expand Down