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
29 changes: 29 additions & 0 deletions src/main/java/org/mockito/DoNotMock.java
@@ -0,0 +1,29 @@
/*
* 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.
*/
@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";
}
@@ -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);
}
}
24 changes: 24 additions & 0 deletions src/main/java/org/mockito/internal/MockitoCore.java
Expand Up @@ -15,12 +15,15 @@
import static org.mockito.internal.verification.VerificationModeFactory.noInteractions;
import static org.mockito.internal.verification.VerificationModeFactory.noMoreInteractions;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

import org.mockito.DoNotMock;
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.creation.MockSettingsImpl;
import org.mockito.internal.invocation.finder.VerifiableInvocationsFinder;
Expand Down Expand Up @@ -60,11 +63,32 @@ 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) {
ArrayList<Class<?>> unmockableTypes = new ArrayList<>(
creationSettings.getExtraInterfaces());

Class<?> mockTypeToCheck = typeToMock;
while (mockTypeToCheck != null && mockTypeToCheck != Object.class) {
Copy link
Member

Choose a reason for hiding this comment

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

I played with the code and noticed that "mockTypeToCheck != null" part of this statement is either: a) not needed or b) not covered with tests.

Do we need this part?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is covered by mocking an interface that directly has the DoNotMock annotation. I added a test to cover this case.

unmockableTypes.add(mockTypeToCheck);
Class<?>[] interfaces = mockTypeToCheck.getInterfaces();
if (interfaces != null) {
unmockableTypes.addAll(Arrays.asList(interfaces));
}
mockTypeToCheck = mockTypeToCheck.getSuperclass();
}

if (unmockableTypes.stream().anyMatch(clazz -> clazz.isAnnotationPresent(DoNotMock.class))) {
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 honor this specific annotation (Mockito API) or any annotation with name "DoNotMock".

The latter seems more useful. For example: a library owner can mark types with any "DoNotMock" annotation without adding compile dependency on mockito-core.

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 would like to keep this for now and open it up later to match on name. If we would implement that now, I would break all users of ErrorProne that rely on @DoNotMock but have grandfathered mock violations.

If we can gather feedback first, we can see adoption patterns and change it to match on the name later. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

How about a happy medium so that we don't need to revisit this in the future (and cause breaking changes :) - we can match for "org.mockito.DoNotMock" by String. This should be backwards compatible for G. and other companies / projects. Thoughts?

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

throw new DoNotMockException(creationSettings.getTypeToMock() + " is annotated with @DoNoMock and can't be mocked.");
}
}

public <T> OngoingStubbing<T> when(T methodCall) {
MockingProgress mockingProgress = mockingProgress();
mockingProgress.stubbingStarted();
Expand Down
73 changes: 73 additions & 0 deletions src/test/java/org/mockitousage/annotation/DoNotMockTest.java
@@ -0,0 +1,73 @@
/*
* 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);
}

@DoNotMock
private static class NotMockable {

}

@DoNotMock
private interface NotMockableInterface {

}

static class SubclassOfNotMockableInterface implements NotMockableInterface {}

private static class DoubleSubclassOfInterface extends SubclassOfNotMockableInterface {}

private static class SubclassOfNotMockableSuperclass extends NotMockable {}

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

}