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

Add annotation to mark a type as DoNotMock #1833

Merged
merged 11 commits into from Nov 19, 2021
36 changes: 36 additions & 0 deletions src/main/java/org/mockito/DoNotMock.java
@@ -0,0 +1,36 @@
/*
* Copyright (c) 2019 Mockito contributors
* This program is made available under the terms of the MIT License.
*/
package org.mockito;

import static java.lang.annotation.ElementType.TYPE;
import static java.lang.annotation.RetentionPolicy.RUNTIME;

import java.lang.annotation.Documented;
import java.lang.annotation.Retention;
import java.lang.annotation.Target;

/**
* Annotation representing a type that should not be mocked.
* <p>When marking a type {@code @DoNotMock}, you should always point to alternative testing
* solutions such as standard fakes or other testing utilities.
*
* Mockito enforces {@code @DoNotMock} with the {@link org.mockito.plugins.DoNotMockEnforcer}.
*
* If you want to use a custom {@code @DoNotMock} annotation, the {@link org.mockito.plugins.DoNotMockEnforcer}
* will match on annotations with a type ending in "org.mockito.DoNotMock". You can thus place
* your custom annotation in {@code com.my.package.org.mockito.DoNotMock} and Mockito will enforce
* that types annotated by {@code @com.my.package.org.mockito.DoNotMock} can not be mocked.
*/
TimvdLippe marked this conversation as resolved.
Show resolved Hide resolved
@Target({TYPE})
@Retention(RUNTIME)
@Documented
public @interface DoNotMock {
/**
* The reason why the annotated type should not be mocked.
*
* <p>This should suggest alternative APIs to use for testing objects of this type.
*/
String value() default "Create a real instance instead.";
TimvdLippe marked this conversation as resolved.
Show resolved Hide resolved
}
@@ -0,0 +1,16 @@
/*
* Copyright (c) 2019 Mockito contributors
* This program is made available under the terms of the MIT License.
*/
package org.mockito.exceptions.misusing;

import org.mockito.exceptions.base.MockitoException;

/**
* Thrown when attempting to mock a class that is annotated with {@link org.mockito.DoNotMock}.
*/
public class DoNotMockException extends MockitoException {
public DoNotMockException(String message) {
super(message);
}
}
34 changes: 34 additions & 0 deletions src/main/java/org/mockito/internal/MockitoCore.java
Expand Up @@ -16,12 +16,17 @@
import static org.mockito.internal.verification.VerificationModeFactory.noMoreInteractions;

import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

import org.mockito.InOrder;
import org.mockito.MockSettings;
import org.mockito.MockingDetails;
import org.mockito.exceptions.misusing.DoNotMockException;
import org.mockito.exceptions.misusing.NotAMockException;
import org.mockito.internal.configuration.plugins.Plugins;
import org.mockito.internal.creation.MockSettingsImpl;
import org.mockito.internal.invocation.finder.VerifiableInvocationsFinder;
import org.mockito.internal.listeners.VerificationStartedNotifier;
Expand All @@ -40,6 +45,7 @@
import org.mockito.invocation.Invocation;
import org.mockito.invocation.MockHandler;
import org.mockito.mock.MockCreationSettings;
import org.mockito.plugins.DoNotMockEnforcer;
import org.mockito.quality.Strictness;
import org.mockito.stubbing.LenientStubber;
import org.mockito.stubbing.OngoingStubbing;
Expand All @@ -50,6 +56,9 @@
@SuppressWarnings("unchecked")
public class MockitoCore {

private static final DoNotMockEnforcer DO_NOT_MOCK_ENFORCER = Plugins.getDoNotMockEnforcer();
private static final Set<Class<?>> SAFE_DONOTMOCK_ENFORCED_CLASSES = Collections.synchronizedSet(new HashSet<>());
Copy link
Member

Choose a reason for hiding this comment

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

I suggest "MOCKABLE_CLASSES"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


public boolean isTypeMockable(Class<?> typeToMock) {
return typeMockabilityOf(typeToMock).mockable();
}
Expand All @@ -60,11 +69,36 @@ public <T> T mock(Class<T> typeToMock, MockSettings settings) {
}
MockSettingsImpl impl = MockSettingsImpl.class.cast(settings);
MockCreationSettings<T> creationSettings = impl.build(typeToMock);
checkDoNotMockAnnotation(creationSettings.getTypeToMock(), creationSettings);
T mock = createMock(creationSettings);
mockingProgress().mockingStarted(mock, creationSettings);
return mock;
}

private void checkDoNotMockAnnotation(Class<?> typeToMock, MockCreationSettings<?> creationSettings) {
checkDoNotMockAnnotationForType(typeToMock);
creationSettings.getExtraInterfaces().forEach(MockitoCore::checkDoNotMockAnnotationForType);
}

private static void checkDoNotMockAnnotationForType(Class<?> type) {
if (type == null) {
TimvdLippe marked this conversation as resolved.
Show resolved Hide resolved
return;
}

if (SAFE_DONOTMOCK_ENFORCED_CLASSES.contains(type)) {
return;
}

DO_NOT_MOCK_ENFORCER.checkTypeForDoNotMockViolation(type).ifPresent(message -> {
throw new DoNotMockException(message);
});

checkDoNotMockAnnotationForType(type.getSuperclass());
Arrays.stream(type.getInterfaces()).forEach(MockitoCore::checkDoNotMockAnnotationForType);

SAFE_DONOTMOCK_ENFORCED_CLASSES.add(type);
Copy link
Member

Choose a reason for hiding this comment

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

I feel uneasy about this perf tweak. Do we need it? Does it really make things faster? Static state that keeps growing with every mocked type may bite us later.

Did you find this perf tweak useful at G?

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 pretty sure this is optimization is necessary as we can have large type hierarchies. I have asked internally for more background, but I expect this optimization to be necessary to not have a large performance impact for existing applications.

Copy link
Member

Choose a reason for hiding this comment

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

Adding perf tweaks "on hunch", without evidence is not good for the codebase (growing complexity :-). I don't mind this change but I'd love to get your assurance that this is useful.

You have the approval so ship this change at will!

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 will double check before merging. I haven't had time during the Christmas vacation to do this work. I probably get to this late next week.

}

public <T> OngoingStubbing<T> when(T methodCall) {
MockingProgress mockingProgress = mockingProgress();
mockingProgress.stubbingStarted();
Expand Down
@@ -0,0 +1,28 @@
/*
* Copyright (c) 2019 Mockito contributors
* This program is made available under the terms of the MIT License.
*/
package org.mockito.internal.configuration;

import java.util.Arrays;
import java.util.Optional;

import org.mockito.DoNotMock;
import org.mockito.plugins.DoNotMockEnforcer;

public class DefaultDoNotMockEnforcer implements DoNotMockEnforcer {

@Override
public Optional<String> checkTypeForDoNotMockViolation(Class<?> type) {
return Arrays.stream(type.getAnnotations()).filter(
annotation -> annotation.annotationType().getName().endsWith("org.mockito.DoNotMock"))
.findFirst()
.map(annotation -> {
String exceptionMessage = type + " is annotated with @DoNoMock and can't be mocked.";
Copy link
Member

Choose a reason for hiding this comment

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

do we want to say "is annotated with org.mockito.@DoNotMock"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's better :)

if (DoNotMock.class.equals(annotation.annotationType())) {
exceptionMessage += " " + type.getAnnotation(DoNotMock.class).value();
}
return exceptionMessage;
});
}
}
Expand Up @@ -9,6 +9,7 @@

import org.mockito.internal.creation.instance.InstantiatorProvider2Adapter;
import org.mockito.plugins.AnnotationEngine;
import org.mockito.plugins.DoNotMockEnforcer;
import org.mockito.plugins.InstantiatorProvider;
import org.mockito.plugins.InstantiatorProvider2;
import org.mockito.plugins.MockMaker;
Expand All @@ -31,6 +32,7 @@ class DefaultMockitoPlugins implements MockitoPlugins {
DEFAULT_PLUGINS.put(AnnotationEngine.class.getName(), "org.mockito.internal.configuration.InjectingAnnotationEngine");
DEFAULT_PLUGINS.put(INLINE_ALIAS, "org.mockito.internal.creation.bytebuddy.InlineByteBuddyMockMaker");
DEFAULT_PLUGINS.put(MockitoLogger.class.getName(), "org.mockito.internal.util.ConsoleMockitoLogger");
DEFAULT_PLUGINS.put(DoNotMockEnforcer.class.getName(), "org.mockito.internal.configuration.DefaultDoNotMockEnforcer");
}

@Override
Expand Down
Expand Up @@ -6,6 +6,7 @@

import org.mockito.internal.creation.instance.InstantiatorProviderAdapter;
import org.mockito.plugins.AnnotationEngine;
import org.mockito.plugins.DoNotMockEnforcer;
import org.mockito.plugins.InstantiatorProvider;
import org.mockito.plugins.InstantiatorProvider2;
import org.mockito.plugins.MockMaker;
Expand All @@ -31,6 +32,8 @@ class PluginRegistry {

private final MockitoLogger mockitoLogger = new PluginLoader(pluginSwitch)
.loadPlugin(MockitoLogger.class);
private final DoNotMockEnforcer doNotMockEnforcer = new PluginLoader(pluginSwitch).loadPlugin(
DoNotMockEnforcer.class);

PluginRegistry() {
Object impl = new PluginLoader(pluginSwitch).loadPlugin(InstantiatorProvider2.class, InstantiatorProvider.class);
Expand Down Expand Up @@ -89,4 +92,14 @@ AnnotationEngine getAnnotationEngine() {
MockitoLogger getMockitoLogger() {
return mockitoLogger;
}

/**
* Returns the DoNotMock enforce for the current runtime.
*
* <p> Returns {@link org.mockito.internal.configuration.DefaultDoNotMockEnforcer} if no
* {@link DoNotMockEnforcer} extension exists or is visible in the current classpath.</p>
*/
DoNotMockEnforcer getDoNotMockEnforcer() {
return doNotMockEnforcer;
}
}
Expand Up @@ -4,7 +4,9 @@
*/
package org.mockito.internal.configuration.plugins;

import org.mockito.DoNotMock;
import org.mockito.plugins.AnnotationEngine;
import org.mockito.plugins.DoNotMockEnforcer;
import org.mockito.plugins.InstantiatorProvider2;
import org.mockito.plugins.MockMaker;
import org.mockito.plugins.MockitoLogger;
Expand Down Expand Up @@ -72,4 +74,14 @@ public static MockitoLogger getMockitoLogger() {
public static MockitoPlugins getPlugins() {
return new DefaultMockitoPlugins();
}

/**
* Returns the {@link DoNotMock} enforcer available for the current runtime.
*
* <p> Returns {@link org.mockito.internal.configuration.DefaultDoNotMockEnforcer} if no
* {@link DoNotMockEnforcer} extension exists or is visible in the current classpath.</p>
*/
public static DoNotMockEnforcer getDoNotMockEnforcer() {
return registry.getDoNotMockEnforcer();
}
}
25 changes: 25 additions & 0 deletions src/main/java/org/mockito/plugins/DoNotMockEnforcer.java
@@ -0,0 +1,25 @@
/*
* Copyright (c) 2019 Mockito contributors
* This program is made available under the terms of the MIT License.
*/
package org.mockito.plugins;

import java.util.Optional;

/**
* Enforcer that is applied to every type in the type hierarchy of the class-to-be-mocked.
*/
public interface DoNotMockEnforcer {

/**
* If this type is allowed to be mocked. Return an empty optional if the enforcer allows
* this type to be mocked. Return a message if there is a reason this type can not be mocked.
*
* Note that Mockito performs traversal of the type hierarchy. Implementations of this class
* should therefore not perform type traversal themselves but rely on Mockito.
*
* @param type The type to check
* @return Optional message if this type can not be mocked, or an empty optional if type can be mocked
*/
Optional<String> checkTypeForDoNotMockViolation(Class<?> type);
Copy link
Member

Choose a reason for hiding this comment

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

So you prefer the enforcer to return the message instead of just throwing?

Works for me. Just checking.

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 felt like this was the nicer way of handling state, since we have to stop type traversal. I will mark it as Incubating and might change later, depending on how well it integrates with our internal system.

}
111 changes: 111 additions & 0 deletions src/test/java/org/mockitousage/annotation/DoNotMockTest.java
@@ -0,0 +1,111 @@
/*
* Copyright (c) 2019 Mockito contributors
* This program is made available under the terms of the MIT License.
*/
package org.mockitousage.annotation;

import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.Mockito.mock;

import org.junit.Test;
import org.mockito.DoNotMock;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
import org.mockito.exceptions.misusing.DoNotMockException;

public class DoNotMockTest {
Copy link
Member

Choose a reason for hiding this comment

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

Great test coverage.


@Test
public void can_not_mock_class_annotated_with_donotmock() {
assertThatThrownBy(() -> {
NotMockable notMockable = mock(NotMockable.class);
}).isInstanceOf(DoNotMockException.class);
}

@Test
public void can_not_mock_class_via_mock_annotation() {
assertThatThrownBy(() -> {
MockitoAnnotations.initMocks(new TestClass());
}).isInstanceOf(DoNotMockException.class);
}

@Test
public void can_not_mock_class_with_interface_annotated_with_donotmock() {
assertThatThrownBy(() -> {
SubclassOfNotMockableInterface notMockable = mock(SubclassOfNotMockableInterface.class);
}).isInstanceOf(DoNotMockException.class);
}

@Test
public void can_not_mock_subclass_with_unmockable_interface() {
assertThatThrownBy(() -> {
DoubleSubclassOfInterface notMockable = mock(DoubleSubclassOfInterface.class);
}).isInstanceOf(DoNotMockException.class);
}

@Test
public void can_not_mock_subclass_with_unmockable_superclass() {
assertThatThrownBy(() -> {
SubclassOfNotMockableSuperclass notMockable = mock(SubclassOfNotMockableSuperclass.class);
}).isInstanceOf(DoNotMockException.class);
}

@Test
public void can_not_mock_subclass_with_unmockable_interface_that_extends_non_mockable_interface() {
assertThatThrownBy(() -> {
SubclasOfSubInterfaceOfNotMockableInterface notMockable = mock(SubclasOfSubInterfaceOfNotMockableInterface.class);
}).isInstanceOf(DoNotMockException.class);
}

@Test
public void thrown_exception_includes_non_mockable_reason() {
assertThatThrownBy(() -> {
NotMockable notMockable = mock(NotMockable.class);
}).isInstanceOf(DoNotMockException.class).hasMessageContaining("Create a real instance instead");
}

@Test
public void thrown_exception_includes_special_non_mockable_reason() {
assertThatThrownBy(() -> {
NotMockableWithReason notMockable = mock(NotMockableWithReason.class);
}).isInstanceOf(DoNotMockException.class).hasMessageContaining("Special reason");
Copy link
Member

Choose a reason for hiding this comment

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

nice coverage, thx!

}

@Test
public void can_not_mock_class_with_custom_donotmock_annotation() {
assertThatThrownBy(() -> {
NotMockableWithDifferentAnnotation notMockable = mock(NotMockableWithDifferentAnnotation.class);
}).isInstanceOf(DoNotMockException.class);
}

@DoNotMock
private static class NotMockable {

}

@DoNotMock
private interface NotMockableInterface {

}

@org.mockitousage.annotation.org.mockito.DoNotMock
private static class NotMockableWithDifferentAnnotation {}

@DoNotMock("Special reason")
private interface NotMockableWithReason {}

static class SubclassOfNotMockableInterface implements NotMockableInterface {}

private static class DoubleSubclassOfInterface extends SubclassOfNotMockableInterface {}

private static class SubclassOfNotMockableSuperclass extends NotMockable {}

private interface SubInterfaceOfNotMockableInterface extends NotMockableInterface {}

private static class SubclasOfSubInterfaceOfNotMockableInterface implements SubInterfaceOfNotMockableInterface {}

private static class TestClass {
@Mock private NotMockable notMockable;
}

}
@@ -0,0 +1,21 @@
/*
* Copyright (c) 2019 Mockito contributors
* This program is made available under the terms of the MIT License.
*/
package org.mockitousage.annotation.org.mockito;

import static java.lang.annotation.ElementType.TYPE;
import static java.lang.annotation.RetentionPolicy.RUNTIME;

import java.lang.annotation.Retention;
import java.lang.annotation.Target;

/**
* Test to make sure that we are matching on name rather than only {@link org.mockito.DoNotMock}
Copy link
Member

Choose a reason for hiding this comment

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

nice coverage

* type equality.
*/
@Target({TYPE})
@Retention(RUNTIME)
public @interface DoNotMock {

}