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.

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

[WIP] JUnit 5 support within Test task #3886

Closed
wants to merge 4 commits into
base: master
from

Conversation

@ajoberstar
Copy link
Contributor

ajoberstar commented Dec 22, 2017

NOTE This is not anywhere near being ready to merge in. The commits are just garbage save points with no descriptions. There are few tests and the code isn't even working yet.

This version tries to stick to the existing Test task and not change anything as much as possible.

This is my least favorite given that it takes discovery pretty much out of the JUnit Platform's hands.

Current issues:

  • Tests appear to execute, but events are not being sent back through the result processor chain

Also see #3887.

ajoberstar added some commits Dec 8, 2017

@ajoberstar

This comment has been minimized.

Copy link
Contributor

ajoberstar commented Dec 22, 2017

@eriwen @bmuschko Current state of what I'd been working. I've been sidetracked the past few days, and with Christmas coming up, I won't have time to look again for another couple weeks.

I am a little stuck at the moment with the tests failing to report back correctly. I'm not sure where this info is getting lost, as I spent a bunch of time trying to see what was happening with a debugger.

This test actually just seems to hang when the tests "finish" though the events aren't coming through either.

@eriwen

This comment has been minimized.

Copy link
Member

eriwen commented Dec 22, 2017

Thanks @ajoberstar. I think Ben and I are taking some time off as well and will review this at the beginning of the year.

@bmuschko
Copy link
Contributor

bmuschko left a comment

Thanks for the spike! Your changes move toward the right direction. I left some comments about the exposed API and the hanging test execution. Would you mind having a look at the listener implementation again? I think there's a problem in there.

/**
* Specifies that JUnit Platform should be used to execute the tests. <p> To configure JUnit Platform specific options, see {@link #useJUnitPlatform(groovy.lang.Closure)}.
*/
public void useJUnitPlatform() {

This comment has been minimized.

@bmuschko

bmuschko Jan 10, 2018

Contributor

FYI: In the final version, we'll need to annotate new public methods with @since and @Incubating.

This comment has been minimized.

@ajoberstar

ajoberstar Jan 11, 2018

Contributor

Sounds good.

*
* @param testFrameworkConfigure A closure used to configure the JUnit options.
*/
public void useJUnitPlatform(Closure testFrameworkConfigure) {

This comment has been minimized.

@bmuschko

bmuschko Jan 10, 2018

Contributor

We may not need the method that takes a Closure. The one with the Action is probably got enough to be handled by Java, Groovy and Kotlin. Should be tested in an integration test.

This comment has been minimized.

@ajoberstar

ajoberstar Jan 11, 2018

Contributor

OK, sounds good.

/**
* Specifies that JUnit Platform should be used to execute the tests. <p> To configure JUnit Platform specific options, see {@link #useJUnitPlatform(groovy.lang.Closure)}.
*/
public void useJUnitPlatform() {

This comment has been minimized.

@bmuschko

bmuschko Jan 10, 2018

Contributor

We need to think about the naming of this method. Many users might probably not know what JUnit Platform is. I think what we should do is the following:

  1. Name this method useJUnit5().
  2. Deprecate the existing method useJUnit().
  3. Introduce a new method useJUnit4() as replacement for 2.

This comment has been minimized.

@ajoberstar

ajoberstar Jan 11, 2018

Contributor

The wrinkle with that is that the JUnit Platform JARs are versioned as 1.0 not 5.0 like the Jupiter (JUnit 5 proper) JARs. I agree that useJUnit5 will seem more obvious at first glance, but not sure if it accurately describes the feature.

This comment has been minimized.

@marcphilipp

marcphilipp Jan 11, 2018

Member

We (the JUnit team) would like to be able to increment the major version number in the future. How about exposing both useJUnit5() and useJUnitPlatform()?

This comment has been minimized.

@sbrannen

sbrannen Jan 11, 2018

I'd even vote for just useJUnitPlatform().

IMHO, it's better to educate people about the "JUnit Platform" now rather than calling JUnit Platform support "JUnit 5", especially since test engines other than the JUnit Jupiter test engine can and will be run via useJUnitPlatform().

This comment has been minimized.

@ajoberstar

ajoberstar Jan 16, 2018

Contributor

I'd agree with @sbrannen.

This comment has been minimized.

@blindpirate

blindpirate Feb 6, 2018

Member

Yes, we can, but that will make things quite opaque in my opinion. We can sure infer JUnit4/JUnit5/TestNG from test runtime classpath, but why should we do this? With explicit useJUnitPlatform, build users can know without consulting documentation, "this build is using JUnit 5". What if users happen to depend all these test frameworks and just want to launch one of them to run tests?

What's more, we need some configuration closure like

useJUnitPlatform {
     excludeTags 'tag1', 'tag2'
}

So I think an extra method is necessary here.

This comment has been minimized.

@melix

melix Feb 6, 2018

Member

I would argue that you already opt-in by declaring the test dependency on junit5 or testng. This is something I have always found awkward with Gradle, as it's redundant. Also, configuration of the excludes should probably be done on a junit5 { ... } block, not on a flag to enable it. This would also allow us to move this to a junit5 plugin, which is more or less what we had started to do with the software model work a couple of years ago. In short, instead of adding a dependency and calling useJUnit5(), we could very well have a junit plugin and do this:

plugins {
   id 'junit5'
}

junit {
    version '5.0.1' // optional
    excludeTags 'tag1', 'tag2' 
}

This comment has been minimized.

@melix

melix Feb 6, 2018

Member

To be clear, the reason I prefer this approach is that it is modeling how we test rather than what we use to test. In particular, I'd be interested in modeling, in the future, things like executing the same test suite on different target platforms.

This comment has been minimized.

@marcphilipp

marcphilipp Feb 6, 2018

Member

I think Bo is trying to follow the same approach that's already been taken for useJUnit {...} and useTestNG {...}. Whether or not this should be a separate plugin is probably beyond the scope of this PR. Whatever you decide, please don't call it junit5 because there will be a JUnit 6 some day and it will hopefully also use the JUnit Platform, although predicting the future is always hard. 😉

This comment has been minimized.

@melix

melix Feb 6, 2018

Member

I'm happy with this too:

junitPlatform {
...
}

@Override
public void processTestClass(TestClassRunInfo testClass) {
System.out.println("Start processing test class: " + testClass.getTestClassName());

This comment has been minimized.

@bmuschko

bmuschko Jan 10, 2018

Contributor

Should use SLF4J logger.

This comment has been minimized.

@ajoberstar

ajoberstar Jan 11, 2018

Contributor

Don't intend to keep these either, there will probably be some logging needs (for which I'll use SLF4J). This was just frustrated debugging. 😄

This comment has been minimized.

@bmuschko

bmuschko Jan 19, 2018

Contributor

Thanks for clarifying.

executer.noExtraLogging()
}

@Timeout(10)

This comment has been minimized.

@bmuschko

bmuschko Jan 10, 2018

Contributor

Why do we need the timeout here? I'd expect the execution to be fast.

This comment has been minimized.

@ajoberstar

ajoberstar Jan 11, 2018

Contributor

Right, it should be fast. Since the test hangs indefinitely I just wanted it to fail if it wasn't going to complete normally. This wouldn't stick around in a working impl.

This comment has been minimized.

@bmuschko

bmuschko Jan 19, 2018

Contributor

Thanks for clarifying.

import org.junit.Rule
import spock.lang.Timeout

class JUnitPlatformIntegrationTest extends AbstractIntegrationSpec {

This comment has been minimized.

@bmuschko

bmuschko Jan 10, 2018

Contributor

Looks to me like org.apiguardian:apiguardian-api:1.0.0 isn't properly discovered on the test runtime classpath when executing the test. This at least starts the tests:

testCompile 'org.apiguardian:apiguardian-api:1.0.0'

This comment has been minimized.

@ajoberstar

ajoberstar Jan 11, 2018

Contributor

Currently that JAR should be on the compile classpath of the testing-jvm module. Are you suggesting adding it to testCompile, as well?

This comment has been minimized.

@bmuschko

bmuschko Jan 19, 2018

Contributor

For some reason the dependency isn't picked up at runtime and I got a warning message.

@Override
public void startProcessing(TestResultProcessor resultProcessor) {
this.launcher = LauncherFactory.create();
launcher.registerTestExecutionListeners(new GradleTestExecutionListener(new InspectingTestResultProcessor(resultProcessor), clock));

This comment has been minimized.

@bmuschko

bmuschko Jan 10, 2018

Contributor

I believe something wrong with the listener implementation. Test execution seems to hang for me. If I use org.junit.platform.launcher.listeners.SummaryGeneratingListener then the test task at least finishes and I retrieve the following summary via listener.getSummary().printTo(new PrintWriter(System.out));:

Test run finished after 8 ms
    [         2 containers found      ]
    [         0 containers skipped    ]
    [         2 containers started    ]
    [         0 containers aborted    ]
    [         2 containers successful ]
    [         0 containers failed     ]
    [         2 tests found           ]
    [         0 tests skipped         ]
    [         2 tests started         ]
    [         0 tests aborted         ]
    [         2 tests successful      ]
    [         0 tests failed          ]

This comment has been minimized.

@ajoberstar

ajoberstar Jan 11, 2018

Contributor

Agreed. The issue is that I can't see where the events are being dropped. I can't recall the specifics, since it's been a couple weeks but I spent a good amount of time in the debugger trying to trace when the events where going through and when they weren't. As far as I can tell my listener is getting fired when it should and passing things to Gradle's result processor. Somewhere after that things are getting lost.

Clearly those processors work, since you use them in many plugins, so it must be in how I'm using/configuring them. If anyone on your side has thoughts on where that's going wrong, it would be much appreciated.

This comment has been minimized.

@bmuschko

bmuschko Jan 19, 2018

Contributor

Our team will try to have a look at it soon.

LauncherDiscoveryRequest request = LauncherDiscoveryRequestBuilder.request()
.selectors(selectClass(testClass.getTestClassName()))
.build();
launcher.execute(request);

This comment has been minimized.

@marcphilipp

marcphilipp Jan 11, 2018

Member

Test classes should be collected and executed in a single run.

This comment has been minimized.

@ajoberstar

ajoberstar Jan 16, 2018

Contributor

Doing it as one run is my preference (see my stab at that in #3887). I find the existing testing infrastructure excessively overcomplicated. The JUnit Platform API is wonderful and has a very nice abstraction around it's feature sets. I'd really prefer not to box JUnit Platform support in just to fit the existing Gradle infrastructure.

This comment has been minimized.

@bmuschko

bmuschko Jan 19, 2018

Contributor

Thanks for your comment on this, @marcphilipp. I was about to that that comment as well.

This comment has been minimized.

@sbrannen

sbrannen Jan 22, 2018

I'd really prefer not to box JUnit Platform support in just to fit the existing Gradle infrastructure.

I truly could not agree more with that statement!

/**
* Specifies that JUnit Platform should be used to execute the tests. <p> To configure JUnit Platform specific options, see {@link #useJUnitPlatform(groovy.lang.Closure)}.
*/
public void useJUnitPlatform() {

This comment has been minimized.

@marcphilipp

marcphilipp Jan 11, 2018

Member

We (the JUnit team) would like to be able to increment the major version number in the future. How about exposing both useJUnit5() and useJUnitPlatform()?

@@ -0,0 +1,4 @@
handlers = java.util.logging.ConsoleHandler
.level = ALL

This comment has been minimized.

@childnode

childnode Jan 28, 2018

Is this period beginning line intended?

@oehme

This comment has been minimized.

Copy link
Member

oehme commented Feb 2, 2018

Thank you for spiking this @ajoberstar! It has been a great base for discussion. It's now being implemented in #4116, targeted for Gradle 4.6

@oehme oehme closed this Feb 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment