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

Jump to bottom

[SUREFIRE-2037] Allow for surefire-junit-platform's TestMethodFilter … #487

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[SUREFIRE-2037] Allow for surefire-junit-platform's TestMethodFilter … #487

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
eee427d
[SUREFIRE-2037] Allow for surefire-junit-platform's TestMethodFilter …
crizzis Mar 10, 2022
2ed1d7a
[SUREFIRE-2037] Fixed javadoc
crizzis Mar 10, 2022
b015bef
Fixed SPI impl visibility
crizzis Mar 10, 2022
318467a
[SUREFIRE-2037] Fixed javadoc
crizzis Mar 10, 2022
8c2dac0
[SUREFIRE-2037] Fixed formatting
crizzis Mar 11, 2022
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
5 changes: 2 additions & 3 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
~ KIND, either express or implied. See the License for the
~ specific language governing permissions and limitations
~ under the License.
~ under the License.
-->

<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
Expand Down Expand Up @@ -532,8 +532,7 @@
<exclude>src/test/resources/**/*</exclude>
<exclude>src/test/resources/**/*.css</exclude>
<exclude>**/*.jj</exclude>
<exclude>src/main/resources/META-INF/services/org.apache.maven.surefire.api.provider.SurefireProvider
</exclude>
<exclude>src/main/resources/META-INF/services/**</exclude>
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't change this POM, it has nothing to do with what you are aiming for. Rather do not make any additional refactoring because it is very hard to separate reasonable and non-reasonable changes. I guess you wanted to make some specific improvement in JUnit5 provider. Do not press CTRL+L because there is again unnecessary changes.

Copy link
Author

@crizzis crizzis Mar 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure I follow.

This was an intentional change, otherwise Rat would expect a license inside META-INF/services/org.apache.maven.surefire.junitplatform.TestSelectorFactory

Should I add an entry here for that specific file instead?

<exclude>DEPENDENCIES</exclude>
<exclude>.m2/**</exclude>
<exclude>.m2</exclude>
Expand Down
12 changes: 6 additions & 6 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 @@ -195,13 +195,13 @@ public boolean shouldRun( Class<?> testClass, String methodName )
/**
* Returns {@code true} if satisfies {@code testClassFile} and {@code methodName} filter.
*
* @param testClassFile format must be e.g. "my/package/MyTest.class" including class extension; or null
* @param methodName real test-method name; or null
* @param containerName format must be e.g. "my/package/MyTest.class" including class extension; or null
Copy link
Contributor

@Tibor17 Tibor17 Mar 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Don't change this class, it has nothing to do with what you are aiming for. Rather do not make any additional refactoring because it is very hard to separate reasonable and non-reasonable changes. I guess you wanted to make some specific improvement in JUnit5 provider. Do not press CTRL+ALT+L because there is again unnecessary changes.

Copy link
Author

@crizzis crizzis Mar 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The parameter names of this method have been changed to reflect their new meaning. The second parameter to this method, for instance, will no longer only ever represent method names. That's the entire purpose of this PR.

Would you care to explain why you would expect the parameters to retain their original names?

* @param selectorName real test-method name; or null
*/
@Override
public boolean shouldRun( String testClassFile, String methodName )
public boolean shouldRun( String containerName, String selectorName )
{
if ( isEmpty() || isBlank( testClassFile ) && isBlank( methodName ) )
if ( isEmpty() || isBlank( containerName ) && isBlank( selectorName ) )
{
return true;
}
Expand All @@ -217,7 +217,7 @@ public boolean shouldRun( String testClassFile, String methodName )
{
for ( ResolvedTest filter : getIncludedPatterns() )
{
if ( filter.matchAsInclusive( testClassFile, methodName ) )
if ( filter.matchAsInclusive( containerName, selectorName ) )
{
shouldRun = true;
break;
Expand All @@ -229,7 +229,7 @@ public boolean shouldRun( String testClassFile, String methodName )
{
for ( ResolvedTest filter : getExcludedPatterns() )
{
if ( filter.matchAsExclusive( testClassFile, methodName ) )
if ( filter.matchAsExclusive( containerName, selectorName ) )
{
shouldRun = false;
break;
Expand Down
41 changes: 41 additions & 0 deletions surefire-providers/surefire-junit-platform/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,47 @@
</includes>
</configuration>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-jar-plugin</artifactId>
<executions>
<execution>
<id>api-jar</id>
<phase>package</phase>
<goals>
<goal>jar</goal>
</goals>
<configuration>
<classifier>api</classifier>
Copy link
Author

Choose a reason for hiding this comment

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

Wasn't sure whether to classify this JAR with api or spi.

In any case, this is simply a slim version of the JAR for service providers who otherwise do not depend on surefire-junit-platform

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a hack. Why you have introduced Apache SPI and you change the default behavior this way? JUnit5 SPI do not exist with same purpose? The plugin has configuration of class/methods. If somebody changes this in the SPI, then what you be valid? Configuration or SPI? Pls describe very properly what you are aiming for in the JIRA in details.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are other two SPI modules surefire-extensions-spi (for the forked JVM) and surefire-extensions-api (for the plugin process) but nothing is suited for this scenario of providers.
Can you explain how the customer would implement the interface TestSelectorFactory you have introduced? What types of sources can be filtered and how they can be configured?

Copy link
Author

@crizzis crizzis Mar 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Can you explain how the customer would implement the interface TestSelectorFactory you have introduced? What types of sources can be filtered and how they can be configured?

Absolutely.

Here is my implementation for ArchUnit's FieldSource:

package com.tngtech.archunit.junit.surefire;

import com.tngtech.archunit.junit.FieldSource;
import org.apache.maven.surefire.junitplatform.TestSelectorFactory;
import org.junit.platform.engine.TestSource;

public class FieldSelectorFactory implements TestSelectorFactory {
    @Override
    public boolean supports(TestSource testSource) {
        return testSource instanceof FieldSource;
    }

    @Override
    public String getContainerName(TestSource testSource) {
        return ((FieldSource) testSource).getClassName();
    }

    @Override
    public String getSelectorName(TestSource testSource) {
        return ((FieldSource) testSource).getFieldName();
    }
}

It solves the problem of adding support for applying the same filtering logic Surefire now applies to methods, to other test sources (FieldSource in this case).

In case you're unfamiliar with ArchUnit, here's what an ArchUnit test might look like:

@AnalyzeClasses(packages = "com.tngtech.archunit.example.layers")
public class CodingRulesTest {
    
    @ArchTest
    private final ArchRule loggers_should_be_private_static_final =
            fields().that().haveRawType(Logger.class)
                    .should().bePrivate()
                    .andShould().beStatic()
                    .andShould().beFinal()
                    .because("we agreed on this convention");
}

As you can see, test cases are represented by fields, and not methods. This is also why the default Surefire include / exclude filters do not work.

JUnit5 SPI do not exist with same purpose?

Yes, of course it exists, and that's exactly what ArchUnit does under the hood. There's no way to influence Surefire filtering logic from inside a JUnit extension, though.

I've already verified providing this implementation of the new SPI in ArchUnit solves the linked ArchUnit issue for Surefire. I'm now about to suggest similar extensibility to other build tools.

Copy link
Author

@crizzis crizzis Mar 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Also, if I understand correctly, here:

The plugin has configuration of class/methods. If somebody changes this in the SPI, then what you be valid? Configuration or SPI?

You're asking what happens if someone overwrites the service to something else than MethodSelectorFactory. The answer is here:

private Collection<TestSelectorFactory> loadSelectorFactories()
    {
        return StreamSupport.stream( ServiceLoader.load( TestSelectorFactory.class ).spliterator(), false )
            .collect( Collectors.toSet() );
    }

Basically, there's no 'changing the SPI'. MethodSelectorFactory is always provided as the default implementation, and extending libraries can only add new service implementations. MethodSelectorFactory cannot be resigned from, though.

Thus, for those users who do not extend this SPI in any way, the behavior of Surefire will be consistent with the behavior before this PR.

There are other two SPI modules surefire-extensions-spi (for the forked JVM) and surefire-extensions-api (for the plugin process)

Yes, and unfortunately, they do not fit my use case. I could implement a custom SurefireProvider, but that would basically mean re-implementing the current JUnitPlatformProvider, which is 99% suitable but completely closed for extension. This minuscule SPI introduction is really the smallest possible change that solves the problem without affecting other plugin users.

<includes>
<include>org/apache/maven/surefire/junitplatform/TestSelectorFactory.class</include>
</includes>
</configuration>
</execution>
</executions>
</plugin>
<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>build-helper-maven-plugin</artifactId>
<version>3.2.0</version>
<executions>
<execution>
<id>api-jar</id>
<goals>
<goal>attach-artifact</goal>
</goals>
<configuration>
<artifacts>
<artifact>
<classifier>api</classifier>
<type>jar</type>
<file>${project.build.directory}/${project.artifactId}-${project.version}-api.jar</file>
</artifact>
</artifacts>
</configuration>
</execution>
</executions>
</plugin>
</plugins>
</build>
</project>
148 changes: 70 additions & 78 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 @@ -19,37 +19,6 @@
* under the License.
*/

import static java.util.Arrays.stream;
import static java.util.Collections.emptyMap;
import static java.util.Optional.empty;
import static java.util.Optional.of;
import static java.util.logging.Level.WARNING;
import static java.util.stream.Collectors.toList;
import static org.apache.maven.surefire.api.booter.ProviderParameterNames.EXCLUDE_JUNIT5_ENGINES_PROP;
import static org.apache.maven.surefire.api.booter.ProviderParameterNames.INCLUDE_JUNIT5_ENGINES_PROP;
import static org.apache.maven.surefire.api.booter.ProviderParameterNames.TESTNG_EXCLUDEDGROUPS_PROP;
import static org.apache.maven.surefire.api.booter.ProviderParameterNames.TESTNG_GROUPS_PROP;
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.util.TestsToRun.fromClass;
import static org.apache.maven.surefire.shared.utils.StringUtils.isBlank;
import static org.junit.platform.engine.discovery.DiscoverySelectors.selectClass;
import static org.junit.platform.engine.discovery.DiscoverySelectors.selectUniqueId;
import static org.junit.platform.launcher.core.LauncherDiscoveryRequestBuilder.request;

import java.io.IOException;
import java.io.StringReader;
import java.io.UncheckedIOException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Properties;
import java.util.logging.Logger;

import org.apache.maven.surefire.api.provider.AbstractProvider;
import org.apache.maven.surefire.api.provider.ProviderParameters;
import org.apache.maven.surefire.api.report.ReporterException;
Expand All @@ -70,13 +39,47 @@
import org.junit.platform.launcher.TestIdentifier;
import org.junit.platform.launcher.core.LauncherDiscoveryRequestBuilder;

import java.io.IOException;
import java.io.StringReader;
import java.io.UncheckedIOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Properties;
import java.util.ServiceLoader;
import java.util.logging.Logger;
import java.util.stream.Collectors;
import java.util.stream.StreamSupport;

import static java.util.Arrays.stream;
import static java.util.Collections.emptyMap;
import static java.util.Optional.empty;
import static java.util.Optional.of;
import static java.util.logging.Level.WARNING;
import static java.util.stream.Collectors.toList;
import static org.apache.maven.surefire.api.booter.ProviderParameterNames.EXCLUDE_JUNIT5_ENGINES_PROP;
import static org.apache.maven.surefire.api.booter.ProviderParameterNames.INCLUDE_JUNIT5_ENGINES_PROP;
import static org.apache.maven.surefire.api.booter.ProviderParameterNames.TESTNG_EXCLUDEDGROUPS_PROP;
import static org.apache.maven.surefire.api.booter.ProviderParameterNames.TESTNG_GROUPS_PROP;
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.util.TestsToRun.fromClass;
import static org.apache.maven.surefire.shared.utils.StringUtils.isBlank;
import static org.junit.platform.engine.discovery.DiscoverySelectors.selectClass;
import static org.junit.platform.engine.discovery.DiscoverySelectors.selectUniqueId;
import static org.junit.platform.launcher.core.LauncherDiscoveryRequestBuilder.request;

/**
* JUnit 5 Platform Provider.
*
* @since 2.22.0
*/
public class JUnitPlatformProvider
extends AbstractProvider
public class JUnitPlatformProvider extends AbstractProvider
Copy link
Contributor

Choose a reason for hiding this comment

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

It is very hard to separate reasonable and non-reasonable changes. I guess you wanted to make some specific improvement in this class. Do not press CTRL+ALT+L because there is again unnecessary changes.

Copy link
Author

@crizzis crizzis Mar 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I've reformatted this class according to maven_checks after getting checkstyle violations.

Let me try to revert unrelated changes

EDIT Done, see updated code

{
static final String CONFIGURATION_PARAMETERS = "configurationParameters";

Expand Down Expand Up @@ -105,7 +108,7 @@ public JUnitPlatformProvider( ProviderParameters parameters )
public Iterable<Class<?>> getSuites()
{
try
{
{
return scanClasspath();
}
finally
Expand All @@ -115,8 +118,7 @@ public Iterable<Class<?>> getSuites()
}

@Override
public RunResult invoke( Object forkTestSet )
throws TestSetFailedException, ReporterException
public RunResult invoke( Object forkTestSet ) throws TestSetFailedException, ReporterException
{
ReporterFactory reporterFactory = parameters.getReporterFactory();
final RunResult runResult;
Expand All @@ -132,16 +134,15 @@ public RunResult invoke( Object forkTestSet )
}
else if ( forkTestSet instanceof Class )
{
invokeAllTests( fromClass( ( Class<?> ) forkTestSet ), adapter );
invokeAllTests( fromClass( (Class<?>) forkTestSet ), adapter );
}
else if ( forkTestSet == null )
{
invokeAllTests( scanClasspath(), adapter );
}
else
{
throw new IllegalArgumentException(
"Unexpected value of forkTestSet: " + forkTestSet );
throw new IllegalArgumentException( "Unexpected value of forkTestSet: " + forkTestSet );
}
}
finally
Expand Down Expand Up @@ -189,7 +190,7 @@ private void invokeAllTests( TestsToRun testsToRun, RunListenerAdapter adapter )
{
// Replace the "discoveryRequest" so that it only specifies the failing tests
LauncherDiscoveryRequest discoveryRequest =
buildLauncherDiscoveryRequestForRerunFailures( adapter );
buildLauncherDiscoveryRequestForRerunFailures( adapter );
// Reset adapter's recorded failures and invoke the failed tests again
adapter.reset();
launcher.execute( discoveryRequest, adapter );
Expand All @@ -212,37 +213,32 @@ private void execute( TestsToRun testsToRun, RunListenerAdapter adapter )
if ( testsToRun.allowEagerReading() )
{
List<DiscoverySelector> selectors = new ArrayList<>();
testsToRun.iterator()
.forEachRemaining( c -> selectors.add( selectClass( c.getName() ) ) );
testsToRun.iterator().forEachRemaining( c -> selectors.add( selectClass( c.getName() ) ) );

LauncherDiscoveryRequestBuilder builder = request()
.filters( filters )
.configurationParameters( configurationParameters )
.selectors( selectors );
LauncherDiscoveryRequestBuilder builder =
request().filters( filters ).configurationParameters( configurationParameters ).selectors( selectors );

launcher.execute( builder.build(), adapter );
}
else
{
testsToRun.iterator()
.forEachRemaining( c ->
{
LauncherDiscoveryRequestBuilder builder = request()
.filters( filters )
.configurationParameters( configurationParameters )
testsToRun.iterator().forEachRemaining( c ->
{
LauncherDiscoveryRequestBuilder builder =
request().filters( filters ).configurationParameters( configurationParameters )
.selectors( selectClass( c.getName() ) );
launcher.execute( builder.build(), adapter );
} );
launcher.execute( builder.build(), adapter );
} );
}
}

private void closeLauncher()
{
if ( launcher instanceof AutoCloseable )
{
try
{
( (AutoCloseable) launcher ).close();
( ( AutoCloseable ) launcher ).close();
}
catch ( Exception e )
{
Expand All @@ -253,8 +249,8 @@ private void closeLauncher()

private LauncherDiscoveryRequest buildLauncherDiscoveryRequestForRerunFailures( RunListenerAdapter adapter )
{
LauncherDiscoveryRequestBuilder builder = request().filters( filters ).configurationParameters(
configurationParameters );
LauncherDiscoveryRequestBuilder builder =
request().filters( filters ).configurationParameters( configurationParameters );
// Iterate over recorded failures
for ( TestIdentifier identifier : new LinkedHashSet<>( adapter.getFailures().keySet() ) )
{
Expand All @@ -263,33 +259,31 @@ private LauncherDiscoveryRequest buildLauncherDiscoveryRequestForRerunFailures(
return builder.build();
}

private Collection<TestSelectorFactory> loadSelectorFactories()
{
return StreamSupport.stream( ServiceLoader.load( TestSelectorFactory.class ).spliterator(), false )
.collect( Collectors.toSet() );
}

private Filter<?>[] newFilters()
{
List<Filter<?>> filters = new ArrayList<>();

getPropertiesList( TESTNG_GROUPS_PROP )
.map( TagFilter::includeTags )
.ifPresent( filters::add );
getPropertiesList( TESTNG_GROUPS_PROP ).map( TagFilter::includeTags ).ifPresent( filters::add );

getPropertiesList( TESTNG_EXCLUDEDGROUPS_PROP )
.map( TagFilter::excludeTags )
.ifPresent( filters::add );
getPropertiesList( TESTNG_EXCLUDEDGROUPS_PROP ).map( TagFilter::excludeTags ).ifPresent( filters::add );

TestListResolver testListResolver = parameters.getTestRequest().getTestListResolver();
if ( !testListResolver.isEmpty() )
{
filters.add( new TestMethodFilter( testListResolver ) );
filters.add( new TestSelectorFilter( testListResolver, loadSelectorFactories() ) );
}

getPropertiesList( INCLUDE_JUNIT5_ENGINES_PROP )
.map( EngineFilter::includeEngines )
.ifPresent( filters::add );
getPropertiesList( INCLUDE_JUNIT5_ENGINES_PROP ).map( EngineFilter::includeEngines ).ifPresent( filters::add );

getPropertiesList( EXCLUDE_JUNIT5_ENGINES_PROP )
.map( EngineFilter::excludeEngines )
.ifPresent( filters::add );
getPropertiesList( EXCLUDE_JUNIT5_ENGINES_PROP ).map( EngineFilter::excludeEngines ).ifPresent( filters::add );

return filters.toArray( new Filter<?>[ filters.size() ] );
return filters.toArray( new Filter<?>[0] );
}

Filter<?>[] getFilters()
Expand All @@ -309,8 +303,7 @@ private Map<String, String> newConfigurationParameters()
Map<String, String> result = new HashMap<>();
Properties props = new Properties();
props.load( reader );
props.stringPropertyNames()
.forEach( key -> result.put( key, props.getProperty( key ) ) );
props.stringPropertyNames().forEach( key -> result.put( key, props.getProperty( key ) ) );
return result;
}
catch ( IOException e )
Expand All @@ -328,9 +321,8 @@ private Optional<List<String>> getPropertiesList( String key )
{
String property = parameters.getProviderProperties().get( key );
return isBlank( property ) ? empty()
: of( stream( property.split( "[,]+" ) )
.filter( StringUtils::isNotBlank )
.map( String::trim )
.collect( toList() ) );
: of( stream( property.split( "[,]+" ) )
.filter( StringUtils::isNotBlank )
.map( String::trim ).collect( toList() ) );
}
}
Loading