Repository: mockito/mockito
Issue: Mockito JUnit Jupiter extension does not correctly support parallel test execution
Labels: junit5

Sorry for not being able to provide the required information, but we just noticed that the MockitoExtension (mockito-junit-jupiter 2.24.5) leads to spurious test failures (e.g. due to calls to mocks not being recorded) when using "junit.jupiter.execution.parallel.enabled=true" with surefire. Prior we used our much simpler MockitoExtension which works fine in the same situation:

```java
public class MockitoExtension implements Extension, TestInstancePostProcessor, AfterEachCallback {
   @Override
   public void postProcessTestInstance(Object testInstance, ExtensionContext context) {
      MockitoAnnotations.initMocks(testInstance);
   }

   @Override
   public void afterEach(ExtensionContext extensionContext) throws Exception {
      Mockito.validateMockitoUsage();
   }
}
```

Issue comments:
Hey! Can you take a look at our FAQ wiki. We documented how Mockito supports parallel execution.
I took a look at the wiki, but found just a very short section about parallel testing in the FAQ section. I issued the bug, because the parallel execution of whole test cases with JUnit 5 seems to be broken. I do not have any complex tests which use mocks in combination with multithreading.

Anyway this seems to be a problem of the MockitoExtension, not Mockito itself. Because with our simple JUnit 5 extension for Mockito, parallel execution of test cases which use mocks with JUnit 5 works fine.
I got a similar issue. 
I have got a workaround, if only a few of your "Mockito"-test-classes in you testsuite are failing when running JUnit jupiter in parallel. You can execute those problematic tests using "SAME_THREAD" for avoiding Mockito concurrency issues.

```
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.api.parallel.Execution;
import org.junit.jupiter.api.parallel.ExecutionMode;
import org.mockito.junit.jupiter.MockitoExtension;

@ExtendWith(MockitoExtension.class)
@Execution(ExecutionMode.SAME_THREAD) // <--- Workaround
class SomeTest {
```
To be a bit more specific, WantedButNotInvoked exceptions are thrown by verification calls failing, because of not recorded invocations: "Actually, there were zero interactions with this mock.".
The main difference between the current and the simple implementation above is, that the simple one inits the mocks in postProcessTestInstance() whereas the current one inits them in beforeEach(). Any ideas why that might be a problem? 

I tried the failing tests with a modified version of the current MockitoExtension which was just altered to init the mocks in postProcessTestInstance() and disabled the init in beforeEach() and the tests do no longer fail. So this seems to be the source of the problems.
Hey guys, can you provide us a test that reproduces the problem? Thanks!
@mockitoguy Here you go: https://github.com/SchulteMarkus/mockito.bug1630

I am not sure, if I reduced the example as much as possible, but, well, the project is not large.
Thank you! The sample project looks great!

I looked at the code but I haven't had time to debug it. How does the JUnit5 parallel execution work? Are we sharing the test instance (and thus, sharing mocks, SUTs?).
You are welcome :-) 
I don't know details about JUnit's parallelization.
> How does the JUnit5 parallel execution work? Are we sharing the test instance (and thus, sharing mocks, SUTs?).

By default not, and the user must be very explicit to enable sharing of the same test class instance with parallel execution enabled. I.e., even if parallel execution mode is enabled by default, and a test class is annotated with `Lifecycle.PER_CLASS`, its tests will still be executed sequentially _unless_ it is also annotated with `@Execution(CONCURRENT)`: https://junit.org/junit5/docs/current/user-guide/#writing-tests-parallel-execution

I also bumped into this issue in our codebase, with some tests using `MockitoExtension` failing in parallel mode, and some working fine. Could not spot anything particularly special about the failing ones.

I can confirm, that the problem exists. I also have tests using MockitoExtension and they fail in a non-deterministic way randomly only if Junit5 parralelization is used.
Scenario:

1. JUnit 5 creates a single `ClassExtensionContext `for a multithreaded test, but each thread becomes an instance of the test class
2. Each thread will come into `postProcessTestInstance `where they put their test instance in the same `ClassExtensionContext.getStore(MOCKITO)` map with the same key (`TEST_INSTANCE`)
3. Each thread then will get into `beforeEach`. They get a thread unique `MethodExtensionContext`, but each of them contain the same instance of `ClassExtensionContext`. Then they fetch the the last updated `TEST_INSTANCE `from the parent `ClassExtensionContext `inside the `collectParentTestInstances `method. Each thread gets quasi a random Test Instance depending on the fact which thread was before inside the `postProcessTestInstance `method. Then they prepare their broken Session inside the store of the current `MethodExtensionContext `with a random Test Instance included.

That is the explanation. I am thinking about how to solve this.

It is not 100% clear to me why the parent context test instance is included. I will check how this scenario looks like in a singlethreaded environment.
This is really painful to debug but I'm making progress. What I found out is that "collectParentTestInstances()" is non-deterministic:

```
Thread[16] collectParentTestInstances() - returns: [org.mockito.AnotherTest@2bf588f6]
Thread[13] collectParentTestInstances() - returns: [org.mockito.AnotherTest@240da1f5, org.mockito.AnotherTest@6646267a]
Thread[17] collectParentTestInstances() - returns: [org.mockito.AnotherTest@6646267a]
Thread[14] collectParentTestInstances() - returns: [org.mockito.AnotherTest@f9b9d48]
```

Notice that AnotherTest@6646267a is processed twice. Effectively, initMocks() is called twice for that test instance, clearing up the invocations.

Do you have an idea how to fix collectParentTestInstances()?
Perhaps we drop the collectParentTestInstances() complexity and just keep the test instances in static ThreadLocal object in the extension. postProcessTestInstance() feeds the thread local state, beforeEach() consumes it. I don't know enough about the "context store" API, though.
I have a plan how to fix it. First, we need to upgrade junit-jupiter (#1788)
@mockitoguy I am not sure why you have closed the issue. In the attached commit I cannot see the fix for the problem.

Context store API seems to be pretty simple to me, it is just a map in the context to store some information. The real question is what is this kind of context hierarchy used for in Junit 5 and how it behaves in case of nested tests or multithreaded tests, or even in multithreaded nested tests. I guess it could have some interesting beahaviour in case of parameterized (dynamic) tests. Probably it would be a good idea simply to ask the JUnit developers about this issue. Maybe they could provide some valuable feedback.
PR #1789 fixes the problem. This ticket got closed by accident. Thanks for looking into it!
Looking forward to the next release!
I would like to confirm the bug is gone, but I don't know how to do it in https://github.com/SchulteMarkus/mockito.bug1630.
Can someone add a PR to https://github.com/SchulteMarkus/mockito.bug1630 using Mockito 3.0.12 (at least)? 
Release have been already published. See https://bintray.com/mockito/maven/mockito-development
@mockitoguy when will this fix be released to maven central?
I will publish a release today
I updated https://github.com/SchulteMarkus/mockito.bug1630 - seems to work without problems using Mockito v.3.1.0, thanks a lot!
I will delete https://github.com/SchulteMarkus/mockito.bug1630 within a few weeks.
Publish to Maven Central failed yet again. I will need to contact sonatype again...
(Although https://repo1.maven.org/maven2/org/mockito/mockito-core/3.1.0/ does exist, so it might just work)
I can confirm, that the solution of 3.1.0 version works for all of the tests in my project. Still I don't understand how JUnit works, but it is fine now, since the tests work.

Thanks for your fast fix & release!

Pull request review comments:
Instead of having a boolean here, can we pass in `context.getParent()` on line 164. E.g. we pass the first parent in already, but then we keep all the other logic the same. That seems a little bit nicer to me and easier to follow. The comment can then be placed before line 164 instead.
Please push a commit to this branch - it's simpler!
Sadly I am not at my working computer. I can do a push on Monday if you prefer.
This works :-) Push on Monday. Have a nice weekend!
Done PTAL

Patch that resolved the issue:
diff --git a/settings.gradle.kts b/settings.gradle.kts
index d57b84e560..dc31ca1f32 100644
--- a/settings.gradle.kts
+++ b/settings.gradle.kts
@@ -8,7 +8,8 @@ include("deprecatedPluginsTest",
     "junitJupiterExtensionTest",
     "module-test",
     "memory-test",
-    "errorprone")
+    "errorprone",
+    "junitJupiterParallelTest")
 
 rootProject.name = "mockito"
 
diff --git a/subprojects/junit-jupiter/src/main/java/org/mockito/junit/jupiter/MockitoExtension.java b/subprojects/junit-jupiter/src/main/java/org/mockito/junit/jupiter/MockitoExtension.java
index 9b90a7d16e..9f697eae8d 100644
--- a/subprojects/junit-jupiter/src/main/java/org/mockito/junit/jupiter/MockitoExtension.java
+++ b/subprojects/junit-jupiter/src/main/java/org/mockito/junit/jupiter/MockitoExtension.java
@@ -5,25 +5,30 @@
 package org.mockito.junit.jupiter;
 
 
-import org.junit.jupiter.api.extension.*;
+import static org.junit.jupiter.api.extension.ExtensionContext.Namespace.create;
+import static org.junit.platform.commons.support.AnnotationSupport.findAnnotation;
+
+import java.lang.reflect.Parameter;
+import java.util.LinkedHashSet;
+import java.util.Optional;
+import java.util.Set;
+import org.junit.jupiter.api.extension.AfterEachCallback;
+import org.junit.jupiter.api.extension.BeforeEachCallback;
+import org.junit.jupiter.api.extension.ExtensionContext;
 import org.junit.jupiter.api.extension.ExtensionContext.Namespace;
+import org.junit.jupiter.api.extension.ParameterContext;
+import org.junit.jupiter.api.extension.ParameterResolutionException;
+import org.junit.jupiter.api.extension.ParameterResolver;
+import org.junit.jupiter.api.extension.TestInstancePostProcessor;
 import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.MockitoSession;
+import org.mockito.internal.configuration.MockAnnotationProcessor;
 import org.mockito.internal.configuration.plugins.Plugins;
 import org.mockito.internal.session.MockitoSessionLoggerAdapter;
-import org.mockito.internal.configuration.MockAnnotationProcessor;
 import org.mockito.junit.MockitoJUnitRunner;
 import org.mockito.quality.Strictness;
 
-import java.lang.reflect.Parameter;
-import java.util.LinkedHashSet;
-import java.util.Optional;
-import java.util.Set;
-
-import static org.junit.jupiter.api.extension.ExtensionContext.Namespace.create;
-import static org.junit.platform.commons.support.AnnotationSupport.findAnnotation;
-
 /**
  * Extension that initializes mocks and handles strict stubbings. This extension is the JUnit Jupiter equivalent
  * of our JUnit4 {@link MockitoJUnitRunner}.
@@ -188,19 +193,30 @@ private Optional<MockitoSettings> retrieveAnnotationFromTestClasses(final Extens
     }
 
     private void collectParentTestInstances(ExtensionContext context, Set<Object> testInstances) {
-        Optional<ExtensionContext> parent = context.getParent();
+        Optional<ExtensionContext> initialParent = context.getParent();
 
-        while (parent.isPresent() && parent.get() != context.getRoot()) {
-            ExtensionContext parentContext = parent.get();
+        // Ignoring first parent avoids parallel execution issues
+        // We can ignore the first parent because it has the test instance that is already in 'testInstances'
+        // See how 'testInstances' is populated
+        initialParent.ifPresent(parent -> collectParentTestInstance(parent.getParent(), context, testInstances));
+    }
 
-            Object testInstance = parentContext.getStore(MOCKITO).remove(TEST_INSTANCE);
+    @SuppressWarnings("OptionalUsedAsFieldOrParameterType")
+    private void collectParentTestInstance(
+        Optional<ExtensionContext> parent, ExtensionContext context,
+        Set<Object> testInstances) {
 
-            if (testInstance != null) {
-                testInstances.add(testInstance);
-            }
+        parent.ifPresent(currentParent -> {
+            if (currentParent != context.getRoot()) {
+                Object testInstance = currentParent.getStore(MOCKITO).remove(TEST_INSTANCE);
 
-            parent = parentContext.getParent();
-        }
+                if (testInstance != null) {
+                    testInstances.add(testInstance);
+                }
+
+                collectParentTestInstance(currentParent.getParent(), context, testInstances);
+            }
+        });
     }
 
     /**
diff --git a/subprojects/junitJupiterParallelTest/junitJupiterParallelTest.gradle b/subprojects/junitJupiterParallelTest/junitJupiterParallelTest.gradle
new file mode 100644
index 0000000000..6da4f3b4d7
--- /dev/null
+++ b/subprojects/junitJupiterParallelTest/junitJupiterParallelTest.gradle
@@ -0,0 +1,15 @@
+description = "Tests that require fine tuned parallel settings for JUnit Jupiter (bug #1630)"
+
+apply from: "$rootDir/gradle/dependencies.gradle"
+
+apply plugin: "java"
+
+dependencies {
+    testImplementation libraries.junitJupiterApi
+    testImplementation project(":junit-jupiter")
+    testRuntime libraries.junitJupiterEngine
+}
+
+test {
+    useJUnitPlatform()
+}


Test patch:
diff --git a/subprojects/junitJupiterParallelTest/src/test/java/org/mockito/ParallelBugTest.java b/subprojects/junitJupiterParallelTest/src/test/java/org/mockito/ParallelBugTest.java
new file mode 100644
index 0000000000..e3e1f8bb40
--- /dev/null
+++ b/subprojects/junitJupiterParallelTest/src/test/java/org/mockito/ParallelBugTest.java
@@ -0,0 +1,83 @@
+/*
+ * Copyright (c) 2019 Mockito contributors
+ * This program is made available under the terms of the MIT License.
+ */
+package org.mockito;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+/**
+ * See bug #1630
+ */
+@ExtendWith(MockitoExtension.class)
+class ParallelBugTest {
+
+    @Mock
+    private SomeService someService;
+
+    @InjectMocks
+    private AnotherService anotherService;
+
+    @Test
+    void test() {
+        perform();
+    }
+
+    private void perform() {
+        // when
+        anotherService.callSomeService();
+
+        // then
+        Mockito.verify(someService).doSomething();
+    }
+
+    @Test
+    void test2() {
+        perform();
+    }
+
+    @Test
+    void test3() {
+        perform();
+    }
+
+    @Test
+    void test4() {
+        perform();
+    }
+
+    @Test
+    void test5() {
+        perform();
+    }
+
+    @Test
+    void test6() {
+        perform();
+    }
+
+    @Test
+    void test7() {
+        perform();
+    }
+
+    public static class AnotherService {
+        private final SomeService someService;
+
+        public AnotherService(final SomeService someService) {
+            this.someService = someService;
+        }
+
+        void callSomeService() {
+            someService.doSomething();
+        }
+    }
+
+    static class SomeService {
+
+        void doSomething() {
+        }
+    }
+}
diff --git a/subprojects/junitJupiterParallelTest/src/test/resources/junit-platform.properties b/subprojects/junitJupiterParallelTest/src/test/resources/junit-platform.properties
new file mode 100644
index 0000000000..cd19e48ea4
--- /dev/null
+++ b/subprojects/junitJupiterParallelTest/src/test/resources/junit-platform.properties
@@ -0,0 +1,3 @@
+junit.jupiter.execution.parallel.enabled=true
+junit.jupiter.execution.parallel.config.strategy=dynamic
+junit.jupiter.execution.parallel.mode.default=concurrent