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

Improved sensitivity of potential stubbing problem #1539

Merged
merged 2 commits into from Nov 25, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/main/java/org/mockito/internal/debugging/LocationImpl.java
Expand Up @@ -16,6 +16,7 @@ public class LocationImpl implements Location, Serializable {

private final Throwable stackTraceHolder;
private final StackTraceFilter stackTraceFilter;
private final String sourceFile;

public LocationImpl() {
this(defaultStackTraceFilter);
Expand All @@ -32,6 +33,13 @@ public LocationImpl(Throwable stackTraceHolder) {
private LocationImpl(StackTraceFilter stackTraceFilter, Throwable stackTraceHolder) {
this.stackTraceFilter = stackTraceFilter;
this.stackTraceHolder = stackTraceHolder;
if (stackTraceHolder.getStackTrace() == null || stackTraceHolder.getStackTrace().length == 0) {
//there are corner cases where exception can have a null or empty stack trace
//for example, a custom exception can override getStackTrace() method
this.sourceFile = "<unknown source file>";
} else {
this.sourceFile = stackTraceFilter.findSourceFile(stackTraceHolder.getStackTrace(), "<unknown source file>");
}
}

@Override
Expand All @@ -43,4 +51,9 @@ public String toString() {
}
return "-> at " + filtered[0].toString();
}

@Override
public String getSourceFile() {
return sourceFile;
}
}
Expand Up @@ -37,4 +37,17 @@ public StackTraceElement[] filter(StackTraceElement[] target, boolean keepTop) {
StackTraceElement[] result = new StackTraceElement[filtered.size()];
return filtered.toArray(result);
}

/**
* Finds the source file of the target stack trace.
* Returns the default value if source file cannot be found.
*/
public String findSourceFile(StackTraceElement[] target, String defaultValue) {
for (StackTraceElement e : target) {
if (CLEANER.isIn(e)) {
return e.getFileName();
}
}
return defaultValue;
}
}
Expand Up @@ -57,8 +57,11 @@ private static List<Invocation> potentialArgMismatches(Invocation invocation, Co
List<Invocation> matchingStubbings = new LinkedList<Invocation>();
for (Stubbing s : stubbings) {
if (UnusedStubbingReporting.shouldBeReported(s)
&& s.getInvocation().getMethod().getName().equals(invocation.getMethod().getName())) {
matchingStubbings.add(s.getInvocation());
&& s.getInvocation().getMethod().getName().equals(invocation.getMethod().getName())
//If stubbing and invocation are in the same source file we assume they are in the test code,
// and we don't flag it as mismatch:
&& !s.getInvocation().getLocation().getSourceFile().equals(invocation.getLocation().getSourceFile())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This would not work if the setup is performed in a base class or helper class. Can we figure out a solution for these situations as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR is improving the current behavior a lot already. We can always improve it further without blocking this PR. Feel free to suggest a solution!

Copy link
Member Author

Choose a reason for hiding this comment

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

@TimvdLippe, thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have not been able to come up with a proper solution, other than using a different stubbing mechanism. I fear that, no matter how hard we try, we will keep on receiving issues on this feature, where users will run into all the edge-cases.

That said, this does improve the current situation, so we can push forward. I just think we have to solve the fundamental problem first to be able to prevent these kind of issues popping up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for review!

matchingStubbings.add(s.getInvocation());
}
}
return matchingStubbings;
Expand Down
14 changes: 13 additions & 1 deletion src/main/java/org/mockito/invocation/Location.java
Expand Up @@ -4,14 +4,26 @@
*/
package org.mockito.invocation;

import org.mockito.NotExtensible;

/**
* Describes the location of something in the source code.
*/
@NotExtensible
public interface Location {

/**
* @return the location
* Human readable location in the source code, see {@link Invocation#getLocation()}
*
* @return location
*/
String toString();

/**
* Source file of this location
*
* @return source file
* @since 2.23.5
*/
String getSourceFile();
}
Expand Up @@ -130,6 +130,9 @@ public InvocationBuilder location(final String location) {
public String toString() {
return location;
}
public String getSourceFile() {
return "SomeClass";
}
};
return this;
}
Expand Down
Expand Up @@ -4,13 +4,15 @@
*/
package org.mockitousage.internal.debugging;

import static org.assertj.core.api.Assertions.assertThat;

import org.junit.Test;
import org.mockito.internal.debugging.LocationImpl;
import org.mockito.internal.exceptions.stacktrace.StackTraceFilter;
import org.mockitoutil.TestBase;

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

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.Assert.assertEquals;

@SuppressWarnings("serial")
Expand All @@ -37,4 +39,18 @@ public StackTraceElement[] filter(StackTraceElement[] target, boolean keepTop) {
//then
assertEquals("-> at <<unknown line>>", loc);
}

@Test
public void provides_location_class() {
//when
final List<String> files = new ArrayList<String>();
new Runnable() { //anonymous inner class adds stress to the check
public void run() {
files.add(new LocationImpl().getSourceFile());
}
}.run();

//then
assertEquals("LocationImplTest.java", files.get(0));
}
}
Expand Up @@ -14,6 +14,7 @@
import org.mockito.junit.MockitoJUnit;
import org.mockito.quality.Strictness;
import org.mockitousage.IMethods;
import org.mockitousage.strictness.ProductionCode;
import org.mockitoutil.SafeJUnitRule;

import static org.junit.Assert.assertEquals;
Expand Down Expand Up @@ -75,7 +76,7 @@ public class StrictJUnitRuleTest {

//when
when(mock.simpleMethod(10)).thenReturn("10");
when(mock.simpleMethod(20)).thenReturn("20");
ProductionCode.simpleMethod(mock, 20);
}

@Test public void fails_fast_when_stubbing_invoked_with_different_argument() throws Throwable {
Expand All @@ -87,7 +88,7 @@ public void doAssert(Throwable t) {
"Strict stubbing argument mismatch. Please check:\n" +
" - this invocation of 'simpleMethod' method:\n" +
" mock.simpleMethod(15);\n" +
" -> at org.mockitousage.junitrule.StrictJUnitRuleTest.fails_fast_when_stubbing_invoked_with_different_argument(StrictJUnitRuleTest.java:0)\n" +
" -> at org.mockitousage.strictness.ProductionCode.simpleMethod(ProductionCode.java:0)\n" +
" - has following stubbing(s) with different arguments:\n" +
" 1. mock.simpleMethod(20);\n" +
" -> at org.mockitousage.junitrule.StrictJUnitRuleTest.fails_fast_when_stubbing_invoked_with_different_argument(StrictJUnitRuleTest.java:0)\n" +
Expand Down Expand Up @@ -116,7 +117,7 @@ public void doAssert(Throwable t) {

//invocation in the code under test uses different argument and should fail immediately
//this helps with debugging and is essential for Mockito strictness
mock.simpleMethod(15);
ProductionCode.simpleMethod(mock, 15);
}

@Test public void verify_no_more_interactions_ignores_stubs() throws Throwable {
Expand Down
Expand Up @@ -13,6 +13,7 @@
import org.mockito.exceptions.misusing.UnnecessaryStubbingException;
import org.mockito.junit.MockitoJUnitRunner;
import org.mockitousage.IMethods;
import org.mockitousage.strictness.ProductionCode;
import org.mockitoutil.JUnitResultAssert;
import org.mockitoutil.TestBase;

Expand Down Expand Up @@ -65,7 +66,7 @@ public static class StubbingArgMismatch {
}
@Test public void argument_mismatch() {
when(mock.simpleMethod(10)).thenReturn("");
mock.simpleMethod(20);
ProductionCode.simpleMethod(mock, 20);
}
}
}
Expand Up @@ -30,12 +30,12 @@ public void mock_is_lenient() {
when(regularMock.simpleMethod("2")).thenReturn("2");

//then lenient mock does not throw:
lenientMock.simpleMethod("3");
ProductionCode.simpleMethod(lenientMock, "3");

//but regular mock throws:
Assertions.assertThatThrownBy(new ThrowableAssert.ThrowingCallable() {
public void call() {
regularMock.simpleMethod("4");
ProductionCode.simpleMethod(regularMock,"4");
}
}).isInstanceOf(PotentialStubbingProblem.class);
}
Expand Down
@@ -0,0 +1,60 @@
/*
* Copyright (c) 2007 Mockito contributors
* This program is made available under the terms of the MIT License.
*/

package org.mockitousage.strictness;

import org.assertj.core.api.Assertions;
import org.assertj.core.api.ThrowableAssert;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.mockito.Mock;
import org.mockito.exceptions.misusing.PotentialStubbingProblem;
import org.mockito.junit.MockitoJUnit;
import org.mockito.junit.MockitoRule;
import org.mockito.quality.Strictness;
import org.mockitousage.IMethods;

import static org.mockito.Mockito.when;

public class PotentialStubbingSensitivityTest {

@Rule public MockitoRule mockito = MockitoJUnit.rule().strictness(Strictness.STRICT_STUBS);
@Mock IMethods mock;

@Before
public void setup() {
when(mock.simpleMethod("1")).thenReturn("1");
}

@Test
public void allows_stubbing_with_different_arg_in_test_code() {
//although we are calling 'simpleMethod' with different argument
//Mockito understands that this is stubbing in the test code and does not trigger PotentialStubbingProblem
when(mock.simpleMethod("2")).thenReturn("2");

//methods in anonymous inner classes are ok, too
new Runnable() {
public void run() {
when(mock.simpleMethod("3")).thenReturn("3");
}
}.run();

//avoiding unnecessary stubbing failures:
mock.simpleMethod("1");
mock.simpleMethod("2");
mock.simpleMethod("3");
}

@Test
public void reports_potential_stubbing_problem_in_production_code() {
//when
Assertions.assertThatThrownBy(new ThrowableAssert.ThrowingCallable() {
public void call() throws Throwable {
ProductionCode.simpleMethod(mock, "2");
}
}).isInstanceOf(PotentialStubbingProblem.class);
}
}
23 changes: 23 additions & 0 deletions src/test/java/org/mockitousage/strictness/ProductionCode.java
@@ -0,0 +1,23 @@
/*
* Copyright (c) 2007 Mockito contributors
* This program is made available under the terms of the MIT License.
*/

package org.mockitousage.strictness;

import org.mockitousage.IMethods;

/**
* Test utility class that simulates invocation of mock in production code.
* In certain tests, the production code needs to be invoked in a different class/source file than the test.
*/
public class ProductionCode {

public static void simpleMethod(IMethods mock, String argument) {
mock.simpleMethod(argument);
}

public static void simpleMethod(IMethods mock, int argument) {
mock.simpleMethod(argument);
}
}
Expand Up @@ -60,9 +60,8 @@ public void potential_stubbing_problem() {

//and on strict stub mock (created by session), we cannot call stubbed method with different arg:
Assertions.assertThatThrownBy(new ThrowableAssert.ThrowingCallable() {
@Override
public void call() throws Throwable {
strictStubsMock.simpleMethod(200);
ProductionCode.simpleMethod(strictStubsMock, 200);
}
}).isInstanceOf(PotentialStubbingProblem.class);
}
Expand Down
Expand Up @@ -51,7 +51,7 @@ public void potential_stubbing_problem() {
assertThatThrownBy(new ThrowableAssert.ThrowingCallable() {
@Override
public void call() throws Throwable {
mock.simpleMethod("100");
ProductionCode.simpleMethod(mock, "100");
}
}).isInstanceOf(PotentialStubbingProblem.class);
}
Expand Down
Expand Up @@ -33,9 +33,8 @@ public void potential_stubbing_problem() {

//but on strict stubbing, we cannot:
assertThatThrownBy(new ThrowableAssert.ThrowingCallable() {
@Override
public void call() throws Throwable {
mock.simpleMethod("100");
public void call() {
ProductionCode.simpleMethod(mock, "100");
}
}).isInstanceOf(PotentialStubbingProblem.class);

Expand Down
Expand Up @@ -34,9 +34,8 @@ public void strictness_per_mock() {
//then previous mock is strict:
when(mock.simpleMethod(1)).thenReturn("1");
assertThatThrownBy(new ThrowableAssert.ThrowingCallable() {
@Override
public void call() throws Throwable {
mock.simpleMethod(2);
public void call() {
ProductionCode.simpleMethod(mock, 2);
}
}).isInstanceOf(PotentialStubbingProblem.class);

Expand All @@ -54,9 +53,8 @@ public void strictness_per_stubbing() {
//then previous mock is strict:
when(mock.simpleMethod(1)).thenReturn("1");
assertThatThrownBy(new ThrowableAssert.ThrowingCallable() {
@Override
public void call() throws Throwable {
mock.simpleMethod(2);
public void call() {
ProductionCode.simpleMethod(mock, 2);
}
}).isInstanceOf(PotentialStubbingProblem.class);

Expand Down
Expand Up @@ -35,9 +35,8 @@ public void potential_stubbing_problem() {

//but on strict stubbing, we cannot:
assertThatThrownBy(new ThrowableAssert.ThrowingCallable() {
@Override
public void call() throws Throwable {
mock.simpleMethod("100");
public void call() {
ProductionCode.simpleMethod(mock, "100");
}
}).isInstanceOf(PotentialStubbingProblem.class);

Expand Down
Expand Up @@ -17,6 +17,7 @@
import org.mockito.exceptions.misusing.UnnecessaryStubbingException;
import org.mockito.quality.Strictness;
import org.mockitousage.IMethods;
import org.mockitousage.strictness.ProductionCode;

import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
Expand Down Expand Up @@ -102,7 +103,7 @@ public static class ReportMismatchButNotUnusedStubbing {

@Test public void mismatch() {
given(mock.simpleMethod(1)).willReturn("");
mock.simpleMethod(2);
ProductionCode.simpleMethod(mock, 2);
}
}

Expand Down
Expand Up @@ -14,6 +14,7 @@
import org.mockito.exceptions.verification.NoInteractionsWanted;
import org.mockito.quality.Strictness;
import org.mockitousage.IMethods;
import org.mockitousage.strictness.ProductionCode;

import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.verify;
Expand Down Expand Up @@ -82,7 +83,7 @@ public void run() {
//stubbing argument mismatch is detected
assertThat(new Runnable() {
public void run() {
mock.simpleMethod(200);
ProductionCode.simpleMethod(mock, 200);
}
}).throwsException(PotentialStubbingProblem.class);
}
Expand Down