Introduce support for includeTags and excludeTags in the junit-platform-surefire-provider

[smoyer64]

As noted in #454, I've been working on a small change to JUnit 5's SurefireProvider that allows the usage of includeTags and excludeTags within the plugin configuration section of the Maven POM. An example is as follows:

<plugin>
    <artifactId>maven-surefire-plugin</artifactId>
    <version>2.19.1</version>
    <configuration>
        <properties>
            <excludeTags>acceptance, integration</excludeTags>
        </properties>
    </configuration>
    <dependencies>
        <dependency>
            <groupId>org.junit.platform</groupId>
            <artifactId>junit-platform-surefire-provider</artifactId>
            <version>${junit.platform.version}</version>
        </dependency>
    </dependencies>
</plugin>

I'll be submitting a related pull-request to the junit5-samples project that updates the POM and README files in addition to adding a couple of test classes that have tags.

---

I hereby agree to the terms of the JUnit Contributor License Agreement.

[britter]

I think you should be reusing the groups and excludeGroups element. See the Surefire JUnit documentation

[britter]

How about adding some tests to JUnitPlatformProviderTests ?

[sbrannen]

I think you should be reusing the groups and excludeGroups element. See the Surefire JUnit documentation

As I stated in #454 (comment), the Surefire support for the JUnit Platform should align with the Gradle support for the JUnit Platform -- which intentionally aligns with terminology used in the ConsoleLauncher as well as within the Platform itself.

So I don't think using the term groups (even if it existed for JUnit 4 support) is such a good idea. The JUnit Platform provides first-class support for tags. So we shouldn't confuse users by calling them something different only when using Maven Surefire.

Furthermore, the JUnit Platform may potentially support a feature called groups at a later date. That's not foreseeable at the moment; however, we can not preclude the possibility.

[smoyer64]

As I stated in #454 (comment)

I realize that this PR doesn't satisfy the need for includeEngines and excludeEngines ... and it definitely needs tests. Things have been crazy-busy at work so I wanted to at least submit the part that was done (and working in a local project).

[britter]

@sbrannen:

So I don't think using the term groups (even if it existed for JUnit 4 support) is such a good idea. The JUnit Platform provides first-class support for tags. So we shouldn't confuse users by calling them something different only when using Maven Surefire.

Problem with this is, that Maven uses groups and excludedGroups not only for JUnit, but also for TestNG and POJO (see http://maven.apache.org/surefire/maven-surefire-plugin/featurematrix.html). So from their point of view, it would be using the term groups only when using Surefire with JUnit 5.

[sbrannen]

@britter, I thought somebody might come back with that argument. 😉

I can understand that the Maven Surefire team also wishes to maintain a level of consistency with regard to the names of features they support. So if the Maven Surefire team opts in favor of groups over tags for their JUnit Platform support, I will leave that up to them.

And if they do choose groups over tags, I agree that we should name the tag support correctly from the start in order to avoid inconveniencing early adopters of the provider.

[sbrannen]

So from their point of view, it would be using the term groups only when using Surefire with JUnit 5.

It's worth mentioning that referring to JUnit 5 (and potentially implying JUnit Jupiter with that) can quickly lead to confusion. So, for the sake of clarity, let's refer to either the JUnit Platform or JUnit Jupiter accordingly.

Cheers!

[Tibor17]

groups and excludeGroups have the same meaning with tags.
Imaging how confusing it would be to have groups and tags in plugin XSD with the same purpose.

[Tibor17]

I mean if the documentation is written properly with example how the plugin is configured or a recommendation, then JUnit 5 may use tags in JUnit 5 API and Surefire project keep backwards compatibility. WDYT?

[sbrannen]

Well, there will be confusion for certain sets of users no matter which route we go.

So I guess it's really just a choice of the lesser of two evils. 😉

Plus, proper documentation will be necessary anyway.

[smoyer64]

Is there a consensus on includeTags/excludeTags? I guess it's also worth remembering we're eventually going to need includeEngines/excludeEngines configuration properties which won't align with any of the existing maven-surefire-plugin parameters.

[marcphilipp]

I think we should support groups/excludeGroups as a synonym for includeTags/excludeTags for compatibility with Maven Surefire.

Edit: Only one of them should be present. Otherwise we should signal a validation error/throw an exception.

[mmerdes]

@marcphilipp
agreed

[sbrannen]

I think we should support groups/excludeGroups as a synonym for includeTags/excludeTags for compatibility with Maven Surefire.

Edit: Only one of them should be present. Otherwise we should signal a validation error/throw an exception.

I fully agree!

We (and by "we" I mean the entire community) have to keep in mind that the JUnit Platform is much more powerful than JUnit 4 in terms of configurability. And that means there will be multiple configuration options that will have to be supported by build tools and IDEs.

So, while backwards compatibility is for sure "a good thing" (especially for people migrating from JUnit 4 to the JUnit Platform), we simultaneously have to keep an eye on the future.

In other words, the design decisions we make now must be forward thinking in order to avoid having a mess of inconsistent configuration options/styles for the JUnit Platform as it evolves over time.

[marcphilipp]

@smoyer64 Can you do the necessary changes to support groups/excludeGroups as synonyms to includeTags/excludeTags?

[smoyer64]

@marcphilipp Yes, I can take care of this though it may be a couple days until I get to it. I'd also like to (finally) dig into how platform tests work and I'll add those if possible. I know we also want includeEngines/excludeEngines - should I delay this PR for that or should I submit a second one (probably at some point after JavaOne)?

[marcphilipp]

Sure, no worries. I think includeEngines/excludeEngines should be done in a separate pull request. Thanks!

[smoyer64]

One of these days I'll remember to apply Spotless before I commit ... I guess I'm still learning the Gradle/JUnit 5 workflow.

The latest commit allows one of includeTags or groups, and one of excludeTags or excludedGroups per the discussion above. It does not check to make sure whether the same tag is implicitly or explicitly included and excluded at the same time.

Unit tests were added to cover the logic required to determine which filters should be created and the code to actually create them.

Since this provider is an surrogate of a Mojo and derives its configuration from the parent plugin, I also didn't write any tests that actually execute the maven-surefire-plugin - I've done that in the past and could write tests at that level if needed. Since the filters are completely specified as properties in the ProviderParameters, I don't feel it's required.

Sorry it took so long for me to return to this task - JavaOne and these weeks afterwards have been busy.

[marcphilipp]

@junit-team/junit-lambda I think this is ready to be merged. Do you want to take a look?

[sbrannen]

Please change this to "JUnit Platform" instead of "JUnit 5".

[sbrannen]

Replace 0 with filters.size().

[sbrannen]

Rename to getGroupsOrTags.

[sbrannen]

Use Preconditions.condition(...).

[marcphilipp]

Good catches, @sbrannen!

[marcphilipp]

I guess the tests will pick this up, but this line should be

Preconditions.condition(!groups.isPresent() || !tags.isPresent(), EXCEPTION_MESSAGE_BOTH_NOT_ALLOWED);

[smoyer64]

Actually, we want to throw the exception only if both are provided per the discussion above - providing neither is allowed.

And I just learned that Spotless doesn't clean up unused imports ;)

[marcphilipp]

Yes, Preconditions.condition() throws an exception if the first argument is false. !groups.isPresent() || !tags.isPresent() is only false when both groups and tags are present.

[smoyer64]

Got it - !(a && b) = !a || ! b

[smoyer64]

I like the suggestions provided the code review with one exception - I'm not so sure how I feel about using the size of an array to provide the "prototype" for the type of array that the list should be converted to.

I've always used a zero size array during that conversion which seems to be in alignment with the Sun documentation: http://docs.oracle.com/javase/7/docs/api/java/util/List.html#toArray(T[]).

[marcphilipp]

I've always used a zero size array during that conversion which seems to be in alignment with the Sun documentation

Well, the Sun documentation says

If the list fits in the specified array, it is returned therein. Otherwise, a new array is allocated with the runtime type of the specified array and the size of this list.

Thus, given a non-empty list, if you use zero, you will effectively allocate two arrays whereas when you pass an array with a length that equals the list's size you will only allocate one. However, the JVM or the JIT compiler may optimize that at runtime.

[smoyer64]

I'm going to have to time that both ways ... but your argument sounds logical.

[marcphilipp]

Thanks! 👍

[smoyer64]

No problem ... and sorry to be so high-maintenance today!

[nipafx]

Since your discussing the performance aspect of this, you should check to Aleksey Shipilev's Arrays of Wisdom of the Ancients, which ends with:

Bottom line: toArray(new T[0]) seems faster, safer, and contractually cleaner, and therefore should be the default choice now. Future VM optimizations may close this performance gap for toArray(new T[size]), rendering the current "believed to be optimal" usages on par with an actually optimal one. Further improvements in toArray APIs would follow the same logic as toArray(new T[0]) — the collection itself should create the appropriate storage.

But unless this is a hot code path (didn't check), the performance discussion is moot anyway. I'd do what's more readable and 0 makes it much shorter so I'd use that.

[smoyer64]

@nicolaiparlog

Thanks for the great link!

This particular code is only executed once when the Maven Surefire provider is reading its configuration, so I don't think it's an issue either way. If the rest of the JUnit 5 project explicitly sets the size of the "template" array, I'd say consistency is a great reason to do this one the same way.

I'm going to pass the article you referenced around the shop though.

[sbrannen]

Thanks for the link, @nicolaiparlog! Very interesting article.

As @smoyer64 mentioned, the performance hit in this particular use case is a moot point: neither variant will make a noticeable difference.

So let's leave it "as is" for the sake of consistency at the moment, and we can reassess our use of toArray(...) at a later date if it becomes pertinent.

[sbrannen]

I think this PR looks good to go now!

But... we may actually want to align the use of filters and selectors in the Surefire provider with the style used in the Launcher's request builder API and the Gradle plugin. See #554 for details.

@junit-team/junit-lambda what do you think? Shall we merge this PR as is and address the configuration style consistency in a separate issue (potentially slotted for M4)?

[marcphilipp]

I think we should merge this pull request as is and open a new ticket for the other configuration changes.

[marcphilipp]

I've created the follow-up issue #558 and will go ahead and merge this pull request.

[marcphilipp]

Thanks, @smoyer64! 👍

[sbrannen]

Thanks, @smoyer64!

Thanks, @marcphilipp.

[smoyer64]

You're very welcome ... thanks for all the work you're putting into keeping the project moving forward in a consistent manner.