Skip to content

Commit

Permalink
Best-effort error reporting for interactions on final methods
Browse files Browse the repository at this point in the history
Add error reporting code to the byte-buddy` mock maker
to report interaction on final methods, which can't be handled.

The IMockInteractionValidator interface allows IMockMakers
to implement different kinds of validations on mock interactions.

Fixes #2039
  • Loading branch information
AndreasTu committed Nov 10, 2024
1 parent b7d50c4 commit 5d5a3e2
Show file tree
Hide file tree
Showing 27 changed files with 725 additions and 63 deletions.
1 change: 1 addition & 0 deletions docs/release_notes.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ include::include.adoc[]
* Size of data providers is no longer calculated multiple times but only once
* Fix exception when using `@RepeatUntilFailure` with a data provider with unknown iteration amount. spockPull:2031[]
* Clarified documentation about data providers and `size()` calls spockIssue:2022[]
* Add best-effort error reporting for interactions on final methods when using the `byte-buddy` mock maker spockIssue:2039[]

== 2.4-M4 (2024-03-21)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
*/
public abstract class AstUtil {
private static final Pattern DATA_TABLE_SEPARATOR = Pattern.compile("_{2,}+");
private static final String GET_METHOD_NAME = "get";
private static final String GET_AT_METHOD_NAME = new IntegerArrayGetAtMetaMethod().getName();

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package org.spockframework.compiler;

import org.spockframework.compiler.model.*;
import org.spockframework.runtime.GroovyRuntimeUtil;
import org.spockframework.runtime.SpockException;
import org.spockframework.util.*;

Expand All @@ -26,7 +27,6 @@
import org.codehaus.groovy.ast.*;
import org.codehaus.groovy.ast.expr.*;
import org.codehaus.groovy.ast.stmt.*;
import org.codehaus.groovy.runtime.MetaClassHelper;
import org.codehaus.groovy.syntax.*;
import org.objectweb.asm.Opcodes;

Expand Down Expand Up @@ -109,7 +109,7 @@ private void changeFinalFieldInternalName(Field field) {
}

private void createSharedFieldGetter(Field field) {
String getterName = "get" + MetaClassHelper.capitalize(field.getName());
String getterName = GroovyRuntimeUtil.propertyToGetterMethodName(field.getName());
MethodNode getter = spec.getAst().getMethod(getterName, Parameter.EMPTY_ARRAY);
if (getter != null) {
errorReporter.error(field.getAst(),
Expand All @@ -135,7 +135,7 @@ private void createSharedFieldGetter(Field field) {
}

private void createFinalFieldGetter(Field field) {
String getterName = "get" + MetaClassHelper.capitalize(field.getName());
String getterName = GroovyRuntimeUtil.propertyToGetterMethodName(field.getName());
MethodNode getter = spec.getAst().getMethod(getterName, Parameter.EMPTY_ARRAY);
if (getter != null) {
errorReporter.error(field.getAst(),
Expand All @@ -158,7 +158,7 @@ private void createFinalFieldGetter(Field field) {
}

private void createSharedFieldSetter(Field field) {
String setterName = "set" + MetaClassHelper.capitalize(field.getName());
String setterName = GroovyRuntimeUtil.propertyToSetterMethodName(field.getName());
Parameter[] params = new Parameter[] { new Parameter(field.getAst().getType(), "$spock_value") };
MethodNode setter = spec.getAst().getMethod(setterName, params);
if (setter != null) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,18 @@
package org.spockframework.compiler;

import org.spockframework.mock.ISpockMockObject;

public class SpockNames {
public static final String VALUE_RECORDER = "$spock_valueRecorder";
public static final String ERROR_COLLECTOR = "$spock_errorCollector";
public static final String OLD_VALUE = "$spock_oldValue";
public static final String SPOCK_EX = "$spock_ex";
/**
* Name of the method {@link ISpockMockObject#$spock_get()}.
*/
public static final String SPOCK_GET = "$spock_get";
/**
* Name of the method {@link ISpockMockObject#$spock_mockInteractionValidator()}.
*/
public static final String SPOCK_MOCK_INTERATION_VALIDATOR = "$spock_mockInteractionValidator";
}
16 changes: 16 additions & 0 deletions spock-core/src/main/java/org/spockframework/mock/IMockObject.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package org.spockframework.mock;

import org.spockframework.mock.runtime.SpecificationAttachable;
import org.spockframework.util.Beta;
import org.spockframework.util.Nullable;
import spock.lang.Specification;

Expand All @@ -29,6 +30,13 @@ public interface IMockObject extends SpecificationAttachable {
@Nullable
String getName();

/**
* Returns the {@link #getName()} of this mock object, or {@code "unnamed"} if it has no name.
*
* @return the name of this mock object, or {@code "unnamed"} if it has no name
*/
String getMockName();

/**
* Returns the declared type of this mock object.
*
Expand Down Expand Up @@ -81,4 +89,12 @@ public interface IMockObject extends SpecificationAttachable {
* @return whether this mock object matches the target of the specified interaction
*/
boolean matches(Object target, IMockInteraction interaction);

/**
* Returns the used mock configuration which created this mock.
*
* @return the mock configuration
*/
@Beta
IMockConfiguration getConfiguration();
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,28 @@

package org.spockframework.mock;

import org.spockframework.mock.runtime.IMockInteractionValidator;
import org.spockframework.util.Beta;
import org.spockframework.util.Nullable;

/**
* Marker-like interface implemented by all mock objects that allows
* {@link MockUtil} to detect mock objects. Not intended for direct use.
* MockObject interface implemented by some {@link spock.mock.MockMakers} that allows the {@link org.spockframework.mock.runtime.MockMakerRegistry}
* to detect mock objects.
*
* <p>Not intended for direct use.
*/
public interface ISpockMockObject {

IMockObject $spock_get();

/**
* @return the {@link IMockInteractionValidator} used to verify {@link IMockInteraction}
* @see IMockInteractionValidator
* @since 2.4
*/
@Nullable
@Beta
default IMockInteractionValidator $spock_mockInteractionValidator() {
return null;
}
}
13 changes: 12 additions & 1 deletion spock-core/src/main/java/org/spockframework/mock/MockUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public class MockUtil {
* @return whether the given object is a (Spock) mock object
*/
public boolean isMock(Object object) {
return getMockMakerRegistry().asMockOrNull(object) != null;
return asMockOrNull(object) != null;
}

/**
Expand Down Expand Up @@ -69,6 +69,17 @@ public IMockObject asMock(Object object) {
return mockOrNull;
}

/**
* Returns information about a mock object or {@code null}, if the object is not a mock.
*
* @param object a mock object
* @return information about the mock object or {@code null}, if the object is not a mock.
*/
@Nullable
public IMockObject asMockOrNull(Object object) {
return getMockMakerRegistry().asMockOrNull(object);
}

/**
* Attaches mock to a Specification.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ public EqualMethodNameConstraint(String methodName) {
this.methodName = methodName;
}

public String getMethodName() {
return methodName;
}

@Override
public boolean isSatisfiedBy(IMockInvocation invocation) {
return invocation.getMethod().getName().equals(methodName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ public EqualPropertyNameConstraint(String propertyName) {
this.propertyName = propertyName;
}

public String getPropertyName() {
return propertyName;
}

@Override
public boolean isSatisfiedBy(IMockInvocation invocation) {
return propertyName.equals(getPropertyName(invocation));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package org.spockframework.mock.constraint;

import org.spockframework.mock.*;
import org.spockframework.mock.runtime.IMockInteractionValidator;
import org.spockframework.runtime.Condition;
import org.spockframework.runtime.InvalidSpecException;
import org.spockframework.util.CollectionUtil;
Expand Down Expand Up @@ -50,12 +51,27 @@ public String describeMismatch(IMockInvocation invocation) {
@Override
public void setInteraction(IMockInteraction interaction) {
this.interaction = interaction;
if (interaction.isRequired() && MOCK_UTIL.isMock(target)) {
IMockObject mockObject = MOCK_UTIL.asMock(target);
IMockObject mockObject = MOCK_UTIL.asMockOrNull(target);
if (mockObject != null) {
checkRequiredInteraction(mockObject, interaction);
validateMockInteraction(mockObject, interaction);
}
}

private void checkRequiredInteraction(IMockObject mockObject, IMockInteraction interaction) {
if (interaction.isRequired()) {
if (!mockObject.isVerified()) {
String mockName = mockObject.getName() != null ? mockObject.getName() : "unnamed";
throw new InvalidSpecException("Stub '%s' matches the following required interaction:" +
"\n\n%s\n\nRemove the cardinality (e.g. '1 *'), or turn the stub into a mock.\n").withArgs(mockName, interaction);
"\n\n%s\n\nRemove the cardinality (e.g. '1 *'), or turn the stub into a mock.\n").withArgs(mockObject.getMockName(), interaction);
}
}
}

private void validateMockInteraction(IMockObject mockObject, IMockInteraction interaction) {
if (target instanceof ISpockMockObject) {
IMockInteractionValidator validation = ((ISpockMockObject) target).$spock_mockInteractionValidator();
if (validation != null) {
validation.validateMockInteraction(mockObject, interaction);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
package org.spockframework.mock.runtime;

import org.spockframework.compiler.SpockNames;
import org.spockframework.mock.IMockObject;
import org.spockframework.mock.ISpockMockObject;
import org.spockframework.runtime.GroovyRuntimeUtil;

import java.lang.reflect.Method;
Expand All @@ -8,8 +11,13 @@

import groovy.lang.*;
import org.jetbrains.annotations.Nullable;
import org.spockframework.util.ReflectionUtil;

import static java.util.Objects.requireNonNull;

public abstract class BaseMockInterceptor implements IProxyBasedMockInterceptor {


private MetaClass mockMetaClass;

BaseMockInterceptor(MetaClass mockMetaClass) {
Expand Down Expand Up @@ -39,11 +47,11 @@ protected String handleGetProperty(GroovyObject target, Object[] args) {
MetaClass metaClass = target.getMetaClass();
//First try the isXXX before getXXX, because this is the expected behavior, if it is boolean property.
MetaMethod booleanVariant = metaClass
.getMetaMethod(GroovyRuntimeUtil.propertyToMethodName("is", propertyName), GroovyRuntimeUtil.EMPTY_ARGUMENTS);
.getMetaMethod(GroovyRuntimeUtil.propertyToBooleanGetterMethodName(propertyName), GroovyRuntimeUtil.EMPTY_ARGUMENTS);
if (booleanVariant != null && booleanVariant.getReturnType() == boolean.class) {
methodName = booleanVariant.getName();
} else {
methodName = GroovyRuntimeUtil.propertyToMethodName("get", propertyName);
methodName = GroovyRuntimeUtil.propertyToGetterMethodName(propertyName);
}
}
return methodName;
Expand All @@ -52,4 +60,11 @@ protected String handleGetProperty(GroovyObject target, Object[] args) {
protected boolean isMethod(Method method, String name, Class<?>... parameterTypes) {
return method.getName().equals(name) && Arrays.equals(method.getParameterTypes(), parameterTypes);
}

public static Object handleSpockMockInterface(Method method, IMockObject mockObject) {
if (method.getName().equals(SpockNames.SPOCK_MOCK_INTERATION_VALIDATOR)) {
return null; //This should be handled in the corresponding MockMakers.
}
return mockObject;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import net.bytebuddy.TypeCache;
import net.bytebuddy.description.annotation.AnnotationDescription;
import net.bytebuddy.description.method.MethodDescription;
import net.bytebuddy.description.modifier.MethodManifestation;
import net.bytebuddy.description.modifier.SynchronizationState;
import net.bytebuddy.description.modifier.SyntheticState;
import net.bytebuddy.description.modifier.Visibility;
Expand All @@ -31,19 +32,23 @@
import net.bytebuddy.dynamic.loading.MultipleParentClassLoader;
import net.bytebuddy.dynamic.scaffold.TypeValidation;
import net.bytebuddy.implementation.FieldAccessor;
import net.bytebuddy.implementation.FixedValue;
import net.bytebuddy.implementation.MethodDelegation;
import net.bytebuddy.implementation.bind.annotation.Morph;
import org.codehaus.groovy.runtime.callsite.AbstractCallSite;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.VisibleForTesting;
import org.spockframework.compiler.SpockNames;
import org.spockframework.mock.ISpockMockObject;
import org.spockframework.mock.codegen.Target;
import org.spockframework.util.ReflectionUtil;

import java.lang.reflect.Method;
import java.util.Collection;
import java.util.concurrent.Callable;
import java.util.concurrent.ThreadLocalRandom;

import static java.util.Objects.requireNonNull;
import static net.bytebuddy.matcher.ElementMatchers.none;

class ByteBuddyMockFactory {
Expand All @@ -60,6 +65,7 @@ class ByteBuddyMockFactory {
private static final Class<?> CODEGEN_TARGET_CLASS = Target.class;
private static final String CODEGEN_PACKAGE = CODEGEN_TARGET_CLASS.getPackage().getName();
private static final AnnotationDescription INTERNAL_ANNOTATION = AnnotationDescription.Builder.ofType(Internal.class).build();
private static final Method MOCK_INTERACTION_VALIDATOR_METHOD = requireNonNull(ReflectionUtil.getMethodByName(ISpockMockObject.class, SpockNames.SPOCK_MOCK_INTERATION_VALIDATOR));

/**
* This array contains {@link TypeCachingLock} instances, which are used as java monitor locks for
Expand Down Expand Up @@ -136,6 +142,10 @@ Object createMock(IMockMaker.IMockCreationSettings settings) {
.method(m -> !isGroovyMOPMethod(type, m))
.intercept(mockInterceptor())
.transform(mockTransformer())
.method(m -> m.represents(MOCK_INTERACTION_VALIDATOR_METHOD))
// Implement the $spock_mockInteractionValidation() method which returns the static field below, so we have a validation instance for every mock class
.intercept(FixedValue.reference(new ByteBuddyMockInteractionValidator(), SpockNames.SPOCK_MOCK_INTERATION_VALIDATOR))
.transform(validateMockInteractionTransformer())
.implement(ByteBuddyInterceptorAdapter.InterceptorAccess.class)
.intercept(FieldAccessor.ofField("$spock_interceptor"))
.defineField("$spock_interceptor", IProxyBasedMockInterceptor.class, Visibility.PRIVATE, SyntheticState.SYNTHETIC)
Expand All @@ -149,6 +159,10 @@ Object createMock(IMockMaker.IMockCreationSettings settings) {
return proxy;
}

private static Transformer<MethodDescription> validateMockInteractionTransformer() {
return Transformer.ForMethod.withModifiers(SynchronizationState.PLAIN, Visibility.PUBLIC, MethodManifestation.FINAL);
}

private static Transformer<MethodDescription> mockTransformer() {
return Transformer.ForMethod.withModifiers(SynchronizationState.PLAIN, Visibility.PUBLIC); //Overridden methods should be public and non-synchronized.
}
Expand Down
Loading

0 comments on commit 5d5a3e2

Please sign in to comment.