Fix off-by-one regression in AbstractMethodMockingControl

This commit fixes the off-by-one regression accidentally introduced in
commit 55961544a7.

Specifically, this fix ensures that the correct recorded call is
indexed in the 'calls' list in the implementation of
AbstractMethodMockingControl.Expectations.nextCall().

In addition, this commit improves the Javadoc for
AbstractMethodMockingControl, @MockStaticEntityMethods, and
AnnotationDrivenStaticEntityMockingControl and introduces a proper
toString() implementation for the internal Expectations.Call class in
AbstractMethodMockingControl. Furthermore, code from the obsolete
Delegate test class has been inlined in
AnnotationDrivenStaticEntityMockingControlTests.

Issue: SPR-11385, SPR-10885

spring-aspects/src/main/java/org/springframework/mock/staticmock/AbstractMethodMockingControl.aj

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2013 the original author or authors.
+ * Copyright 2002-2014 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -18,16 +18,22 @@ package org.springframework.mock.staticmock;
 
 import java.util.Arrays;
 import java.util.LinkedList;
-import java.util.List;
+
+import org.springframework.util.ObjectUtils;
 
 /**
  * Abstract aspect to enable mocking of methods picked out by a pointcut.
- * Sub-aspects must define the mockStaticsTestMethod() pointcut to
+ *
- * indicate call stacks when mocking should be triggered, and the
+ * <p>Sub-aspects must define:
- * methodToMock() pointcut to pick out a method invocations to mock.
+ * <ul>
+ * <li>the {@link #mockStaticsTestMethod()} pointcut to indicate call stacks
+ * when mocking should be triggered
+ * <li>the {@link #methodToMock()} pointcut to pick out method invocations to mock
+ * </ul>
  *
  * @author Rod Johnson
  * @author Ramnivas Laddad
+ * @author Sam Brannen
  */
 public abstract aspect AbstractMethodMockingControl percflow(mockStaticsTestMethod()) {
 
@@ -35,24 +41,34 @@ public abstract aspect AbstractMethodMockingControl percflow(mockStaticsTestMeth
 
 	protected abstract pointcut methodToMock();
 
+
 	private boolean recording = true;
 
-	static enum CallResponse { nothing, return_, throw_ };
 
-	// Represents a list of expected calls to static entity methods
+	static enum CallResponse {
+		nothing, return_, throw_
+	};
+
+	/**
+	 * Represents a list of expected calls to methods.
+	 */
 	// Public to allow inserted code to access: is this normal??
 	public class Expectations {
 
-		// Represents an expected call to a static entity method
+		/**
+		 * Represents an expected call to a method.
+		 */
 		private class Call {
+
 			private final String signature;
 			private final Object[] args;
 
 			private Object responseObject; // return value or throwable
 			private CallResponse responseType = CallResponse.nothing;
 
-			public Call(String name, Object[] args) {
+
-				this.signature = name;
+			public Call(String signature, Object[] args) {
+				this.signature = signature;
 				this.args = args;
 			}
 
@@ -88,16 +104,29 @@ public abstract aspect AbstractMethodMockingControl percflow(mockStaticsTestMeth
 					throw new IllegalArgumentException("Arguments don't match");
 				}
 			}
+
+			@Override
+			public String toString() {
+				return String.format("Call with signature [%s] and arguments %s", this.signature,
+					ObjectUtils.nullSafeToString(args));
+			}
 		}
 
-		private List<Call> calls = new LinkedList<Call>();
 
-		// Calls already verified
+		/**
+		 * The list of recorded calls.
+		 */
+		private final LinkedList<Call> calls = new LinkedList<Call>();
+
+		/**
+		 * The number of calls already verified.
+		 */
 		private int verified;
 
+
 		public void verify() {
 			if (verified != calls.size()) {
-				throw new IllegalStateException("Expected " + calls.size() + " calls, received " + verified);
+				throw new IllegalStateException("Expected " + calls.size() + " calls, but received " + verified);
 			}
 		}
 
@@ -105,31 +134,32 @@ public abstract aspect AbstractMethodMockingControl percflow(mockStaticsTestMeth
 		 * Validate the call and provide the expected return value.
 		 */
 		public Object respond(String lastSig, Object[] args) {
-			Call call = nextCall();
+			Call c = nextCall();
-			CallResponse responseType = call.responseType;
+
-			if (responseType == CallResponse.return_) {
+			switch (c.responseType) {
-				return call.returnValue(lastSig, args);
+				case return_: {
+					return c.returnValue(lastSig, args);
+				}
+				case throw_: {
+					return c.throwException(lastSig, args);
 				}
-			else if (responseType == CallResponse.throw_) {
+				default: {
-				return call.throwException(lastSig, args);
+					throw new IllegalStateException("Behavior of " + c + " not specified");
 				}
-			else if (responseType == CallResponse.nothing) {
-				// do nothing
 			}
-			throw new IllegalStateException("Behavior of " + call + " not specified");
 		}
 
 		private Call nextCall() {
 			verified++;
 			if (verified > calls.size()) {
-				throw new IllegalStateException("Expected " + calls.size() + " calls, received " + verified);
+				throw new IllegalStateException("Expected " + calls.size() + " calls, but received " + verified);
 			}
-			return calls.get(verified);
+			// The 'verified' count is 1-based; whereas, 'calls' is 0-based.
+			return calls.get(verified - 1);
 		}
 
-		public void expectCall(String lastSig, Object lastArgs[]) {
+		public void expectCall(String lastSig, Object[] lastArgs) {
-			Call call = new Call(lastSig, lastArgs);
+			calls.add(new Call(lastSig, lastArgs));
-			calls.add(call);
 		}
 
 		public boolean hasCalls() {
@@ -137,23 +167,26 @@ public abstract aspect AbstractMethodMockingControl percflow(mockStaticsTestMeth
 		}
 
 		public void expectReturn(Object retVal) {
-			Call call = calls.get(calls.size() - 1);
+			Call c = calls.getLast();
-			if (call.hasResponseSpecified()) {
+			if (c.hasResponseSpecified()) {
-				throw new IllegalStateException("No static method invoked before setting return value");
+				throw new IllegalStateException("No method invoked before setting return value");
 			}
-			call.setReturnVal(retVal);
+			c.setReturnVal(retVal);
 		}
 
 		public void expectThrow(Throwable throwable) {
-			Call call = calls.get(calls.size() - 1);
+			Call c = calls.getLast();
-			if (call.hasResponseSpecified()) {
+			if (c.hasResponseSpecified()) {
-				throw new IllegalStateException("No static method invoked before setting throwable");
+				throw new IllegalStateException("No method invoked before setting throwable");
+			}
+			c.setThrow(throwable);
 		}
-			call.setThrow(throwable);
 		}
 	}
 
-	private Expectations expectations = new Expectations();
+
+	private final Expectations expectations = new Expectations();
+
 
 	after() returning : mockStaticsTestMethod() {
 		if (recording && (expectations.hasCalls())) {

spring-aspects/src/main/java/org/springframework/mock/staticmock/AnnotationDrivenStaticEntityMockingControl.aj

@@ -17,20 +17,21 @@
 package org.springframework.mock.staticmock;
 
 /**
- * Annotation-based aspect to use in test builds to enable mocking static methods
+ * Annotation-based aspect to use in test builds to enable mocking of static methods
- * on JPA-annotated {@code @Entity} classes, as used by Spring Roo for finders.
+ * on JPA-annotated {@code @Entity} classes, as used by Spring Roo for so-called
+ * <em>finder methods</em>.
  *
- * <p>Mocking will occur in the call stack of any method in a class (typically a test class)
+ * <p>Mocking will occur within the call stack of any method in a class (typically a
- * that is annotated with the {@code @MockStaticEntityMethods} annotation.
+ * test class) that is annotated with {@code @MockStaticEntityMethods}.
  *
- * <p>Also provides static methods to simplify the programming model for
+ * <p>This aspect also provides static methods to simplify the programming model for
- * entering playback mode and setting expected return values.
+ * setting expectations and entering playback mode.
  *
  * <p>Usage:
  * <ol>
  * <li>Annotate a test class with {@code @MockStaticEntityMethods}.
  * <li>In each test method, {@code AnnotationDrivenStaticEntityMockingControl}
- * will begin in recording mode.
+ * will begin in <em>recording</em> mode.
  * <li>Invoke static methods on JPA-annotated {@code @Entity} classes, with each
  * recording-mode invocation being followed by an invocation of either the static
  * {@link #expectReturn(Object)} method or the static {@link #expectThrow(Throwable)}
@@ -48,22 +49,37 @@ package org.springframework.mock.staticmock;
 public aspect AnnotationDrivenStaticEntityMockingControl extends AbstractMethodMockingControl {
 
 	/**
-	 * Stop recording mock calls and enter playback state
+	 * Expect the supplied {@link Object} to be returned by the previous static
+	 * method invocation.
+	 * @see #playback()
 	 */
-	public static void playback() {
-		AnnotationDrivenStaticEntityMockingControl.aspectOf().playbackInternal();
-	}
-
 	public static void expectReturn(Object retVal) {
 		AnnotationDrivenStaticEntityMockingControl.aspectOf().expectReturnInternal(retVal);
 	}
 
+	/**
+	 * Expect the supplied {@link Throwable} to be thrown by the previous static
+	 * method invocation.
+	 * @see #playback()
+	 */
 	public static void expectThrow(Throwable throwable) {
 		AnnotationDrivenStaticEntityMockingControl.aspectOf().expectThrowInternal(throwable);
 	}
 
-	// Only matches directly annotated @Test methods, to allow methods in
+	/**
-	// @MockStatics classes to invoke each other without resetting the mocking environment
+	 * Stop recording mock expectations and enter <em>playback</em> mode.
+	 * @see #expectReturn(Object)
+	 * @see #expectThrow(Throwable)
+	 */
+	public static void playback() {
+		AnnotationDrivenStaticEntityMockingControl.aspectOf().playbackInternal();
+	}
+	
+	// Apparently, the following pointcut was originally defined to only match
+	// methods directly annotated with @Test (in order to allow methods in
+	// @MockStaticEntityMethods classes to invoke each other without resetting
+	// the mocking environment); however, this is no longer the case. The current
+	// pointcut applies to all public methods in @MockStaticEntityMethods classes.
 	protected pointcut mockStaticsTestMethod() : execution(public * (@MockStaticEntityMethods *).*(..));
 
 	protected pointcut methodToMock() : execution(public static * (@javax.persistence.Entity *).*(..));

spring-aspects/src/main/java/org/springframework/mock/staticmock/MockStaticEntityMethods.java

@@ -22,12 +22,13 @@ import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;
 
 /**
- * Annotation to indicate a test class for whose {code @Test} methods
+ * Annotation to indicate a test class for whose {@code @Test} methods
  * static methods on JPA-annotated {@code @Entity} classes should be mocked.
  *
- * <p>See {@code AnnotationDrivenStaticEntityMockingControl} for details.
+ * <p>See {@link AnnotationDrivenStaticEntityMockingControl} for details.
  *
  * @author Rod Johnson
+ * @author Sam Brannen
  */
 @Retention(RetentionPolicy.RUNTIME)
 @Target(ElementType.TYPE)

spring-aspects/src/test/java/org/springframework/mock/staticmock/AnnotationDrivenStaticEntityMockingControlTests.java

@@ -16,16 +16,18 @@
 
 package org.springframework.mock.staticmock;
 
+import java.rmi.RemoteException;
+
 import javax.persistence.PersistenceException;
 
-import org.junit.Ignore;
 import org.junit.Test;
 
 import static org.junit.Assert.*;
 import static org.springframework.mock.staticmock.AnnotationDrivenStaticEntityMockingControl.*;
 
 /**
- * Tests for static entity mocking framework.
+ * Tests for Spring's static entity mocking framework (i.e., @{@link MockStaticEntityMethods}
+ * and {@link AnnotationDrivenStaticEntityMockingControl}).
  * 
  * @author Rod Johnson
  * @author Ramnivas Laddad
@@ -34,10 +36,8 @@ import static org.springframework.mock.staticmock.AnnotationDrivenStaticEntityMo
 @MockStaticEntityMethods
 public class AnnotationDrivenStaticEntityMockingControlTests {
 
-	// TODO Fix failing test
-	@Ignore
 	@Test
-	public void noArgIntReturn() {
+	public void noArgumentMethodInvocationReturnsInt() {
 		int expectedCount = 13;
 		Person.countPeople();
 		expectReturn(expectedCount);
@@ -45,20 +45,16 @@ public class AnnotationDrivenStaticEntityMockingControlTests {
 		assertEquals(expectedCount, Person.countPeople());
 	}
 
-	// TODO Fix failing test
-	@Ignore
 	@Test(expected = PersistenceException.class)
-	public void noArgThrows() {
+	public void noArgumentMethodInvocationThrowsException() {
 		Person.countPeople();
 		expectThrow(new PersistenceException());
 		playback();
 		Person.countPeople();
 	}
 
-	// TODO Fix failing test
-	@Ignore
 	@Test
-	public void argMethodMatches() {
+	public void methodArgumentsMatch() {
 		long id = 13;
 		Person found = new Person();
 		Person.findPerson(id);
@@ -67,8 +63,6 @@ public class AnnotationDrivenStaticEntityMockingControlTests {
 		assertEquals(found, Person.findPerson(id));
 	}
 
-	// TODO Fix failing test
-	@Ignore
 	@Test
 	public void longSeriesOfCalls() {
 		long id1 = 13;
@@ -91,36 +85,26 @@ public class AnnotationDrivenStaticEntityMockingControlTests {
 		assertEquals(0, Person.countPeople());
 	}
 
-	/**
-	 * Note delegation is used when tests are invalid and should fail, as otherwise the
-	 * failure will occur on the verify() method in the aspect after this method returns,
-	 * failing the test case.
-	 */
-	// TODO Fix failing test
-	@Ignore
-	@Test
-	public void argMethodNoMatchExpectReturn() {
-		try {
-			new Delegate().argMethodNoMatchExpectReturn();
-			fail();
-		}
-		catch (IllegalArgumentException expected) {
-		}
-	}
-
-	// TODO Fix failing test
-	@Ignore
 	@Test(expected = IllegalArgumentException.class)
-	public void argMethodNoMatchExpectThrow() {
+	public void methodArgumentsDoNotMatchAndReturnsObject() {
-		new Delegate().argMethodNoMatchExpectThrow();
+		long id = 13;
+		Person found = new Person();
+		Person.findPerson(id);
+		AnnotationDrivenStaticEntityMockingControl.expectReturn(found);
+		AnnotationDrivenStaticEntityMockingControl.playback();
+		assertEquals(found, Person.findPerson(id + 1));
 	}
 
-	private void called(Person found, long id) {
+	@Test(expected = IllegalArgumentException.class)
-		assertEquals(found, Person.findPerson(id));
+	public void methodArgumentsDoNotMatchAndThrowsException() {
+		long id = 13;
+		Person found = new Person();
+		Person.findPerson(id);
+		AnnotationDrivenStaticEntityMockingControl.expectThrow(new PersistenceException());
+		AnnotationDrivenStaticEntityMockingControl.playback();
+		assertEquals(found, Person.findPerson(id + 1));
 	}
 
-	// TODO Fix failing test
-	@Ignore
 	@Test
 	public void reentrant() {
 		long id = 13;
@@ -131,31 +115,56 @@ public class AnnotationDrivenStaticEntityMockingControlTests {
 		called(found, id);
 	}
 
+	private void called(Person found, long id) {
+		assertEquals(found, Person.findPerson(id));
+	}
+
 	@Test(expected = IllegalStateException.class)
 	public void rejectUnexpectedCall() {
-		new Delegate().rejectUnexpectedCall();
+		AnnotationDrivenStaticEntityMockingControl.playback();
+		Person.countPeople();
 	}
 
-	// TODO Fix failing test
-	@Ignore
 	@Test(expected = IllegalStateException.class)
-	public void failTooFewCalls() {
+	public void tooFewCalls() {
-		new Delegate().failTooFewCalls();
+		long id = 13;
+		Person found = new Person();
+		Person.findPerson(id);
+		AnnotationDrivenStaticEntityMockingControl.expectReturn(found);
+		Person.countPeople();
+		AnnotationDrivenStaticEntityMockingControl.expectReturn(25);
+		AnnotationDrivenStaticEntityMockingControl.playback();
+		assertEquals(found, Person.findPerson(id));
 	}
 
 	@Test
 	public void empty() {
-		// Test that verification check doesn't blow up if no replay() call happened
+		// Test that verification check doesn't blow up if no replay() call happened.
 	}
 
 	@Test(expected = IllegalStateException.class)
-	public void doesntEverReplay() {
+	public void doesNotEnterPlaybackMode() {
-		new Delegate().doesntEverReplay();
+		Person.countPeople();
 	}
 
 	@Test(expected = IllegalStateException.class)
-	public void doesntEverSetReturn() {
+	public void doesNotSetExpectedReturnValue() {
-		new Delegate().doesntEverSetReturn();
+		Person.countPeople();
+		AnnotationDrivenStaticEntityMockingControl.playback();
+	}
+
+	/**
+	 * Note: this test method currently does NOT actually verify that the mock
+	 * verification fails.
+	 */
+	// TODO Determine if it's possible for a mock verification failure to fail a test in
+	// JUnit 4+ if the test method itself throws an expected exception.
+	@Test(expected = RemoteException.class)
+	public void verificationFailsEvenWhenTestFailsInExpectedManner() throws Exception {
+		Person.countPeople();
+		AnnotationDrivenStaticEntityMockingControl.playback();
+		// No calls in order to allow verification failure
+		throw new RemoteException();
 	}
 
 }

spring-aspects/src/test/java/org/springframework/mock/staticmock/Delegate.java

@@ -1,90 +0,0 @@
-/*
- * Copyright 2002-2014 the original author or authors.
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package org.springframework.mock.staticmock;
-
-import java.rmi.RemoteException;
-
-import javax.persistence.PersistenceException;
-
-import org.junit.Ignore;
-
-import static org.junit.Assert.*;
-
-/**
- * This isn't meant for direct testing; rather it is driven from
- * {@link AnnotationDrivenStaticEntityMockingControlTests}.
- * 
- * @author Rod Johnson
- * @author Ramnivas Laddad
- * @author Sam Brannen
- */
-@MockStaticEntityMethods
-@Ignore("Used because verification failures occur after method returns, so we can't test for them in the test case itself")
-public class Delegate {
-
-	public void argMethodNoMatchExpectReturn() {
-		long id = 13;
-		Person found = new Person();
-		Person.findPerson(id);
-		AnnotationDrivenStaticEntityMockingControl.expectReturn(found);
-		AnnotationDrivenStaticEntityMockingControl.playback();
-		assertEquals(found, Person.findPerson(id + 1));
-	}
-
-	public void argMethodNoMatchExpectThrow() {
-		long id = 13;
-		Person found = new Person();
-		Person.findPerson(id);
-		AnnotationDrivenStaticEntityMockingControl.expectThrow(new PersistenceException());
-		AnnotationDrivenStaticEntityMockingControl.playback();
-		assertEquals(found, Person.findPerson(id + 1));
-	}
-
-	public void failTooFewCalls() {
-		long id = 13;
-		Person found = new Person();
-		Person.findPerson(id);
-		AnnotationDrivenStaticEntityMockingControl.expectReturn(found);
-		Person.countPeople();
-		AnnotationDrivenStaticEntityMockingControl.expectReturn(25);
-		AnnotationDrivenStaticEntityMockingControl.playback();
-		assertEquals(found, Person.findPerson(id));
-	}
-
-	public void doesntEverReplay() {
-		Person.countPeople();
-	}
-
-	public void doesntEverSetReturn() {
-		Person.countPeople();
-		AnnotationDrivenStaticEntityMockingControl.playback();
-	}
-
-	public void rejectUnexpectedCall() {
-		AnnotationDrivenStaticEntityMockingControl.playback();
-		Person.countPeople();
-	}
-
-	public void verificationFailsEvenWhenTestFailsInExpectedManner()
-			throws RemoteException {
-		Person.countPeople();
-		AnnotationDrivenStaticEntityMockingControl.playback();
-		// No calls to allow verification failure
-		throw new RemoteException();
-	}
-
-}