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.

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

MockitoExtension for JUnit5 #1221

Merged
merged 38 commits into from Mar 24, 2018
Merged

Conversation

ChristianSchwarz
Copy link
Contributor

@ChristianSchwarz ChristianSchwarz commented Oct 24, 2017

Fixes #445

Discussion: https://groups.google.com/forum/#!topic/mockito/R1L1aG2RgBQ

First of all, thanks to the JUnit5-Team (@sbrannen) for that great API! The implementation was straight forward and the docu is top notch. Not comparable to a JUnitRule or Runner implementation. One can feel the hard work.


The here provided MockitoExtension allows to mock/spy/validate like the well known Mockito-JUnitRule/Runner. This is still "work in progress", feel free to comment.

Usage:

@ExtendWith(MockitoExtension.class)
class JUnit5Test {
 
  @Mock
  private Dependency mock ;

  @Test
  void checkMockCreation(){
    when(mock.foo()).thenReturn("Hello JUnit5");
    mock.foo();
    verify(mock).foo();
  }
}

@ChristianSchwarz
Copy link
Contributor Author

Possibly we need an annotation on class or method level to provide control of the "strictness". E.g.:

@ExtendWith(MockitoExtension.class)
@MockitoStrictness(WARN)
class JUnit5Test {

    @MockitoStrictness(STRICT_STUBS)
    void testWithLocalDefinedStrictness(){
    }
}

@ChristianSchwarz ChristianSchwarz changed the title Mockito-Extension JUnit5 MockitoExtension for JUnit5 Oct 24, 2017
Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

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

Nice @ChristianSchwarz thanks for the work!

@@ -0,0 +1,14 @@
description = "Mockito preconfigured inline mock maker (intermediate and to be superseeded by automatic usage in a future version)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just so we don't forget: we need to update this description

apply from: "$rootDir/gradle/java-library.gradle"

sourceCompatibility = 1.8
targetCompatibility = 1.8
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to be 1.6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am afraid that don't work cause JUnit5 ExtensionContext.getTestInstance() returns Optional that was introduced in Java 8. IntelliJ refuses shows an error in this case.

Edit: see http://junit.org/junit5/docs/current/user-guide/#overview-java-versions

Optional<?> testInstance=context.getTestInstance();
if (!testInstance.isPresent()){
//for some reason junit don't give us the test instance
//there is nothing we can do in this case
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 weird. Can we construct a test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure..


session = mockitoSession()
.initMocks(testInstance.get())
.strictness(Strictness.WARN)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be an option then I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, see #1221 (comment) for a proposal.

@codecov-io
Copy link

codecov-io commented Oct 25, 2017

Codecov Report

Merging #1221 into release/2.x will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##             release/2.x    #1221   +/-   ##
==============================================
  Coverage          88.18%   88.18%           
  Complexity          2354     2354           
==============================================
  Files                291      291           
  Lines               5935     5935           
  Branches             710      710           
==============================================
  Hits                5234     5234           
  Misses               521      521           
  Partials             180      180
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/mockito/Mockito.java 96.55% <ø> (ø) 39 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b1a6e0...f4d943f. Read the comment docs.


dependencies {
compile project.rootProject
implementation libraries.junit5
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's reduce/change this dependency to org.junit.jupiter:junit-jupiter-api:5.0.1 since we should be fine with the api for implementing the extension.
Additionally let's add testRuntime libraries.junit5.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for these hints!
Fixed!
Edit: testRuntime libraries.junit5 doesn't work since the implementation of the extension depends on it

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, the implementation of the extension should just depend on the api.
Therefore we could change it to

implementation 'org.junit.jupiter:junit-jupiter-api:5.0.1'
testRuntime 'org.junit.vintage:junit-vintage-engine:5.0.1'

isn't it?

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

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

Could we subclass the extension for the silent and nonsilent variant? Thats in line with the JUnit4 variant :)

@ChristianSchwarz
Copy link
Contributor Author

Could we subclass the extension for the silent and nonsilent variant? Thats in line with the JUnit4 variant :)

You mean 3 extensions like MockitoWarnExtension, MockitoLenientExtension, MockitoStrictStubExtension ?

@ChristianSchwarz
Copy link
Contributor Author

ChristianSchwarz commented Oct 25, 2017

Current state: added support for strictness...

The Test class and nested test classes and methods can define the stricness by declaring @Strictness.
Note: The strictness level is inherited and can be overriden per context.

@ExtendWith(MockitoExtension.class)
//default strictness WARN
class StricnessTest {

    @Mock
    Predicate mock;

    @Test
    void strictnessFromTestRoot_defaultIsWarn_shouldLogUnnecessaryStubbingException() {
        when(mock.test(1)).thenReturn(true);
    }

    @Test
    @Strictness(LENIENT)
    void strictnessFromTestMethod_lenient_shouldIgnoreUnnecessaryStubbing() {
        when(mock.test(1)).thenReturn(true);
    }

    @Nested
    @Strictness(WARN)
    class NestedTest{

        @Mock
        Predicate mock;

        @Test
        void strictnessFromNestedTest_shouldLogUnnecessaryStubbing() {
            when(mock.test(1)).thenReturn(true);
        }

        @Test
        @Strictness(STRICT_STUBS)
        void strictnessFromTestMethod_shouldThrowUnnecessaryStubbingException() {
            when(mock.test(1)).thenReturn(true);
        }
    }
}


public class MockitoExtension implements BeforeEachCallback, AfterEachCallback {

private MockitoSession session;
Copy link
Contributor

Choose a reason for hiding this comment

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

JUnit does not seem to guarantee how to handle Extension instances, so I think that it is better to use ExtensionContext#getStore(Namespace) rather than holding the MockitoSession instance in the field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@TimvdLippe
Copy link
Contributor

@ChristianSchwarz Correct, it would have the 3 different versions again.

Also, Travis is unhappy because we are targeting Java 8 with this PR. Since we state Mockito 2 supports Java 6+, I am not sure if we can integrate into Mockito 2. Instead, we maybe should include it in master and publish a beta version of mockito 3?

@ChristianSchwarz
Copy link
Contributor Author

Instead, we maybe should include it in master and publish a beta version of mockito 3?

That means stable JUnit5 support will only be available when mockito 3 final is released. I guess this will not happen before Q4 2018.

An other option is to release junit5 support as seperate mockito 2 addon artefact which requires java 8, like junit 5 itself. It can be released much earlier. What do you think?

@ChristianSchwarz
Copy link
Contributor Author

You mean 3 extensions like MockitoWarnExtension, MockitoLenientExtension, MockitoStrictStubExtension ?

Correct, it would have the 3 different versions again.

What is the benefit for the user here? What do you think of the annotation proposal above? It allows to define strictness at method and nested test level.

@TimvdLippe
Copy link
Contributor

An other option is to release junit5 support as seperate mockito 2 addon artefact which requires java 8, like junit 5 itself. It can be released much earlier. What do you think?

If we can find a way to make that pass on the build, I am all for that 👍 👍

What is the benefit for the user here? What do you think of the annotation proposal above? It allows to define strictness at method and nested test level.

Yes definitely keep them, they are very useful to specify exceptions for a single method/nested class. However, to specify the default strictness on a whole test class, I would like to have that incorporated in the @ExtendWith annotation.

@tmurakami
Copy link
Contributor

JUnit 5 supports meta-annotations, so we could provide the following annotation:

@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.ANNOTATION_TYPE, ElementType.TYPE, ElementType.METHOD})
@ExtendWith(MockitoExtension.class)
public @interface WithMockito {
    Strictness value() default Strictness.STRICT_STUBS;
}

@ChristianSchwarz
Copy link
Contributor Author

ChristianSchwarz commented Oct 26, 2017

I am not sure if we should do that. We would have two annotation concepts to define the strictness, that is maybe confusing and inconsequent and even conflicting.

Both annotations @Strictness and @WithMockito are declared with target ElementType.TYPE to be applicable to classes. In JUnit5 we have 2 kind of test classes the root-test and nested-test. @WithMockito will be ignored on nested classes, thats fine. @Strictness can be placed on both, but this can lead to conflicts, like this:

@WithMockito(WARN)
@Strictness(STRICT_STUBS)
class Test{
   ...
}

Anyway the meta-annotation @WithMockito is a good idea to avoid boilerplate code, but I would like to leave out the strictness.
I overlook the ElementType.METHOD in your poposal @tmurakami . Now I get your point, one can use @WithMockito to init mocks and define the strictness on a root-test, nested and method level. This violates the SRP , and may lead to problems down the road. What do you think?

}

private static MockitoSession mockitoSession(ExtensionContext context) {
return (MockitoSession) context.getStore(MOCKITO).get(SESSION);
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TimvdLippe
Copy link
Contributor

@ChristianSchwarz Could you also add a test for #1163 and verify we can fix that issue?

}

private static MockitoSession mockitoSession(ExtensionContext context) {
return (MockitoSession) context.getStore(MOCKITO).get(SESSION);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since a MockitoSession instance acquired here is finished on afterEach(), I think that you should call Store#remove(Object, Class) instead of get().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, makes perfect sense!


private static Strictness retrieveStrictness(ExtensionContext context) {
Optional<AnnotatedElement> annotatedElement = context.getElement();
if (!annotatedElement.isPresent()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it is better to use AnnotationSupport#findAnnotation(AnnotatedElement, Class) which supports meta-annotations.

context.getElement()
       .flatMap(e -> AnnotationSupport.findAnnotation(e, Strictness.class))
       .map(Strictness::value)
       .orElse(DEFAULT_STRICTNESS);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True!

@tmurakami
Copy link
Contributor

@ChristianSchwarz Sorry I didn't explain it enough.
I thought that it would be better to provide an annotation (like @WithMockito) that was annotated with @ExtendWith(MockitoExtension.class) instead of providing @Strictness.
The reason is to reduce boilerplate code as you said.

This violates the SRP , and may lead to problems down the road. What do you think?

I was lacking consideration.
As you say, my proposal may violate the SPR.

If MockitoExtension supports meta-annotation, I suppose that Mockito users would be able to create an annotation like @WithMockito as needed.
So, I think we don't have to provide @WithMockito.

@tmurakami
Copy link
Contributor

BTW, JUnit's MockitoExtension supports method/constructor injection.
http://junit.org/junit5/docs/current/user-guide/#writing-tests-dependency-injection

What do you think about supporting it?

@ChristianSchwarz
Copy link
Contributor Author

What do you think about supporting it?

It was discussed in #445 and definetly worth a look. For this PR I would like to provide only a MVP, more features should be added in the next steps.

@ChristianSchwarz
Copy link
Contributor Author

So, I think we don't have to provide @WithMockito.

I like it and keep it for now but without stricness attribute.

@ChristianSchwarz
Copy link
Contributor Author

@TimvdLippe @tmurakami
Do you guys have an idea how to get the build green? I must admit I'm a gradle noob 😨

@TimvdLippe
Copy link
Contributor

TimvdLippe commented Oct 27, 2017 via email


dependencies {
compile project.rootProject
testRuntime libraries.assertj
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should use testCompile or testImplementation instead of testRuntime.
Otherwise, AssertJ classes would not be read at build time and you would not be able to compile test classes.

@tmurakami
Copy link
Contributor

@ChristianSchwarz Thank you. I'll read #445.

@TimvdLippe
Copy link
Contributor

@ChristianSchwarz I pushed fixes to this PR which should make the build pass now

@ChristianSchwarz
Copy link
Contributor Author

@TimvdLippe big thanks for fixing the build!

@TimvdLippe
Copy link
Contributor

I am going to polish up this PR this weekend to hopefully get this close to merging. It has been waiting for quite a while now 😄

@TimvdLippe
Copy link
Contributor

All right, I have polished up the implementation now (with help from @marcphilipp, much appreciated!).

The implementation now includes a @ConfiguredWithMockito which is an annotation that describes how Mockito should run. This includes the Strictness for now, but is extensible to future configuration we would like.

There are a couple of action points left:

  • @mockitoguy could you review this PR in terms of documentation/implementation?
  • ./gradlew build does NOT include the junit5 subproject tests. I have no clue why yet 😢
  • Running the tests directly works in IntelliJ, but if I run the module/package instead, I get the following stacktrace:
Mar 17, 2018 3:33:58 PM org.junit.platform.launcher.core.DefaultLauncher handleThrowable
WARNING: TestEngine with ID 'junit-jupiter' failed to discover tests
java.lang.ArrayStoreException: sun.reflect.annotation.TypeNotPresentExceptionProxy
	at sun.reflect.annotation.AnnotationParser.parseClassArray(AnnotationParser.java:724)
	at sun.reflect.annotation.AnnotationParser.parseArray(AnnotationParser.java:531)
	at sun.reflect.annotation.AnnotationParser.parseMemberValue(AnnotationParser.java:355)
	at sun.reflect.annotation.AnnotationParser.parseAnnotation2(AnnotationParser.java:286)
	at sun.reflect.annotation.AnnotationParser.parseAnnotations2(AnnotationParser.java:120)
	at sun.reflect.annotation.AnnotationParser.parseAnnotations(AnnotationParser.java:72)
	at java.lang.Class.createAnnotationData(Class.java:3521)
	at java.lang.Class.annotationData(Class.java:3510)
	at java.lang.Class.getDeclaredAnnotation(Class.java:3458)
	at org.junit.platform.commons.util.AnnotationUtils.findAnnotation(AnnotationUtils.java:135)
	at org.junit.platform.commons.util.AnnotationUtils.findAnnotation(AnnotationUtils.java:114)
	at org.junit.jupiter.engine.descriptor.JupiterTestDescriptor.determineDisplayName(JupiterTestDescriptor.java:86)
	at org.junit.jupiter.engine.descriptor.ClassTestDescriptor.<init>(ClassTestDescriptor.java:83)
	at org.junit.jupiter.engine.descriptor.ClassTestDescriptor.<init>(ClassTestDescriptor.java:77)
	at org.junit.jupiter.engine.discovery.TestContainerResolver.resolveClass(TestContainerResolver.java:100)
	at org.junit.jupiter.engine.discovery.TestContainerResolver.resolveElement(TestContainerResolver.java:45)
	at org.junit.jupiter.engine.discovery.JavaElementsResolver.tryToResolveWithResolver(JavaElementsResolver.java:208)
	at org.junit.jupiter.engine.discovery.JavaElementsResolver.lambda$resolve$9(JavaElementsResolver.java:195)
	at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)
	at java.util.Iterator.forEachRemaining(Iterator.java:116)
	at java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1801)
	at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:481)
	at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:471)
	at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:708)
	at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:499)
	at org.junit.jupiter.engine.discovery.JavaElementsResolver.resolve(JavaElementsResolver.java:198)
	at org.junit.jupiter.engine.discovery.JavaElementsResolver.lambda$resolveForAllParents$5(JavaElementsResolver.java:164)
	at java.util.Collections$SingletonSet.forEach(Collections.java:4767)
	at org.junit.jupiter.engine.discovery.JavaElementsResolver.resolveForAllParents(JavaElementsResolver.java:164)
	at org.junit.jupiter.engine.discovery.JavaElementsResolver.resolveContainerWithParents(JavaElementsResolver.java:85)
	at org.junit.jupiter.engine.discovery.JavaElementsResolver.resolveClass(JavaElementsResolver.java:60)
	at java.util.ArrayList.forEach(ArrayList.java:1255)
	at java.util.Collections$UnmodifiableCollection.forEach(Collections.java:1080)
	at org.junit.jupiter.engine.discovery.DiscoverySelectorResolver.lambda$resolve$2(DiscoverySelectorResolver.java:66)
	at java.util.ArrayList.forEach(ArrayList.java:1255)
	at org.junit.jupiter.engine.discovery.DiscoverySelectorResolver.resolve(DiscoverySelectorResolver.java:65)
	at org.junit.jupiter.engine.discovery.DiscoverySelectorResolver.resolveSelectors(DiscoverySelectorResolver.java:50)
	at org.junit.jupiter.engine.JupiterTestEngine.discover(JupiterTestEngine.java:61)
	at org.junit.platform.launcher.core.DefaultLauncher.discoverEngineRoot(DefaultLauncher.java:130)
	at org.junit.platform.launcher.core.DefaultLauncher.discoverRoot(DefaultLauncher.java:117)
	at org.junit.platform.launcher.core.DefaultLauncher.discover(DefaultLauncher.java:82)
	at com.intellij.junit5.JUnit5IdeaTestRunner.startRunnerWithArgs(JUnit5IdeaTestRunner.java:48)
	at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47)
	at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
	at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)

Mar 17, 2018 3:33:58 PM org.junit.platform.launcher.core.DefaultLauncher handleThrowable
WARNING: TestEngine with ID 'junit-jupiter' failed to discover tests
java.lang.ArrayStoreException: sun.reflect.annotation.TypeNotPresentExceptionProxy
	at sun.reflect.annotation.AnnotationParser.parseClassArray(AnnotationParser.java:724)
	at sun.reflect.annotation.AnnotationParser.parseArray(AnnotationParser.java:531)
	at sun.reflect.annotation.AnnotationParser.parseMemberValue(AnnotationParser.java:355)
	at sun.reflect.annotation.AnnotationParser.parseAnnotation2(AnnotationParser.java:286)
	at sun.reflect.annotation.AnnotationParser.parseAnnotations2(AnnotationParser.java:120)
	at sun.reflect.annotation.AnnotationParser.parseAnnotations(AnnotationParser.java:72)
	at java.lang.Class.createAnnotationData(Class.java:3521)
	at java.lang.Class.annotationData(Class.java:3510)
	at java.lang.Class.getDeclaredAnnotation(Class.java:3458)
	at org.junit.platform.commons.util.AnnotationUtils.findAnnotation(AnnotationUtils.java:135)
	at org.junit.platform.commons.util.AnnotationUtils.findAnnotation(AnnotationUtils.java:114)
	at org.junit.jupiter.engine.descriptor.JupiterTestDescriptor.determineDisplayName(JupiterTestDescriptor.java:86)
	at org.junit.jupiter.engine.descriptor.ClassTestDescriptor.<init>(ClassTestDescriptor.java:83)
	at org.junit.jupiter.engine.descriptor.ClassTestDescriptor.<init>(ClassTestDescriptor.java:77)
	at org.junit.jupiter.engine.discovery.TestContainerResolver.resolveClass(TestContainerResolver.java:100)
	at org.junit.jupiter.engine.discovery.TestContainerResolver.resolveElement(TestContainerResolver.java:45)
	at org.junit.jupiter.engine.discovery.JavaElementsResolver.tryToResolveWithResolver(JavaElementsResolver.java:208)
	at org.junit.jupiter.engine.discovery.JavaElementsResolver.lambda$resolve$9(JavaElementsResolver.java:195)
	at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)
	at java.util.Iterator.forEachRemaining(Iterator.java:116)
	at java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1801)
	at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:481)
	at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:471)
	at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:708)
	at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:499)
	at org.junit.jupiter.engine.discovery.JavaElementsResolver.resolve(JavaElementsResolver.java:198)
	at org.junit.jupiter.engine.discovery.JavaElementsResolver.lambda$resolveForAllParents$5(JavaElementsResolver.java:164)
	at java.util.Collections$SingletonSet.forEach(Collections.java:4767)
	at org.junit.jupiter.engine.discovery.JavaElementsResolver.resolveForAllParents(JavaElementsResolver.java:164)
	at org.junit.jupiter.engine.discovery.JavaElementsResolver.resolveContainerWithParents(JavaElementsResolver.java:85)
	at org.junit.jupiter.engine.discovery.JavaElementsResolver.resolveClass(JavaElementsResolver.java:60)
	at java.util.ArrayList.forEach(ArrayList.java:1255)
	at java.util.Collections$UnmodifiableCollection.forEach(Collections.java:1080)
	at org.junit.jupiter.engine.discovery.DiscoverySelectorResolver.lambda$resolve$2(DiscoverySelectorResolver.java:66)
	at java.util.ArrayList.forEach(ArrayList.java:1255)
	at org.junit.jupiter.engine.discovery.DiscoverySelectorResolver.resolve(DiscoverySelectorResolver.java:65)
	at org.junit.jupiter.engine.discovery.DiscoverySelectorResolver.resolveSelectors(DiscoverySelectorResolver.java:50)
	at org.junit.jupiter.engine.JupiterTestEngine.discover(JupiterTestEngine.java:61)
	at org.junit.platform.launcher.core.DefaultLauncher.discoverEngineRoot(DefaultLauncher.java:130)
	at org.junit.platform.launcher.core.DefaultLauncher.discoverRoot(DefaultLauncher.java:117)
	at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:90)
	at com.intellij.junit5.JUnit5IdeaTestRunner.startRunnerWithArgs(JUnit5IdeaTestRunner.java:65)
	at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47)
	at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
	at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)


Process finished with exit code 0
Empty test suite.

You can run the individual tests though and see how it works. I would like to finish this PR asap and publish next week, to finally have Mockito + Junit5 working 🎉

@TimvdLippe
Copy link
Contributor

I was able to fix the Gradle integration after following https://junit.org/junit5/docs/current/user-guide/#running-tests-build-gradle IntelliJ still broken, but I got to go now.

build.gradle Outdated
@@ -81,7 +81,7 @@ dependencies {
}

task wrapper(type: Wrapper) {
gradleVersion = '3.5'
gradleVersion = '4.2.1'
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be 4.6, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch 👍

p.apply plugin: 'ru.vyarus.animalsniffer'
p.dependencies {
signature 'org.codehaus.mojo.signature:java16:1.0@signature'
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would make sense to configure a 1.8 baseline for the junit5 subproject.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not really sure what you mean with baseline. We configure the Java 1.8 compatibility in the subproject:

sourceCompatibility = 1.8
targetCompatibility = 1.8

Could you clarify your comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Source and target compatibility make sure you only use Java 8 language features and compile to Java 8 byte code, respectively. The animalsniffer plugin checks that you don't use any APIs that are not present in whatever baseline you pick. For example, if you would use an API that was introduced in Java 9, and the build runs on Java 9, it would compile without any issues. Users on Java 8 would however get a runtime exception. For Java 8, there's org.codehaus.mojo.signature:java18:1.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see, yes good call. Done 👍

* Copyright (c) 2018 Mockito contributors
* This program is made available under the terms of the MIT License.
*/
package org.mockito.junit5;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename this package to org.mockito.junit.jupiter? The JUnit team has taken extra care not to put 5 into the package name to make major version updates possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good call, done 👍

Copy link
Member

@mockitoguy mockitoguy left a comment

Choose a reason for hiding this comment

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

This is great!!!

Can we discuss:

  • API changes (e.g. the name of the new annotation for configuring the extension)
  • default Strictness
  • why is Gradle team calling Jupiter 'JUnitPlatform' and we are calling it 'JUnit Jupiter'

Other than that, I love it.

@@ -38,7 +38,7 @@ script:
# We are using && below on purpose
# ciPerformRelease must run only when the entire build has completed
# This guarantees that no release steps are executed when the build or tests fail
- ./gradlew build idea -s -PcheckJava6Compatibility && ./gradlew ciPerformRelease
- ./gradlew build idea -s -PcheckJavaCompatibility && ./gradlew ciPerformRelease
Copy link
Member

Choose a reason for hiding this comment

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

Nice cleanup!

useJUnitPlatform()
}

tasks.javadoc.enabled = true
Copy link
Member

Choose a reason for hiding this comment

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

Most likely not needed - javadoc is enabled by default.

*/
@ExtendWith(MockitoExtension.class)
@Retention(RUNTIME)
public @interface ConfiguredWithMockito {
Copy link
Member

Choose a reason for hiding this comment

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

Can you update PR description with all the public API changes / additions so that the team can review them in one spot?

I'm curious why did you go with ConfiguredWithMockito? Alternatives:

  • ExtendWithMockito - is syntactically consistent with ExtendWith, default Jupiter annotation.
  • MockitoExtension.Settings - easier to discover, clearer intent (it's a setting for the extension, not general purpose Mockito configuration)
  • MockitoExtensionSettings

+1 to ExtendWithMockito
-1 to ConfiguredWithMockito

If I missed a discussion about it, please let me know so that I can read it and revise my vote.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was mostly after a discussion with @marcphilipp We concluded that this annotation is for "configuring" the test, as it takes configuration parameters of the extension. Therefore I would say this is more clear

Copy link
Member

Choose a reason for hiding this comment

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

Are you attached to “ConfiguredWithMockito” annotation name? It looks odd in test. Isn’t “WithMockito” or “MockitoSettings” nicer? “Configured” is redundant, “MockitoSettings” is nice because we already have withSettings() method at mock creation. I'm not against "ConfiguredWithMockito" but I think we could do better. I'm looking for a best name because long term we will have similar annotation in mockito-core.

* Configure the strictness used in this test.
* @return The strictness to configure, by default {@link Strictness#WARN}
*/
Strictness strictness() default Strictness.WARN;
Copy link
Member

Choose a reason for hiding this comment

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

New Mockito features (such as MockitoSession) uses Strictness.STRICT_STUBS by default because it helps us and the users:

  • cleaner tests that are easier to debug with STRICT_STUBS
  • it is a compatible way of introducing a new feature

How about we change the default for Jupiter to use STRICT_STUBS, too?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was under the impression that the WARN behavior was still default, but I just checked

public MockitoJUnitRunner(Class<?> klass) throws InvocationTargetException {
//by default, StrictRunner is used. We can change that potentially based on feedback from users
this(new StrictRunner(new RunnerFactory().createStrict(klass), klass));
}
and it is indeed Strict. Will update 👍

import static org.junit.platform.engine.discovery.DiscoverySelectors.selectClass;

@SuppressWarnings("ConstantConditions")
class StrictnessTest {
Copy link
Member

Choose a reason for hiding this comment

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

How come this test does not need MockitoExtension?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, meta-annotations. Cool.

Copy link
Contributor

Choose a reason for hiding this comment

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

This test is intended to run a single JUnit test that then runs inner classes (which do have the extension)

import static org.junit.platform.commons.support.AnnotationSupport.findAnnotation;

/**
* Extension that initializes mocks and handles strict stubbings. This extension is the JUnit Jupiter equivalent
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about a new entry in main mockito javadoc in Mockito.java? It's a noteworthy feature!!!

Copy link
Contributor

Choose a reason for hiding this comment

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

Done 👍

}

test {
useJUnitPlatform()
Copy link
Member

Choose a reason for hiding this comment

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

Gradle team call Junit Jupiter "JUnit platform". I'm curious why? Should we follow this naming convention?

Choose a reason for hiding this comment

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

JUnit Platform supports many engines, like "JUnit Jupiter (JUnit 5)", or "JUnit Vintage (JUnit 3 and 4)" or other TestEngine implementations, like those listed here: https://github.com/junit-team/junit5/wiki/Third-party-Extensions#junit-platform-test-engines

JUnit Jupiter is an implementation of the TestEngine interface ... which provides extension support. Here is where the Mockito Extension is living.

More details at: https://junit.org/junit5/docs/current/user-guide/#overview-what-is-junit-5

@mockitoguy
Copy link
Member

@sbrannen, what's the most consistent and popular naming convention? Gradle team calls new JUnit5 JUnitPlatform because in the Gradle file you write this:

test {
  useJUnitPlatform()
}

In this PR, we're shipping new module org.mockito:junit-jupiter (which I prefer). What is the best name?

@TimvdLippe
Copy link
Contributor

@mockitoguy The platform is something different than Jupiter. The platform is a separate organisation (https://mvnrepository.com/artifact/org.junit.platform) which handles integration with platform-oriented clients (like Gradle, IntelliJ, Maven, etc...). Instead, we are building an extension that is built on top of the JUnit API, and this is called Jupiter.

@sbrannen
Copy link

sbrannen commented Mar 20, 2018

@mockitoguy,

what's the most consistent and popular naming convention? Gradle team calls new JUnit5 JUnitPlatform...

That's because the Gradle test task actually launches the JUnit Platform -- which was released under the "JUnit 5" umbrella project". The "Platform" can be used to run test engines for JUnit Jupiter (aka, JUnit 5), JUnit Vintage (aka, JUnit 3 and 4), and any other testing frameworks for which there is a TestEngine (e.g., Spek, etc.).

So the Gradle team has made the best choice in calling that support for the JUnit Platform.

In this PR, we're shipping new module org.mockito:junit-jupiter (which I prefer). What is the best name?

junit-jupiter makes perfect sense here: your extension is an implementation of APIs from JUnit Jupiter. So you should definitely stick with that!

I do the same thing in Spring's support for JUnit Jupiter. For example, the SpringExtension in Spring Framework 5.0 resides in the org.springframework.test.context.junit.jupiter package within the spring-test artifact.

Make sense?

@sbrannen
Copy link

BTW, I highly recommend my explanation of what "JUnit 5" is here. 😉

JUnit 5 = JUnit Platform + JUnit Jupiter + JUnit Vintage

@blandger
Copy link

When can we expect releasing junit5 support by Mockito?

Copy link
Member

@mockitoguy mockitoguy left a comment

Choose a reason for hiding this comment

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

Thank you for addressing the feedback! 2 requests:

  • Sure about "ConfiguredWithMockito"? (see my comment, I've also reached out on slack)
  • Can you update PR description? Currently it has noise and it is first thing what users read (there will be link to this PR from the release notes)

* of our extension.
*/
@SuppressWarnings("ConstantConditions")
class StrictnessTest {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test that demonstrates that by default we're using STRICT_STUBS?

*/
@ExtendWith(MockitoExtension.class)
@Retention(RUNTIME)
public @interface ConfiguredWithMockito {
Copy link
Member

Choose a reason for hiding this comment

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

Are you attached to “ConfiguredWithMockito” annotation name? It looks odd in test. Isn’t “WithMockito” or “MockitoSettings” nicer? “Configured” is redundant, “MockitoSettings” is nice because we already have withSettings() method at mock creation. I'm not against "ConfiguredWithMockito" but I think we could do better. I'm looking for a best name because long term we will have similar annotation in mockito-core.

@TimvdLippe
Copy link
Contributor

@mockitoguy Feedback addressed! If no one else objects, I am merging this later today and publish a new Maven central release for Mockito Java 10/11 and Junit5 compatibility!

@TimvdLippe
Copy link
Contributor

All right, let's do this.

Thanks @ChristianSchwarz for the initial implementation,