Make Groovy global mocking more strict (#1848)

Add check to prevent double global mocking

Co-authored-by: Andreas Turban <[email protected]>

docs/interaction_based_testing.adoc

@@ -952,6 +952,8 @@ it is only used to describe the interaction.
 NOTE: A global mock can only be created for a class type. It effectively replaces
 all instances of that type for the duration from mock creation up until the end of the feature method.
 
+CAUTION: Using global mocks for standard types from the JDK, for example `ArrayList`, is a bad idea and can lead to unforeseen and widespread consequences.
+
 CAUTION: The declaration order of global mocks is relevant.
          The `GroovySpy(global:true, <type>)` must come before all creations of new mocked/spied objects of `<type>`.
          The global spies will only take effect on objects of that type, if the `GroovySpy(global:true, <type>)` was
@@ -983,6 +985,15 @@ When `Specifications` or `Features` are executed concurrently you have to make s
 global mocks on the same types are properly guarded against each other, because a global mock changes the global state
 for the mocked `Class` during execution.
 
+[[global-mocks-parallel-execution]]
+==== Global mocks and parallel execution
+
+Creating a global `GroovyMock`/`GroovyStub`/`GroovySpy` when <<parallel-execution.adoc#parallel-execution,parallel execution>> is enabled,
+requires that the spec is annotated with <<parallel-execution.adoc#isolated-execution, @Isolated>> or `@ResourceLock(org.spockframework.runtime.model.parallel.Resources.META_CLASS_REGISTRY)`.
+If it is only used for a feature, then it suffices that the feature is annotated with `@ResourceLock(org.spockframework.runtime.model.parallel.Resources.META_CLASS_REGISTRY)`.
+The rule of thumb to choose between `@ResourceLock` and `@Isolated`, is to look at how widespread the mocked type is used.
+If it is widely used, then `@Isolated` is the safe choice, otherwise `@ResourceLock` may be sufficient.
+
 .How Are Global Groovy Mocks Implemented?
 ****
 Global Groovy mocks get their super powers from Groovy meta-programming. To be more precise,

docs/parallel_execution.adoc

@@ -643,6 +643,7 @@ skinparam shadowing false
 @endwbs
 ....
 
+[[isolated-execution]]
 === Isolated Execution
 
 Sometimes, you want to modify and test something that affects every other feature, you could put a `READ` `@ResourceLock` on _every_ feature, but that is impractical.

docs/release_notes.adoc

@@ -8,6 +8,9 @@ include::include.adoc[]
 === Breaking Changes
 
 * Calling `old(...)` with multiple arguments is now a compilation error. Previously the additional arguments were simply ignored.
+* Creating `GroovyMock`/`GroovyStub`/`GroovySpy` for an already mocked type will now fail.
+* Creating a global `GroovyMock`/`GroovyStub`/`GroovySpy` when <<parallel-execution.adoc#parallel-execution, parallel execution>> is enabled,
+  will now require that the spec is annotated with <<parallel-execution.adoc#isolated-execution, @Isolated>> or `@ResourceLock(org.spockframework.runtime.model.parallel.Resources.META_CLASS_REGISTRY)`. See <<interaction-based-testing.adoc#global-mocks-parallel-execution, Global mocks and parallel execution>>
 
 === Misc
 * Add support for <<extensions.adoc#extension-store,keeping state in extensions>>

spock-core/src/main/java/org/spockframework/mock/runtime/GroovyMockFactory.java

@@ -17,15 +17,28 @@
 import org.spockframework.mock.*;
 import org.spockframework.runtime.GroovyRuntimeUtil;
 import org.spockframework.runtime.RunContext;
+import org.spockframework.runtime.model.FeatureInfo;
+import org.spockframework.runtime.model.IterationInfo;
+import org.spockframework.runtime.model.parallel.ExclusiveResource;
+import org.spockframework.runtime.model.parallel.ResourceAccessMode;
+import org.spockframework.runtime.model.parallel.Resources;
 import org.spockframework.util.ReflectionUtil;
 import org.spockframework.util.SpockDocLinks;
+import spock.config.RunnerConfiguration;
 import spock.lang.Specification;
 
 import java.lang.reflect.Modifier;
+import java.util.Set;
 
 import groovy.lang.*;
 
 public class GroovyMockFactory implements IMockFactory {
+
+  private static final ExclusiveResource META_CLASS_REGISTRY_RW = new ExclusiveResource(Resources.META_CLASS_REGISTRY,
+    ResourceAccessMode.READ_WRITE);
+  private static final ExclusiveResource GLOBAL_LOCK = new ExclusiveResource(
+    org.junit.platform.engine.support.hierarchical.ExclusiveResource.GLOBAL_KEY, ResourceAccessMode.READ_WRITE);
+
   public static final GroovyMockFactory INSTANCE = new GroovyMockFactory();
 
   @Override
@@ -35,12 +48,23 @@ public boolean canCreate(IMockConfiguration configuration) {
 
   @Override
   public Object create(IMockConfiguration configuration, Specification specification) throws CannotCreateMockException {
+    final Class<?> type = configuration.getType();
     final MetaClass oldMetaClass = GroovyRuntimeUtil.getMetaClass(configuration.getType());
+    if (oldMetaClass instanceof GroovyMockMetaClass) {
+      throw new CannotCreateMockException(type,
+        ". The given type is already mocked by Spock.");
+    }
     GroovyMockMetaClass newMetaClass = new GroovyMockMetaClass(configuration, specification, oldMetaClass);
-    final Class<?> type = configuration.getType();
 
     boolean hasAdditionalInterfaces = !configuration.getAdditionalInterfaces().isEmpty();
     if (configuration.isGlobal()) {
+      if (!isIsolatedOrHasMetaClassRegistryReadWriteLock(specification)) {
+        throw new CannotCreateMockException(type,
+          ". Global mocking in parallel execution mode is only possible, when the specification is @Isolated, " +
+            "or the specification or feature is annotated with " +
+            "@ResourceLock(org.spockframework.runtime.model.parallel.Resources.META_CLASS_REGISTRY).");
+      }
+
       if (type.isInterface()) {
         throw new CannotCreateMockException(type,
             ". Global mocking is only possible for classes, but not for interfaces.");
@@ -93,6 +117,20 @@ public Object create(IMockConfiguration configuration, Specification specificati
     return proxy;
   }
 
+  private boolean isIsolatedOrHasMetaClassRegistryReadWriteLock(Specification specification) {
+    if (!RunContext.get().getConfiguration(RunnerConfiguration.class).parallel.enabled) {
+      // we don't have a problem in single threaded execution
+      return true;
+    }
+    FeatureInfo feature = specification.getSpecificationContext().getCurrentIteration().getFeature();
+    Set<ExclusiveResource> specExclusiveResources = feature.getSpec().getExclusiveResources();
+    if (specExclusiveResources.contains(GLOBAL_LOCK) || specExclusiveResources.contains(META_CLASS_REGISTRY_RW)) {
+      return true;
+    }
+    // GLOBAL_LOCK can't be declared on the feature
+    return feature.getExclusiveResources().contains(META_CLASS_REGISTRY_RW);
+  }
+
   private boolean isFinalClass(Class<?> type) {
     return !type.isInterface() && Modifier.isFinal(type.getModifiers());
   }

spock-specs/src/test/groovy/org/spockframework/docs/interaction/GlobalMockDocSpec.groovy

@@ -1,10 +1,13 @@
 package org.spockframework.docs.interaction
 
+import org.spockframework.runtime.model.parallel.Resources
 import spock.lang.Issue
+import spock.lang.ResourceLock
 import spock.lang.Specification
 import spock.lang.Stepwise
 
 @Stepwise
+@ResourceLock(Resources.META_CLASS_REGISTRY)
 class GlobalMockDocSpec extends Specification {
 
   @Issue("https://github.com/spockframework/spock/issues/785")

spock-specs/src/test/groovy/org/spockframework/smoke/mock/GroovyMockGlobalClass.groovy

@@ -1,8 +1,11 @@
 package org.spockframework.smoke.mock
 
-import org.spockframework.smoke.mock.Subject
-import spock.lang.*
+import org.spockframework.runtime.model.parallel.Resources
+import spock.lang.Issue
+import spock.lang.ResourceLock
+import spock.lang.Specification
 
+@ResourceLock(Resources.META_CLASS_REGISTRY)
 @Issue('https://github.com/spockframework/spock/issues/761')
 class GroovyMockGlobalClass extends Specification {
 
@@ -32,4 +35,5 @@ class Subject {
 }
 
 class ClassA {}
+
 class ClassB {}

spock-specs/src/test/groovy/org/spockframework/smoke/mock/GroovyMocks.groovy

@@ -1,11 +1,50 @@
 package org.spockframework.smoke.mock
 
+import org.spockframework.mock.CannotCreateMockException
+import org.spockframework.runtime.model.parallel.Resources
+import spock.lang.ResourceLock
 import spock.lang.Specification
+import spock.lang.Unroll
 
 class GroovyMocks extends Specification {
   def "implement GroovyObject"() {
     expect:
     GroovyMock(List) instanceof GroovyObject
     GroovyMock(ArrayList) instanceof GroovyObject
   }
+
+  @ResourceLock(Resources.META_CLASS_REGISTRY)
+  @Unroll("A Groovy#typeB can't be created for a type that was already Groovy#typeA'd")
+  def "global GroovyMocks can't be created for a type that is already mocked"(String typeA, String typeB) {
+    given:
+    createMock(typeA)
+
+    when:
+    createMock(typeB)
+
+    then:
+    CannotCreateMockException e = thrown()
+    e.message == 'Cannot create mock for class org.spockframework.smoke.mock.GroovyMocks$LocalClassForMocking. The given type is already mocked by Spock.'
+
+    where:
+    [typeA, typeB] << ([['Mock', 'Stub', 'Spy']] * 2).combinations()
+  }
+
+  void createMock(String type) {
+    switch (type) {
+      case 'Mock':
+        GroovyMock(global: true, LocalClassForMocking)
+        break
+      case 'Stub':
+        GroovyStub(global: true, LocalClassForMocking)
+        break
+      case 'Spy':
+        GroovySpy(global: true, LocalClassForMocking)
+        break
+      default:
+        throw new IllegalArgumentException(type)
+    }
+  }
+
+  class LocalClassForMocking {}
 }