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

[crizzis]

…to support custom TestSources

o Non-breaking change, introduces a new SPI and a new output artifact with the SPI interface, resolving test names from custom test sources

Signed-off-by: Krzysztof Sierszeń krzysztof.sierszen@digitalnewagency.com

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.

[crizzis]

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

[Tibor17]

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.

[Tibor17]

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?

[crizzis]

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.

[crizzis]

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.

[Tibor17]

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.

[crizzis]

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?

[Tibor17]

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.

[crizzis]

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?

[Tibor17]

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.

[crizzis]

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