Fix off-by-one regression in AbstractMethodMockingControl

Issue: SPR-11385, SPR-10885
(cherry picked from commits 69a89b1 and 3a89bc4)

Author: sbrannen

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,41 +18,57 @@ 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
- * methodToMock() pointcut to pick out a method invocations to mock.
+ *
+ * <p>Sub-aspects must define:
+ * <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()) {
 
 	protected abstract pointcut mockStaticsTestMethod();
 
 	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;
 			}
 
@@ -77,7 +93,7 @@ public abstract aspect AbstractMethodMockingControl percflow(mockStaticsTestMeth
 
 			public Object throwException(String lastSig, Object[] args) {
 				checkSignature(lastSig, args);
-				throw (RuntimeException)responseObject;
+				throw (RuntimeException) responseObject;
 			}
 
 			private void checkSignature(String lastSig, Object[] args) {
@@ -88,78 +104,94 @@ 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);
 			}
 		}
 
 		/**
 		 * Validate the call and provide the expected return value.
 		 */
 		public Object respond(String lastSig, Object[] args) {
-			Call call = nextCall();
-			CallResponse responseType = call.responseType;
-			if (responseType == CallResponse.return_) {
-				return call.returnValue(lastSig, args);
-			}
-			else if (responseType == CallResponse.throw_) {
-				return call.throwException(lastSig, args);
-			}
-			else if (responseType == CallResponse.nothing) {
-				// do nothing
+			Call c = nextCall();
+
+			switch (c.responseType) {
+				case return_: {
+					return c.returnValue(lastSig, args);
+				}
+				case throw_: {
+					return c.throwException(lastSig, args);
+				}
+				default: {
+					throw new IllegalStateException("Behavior of " + c + " not specified");
+				}
 			}
-			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[]) {
-			Call call = new Call(lastSig, lastArgs);
-			calls.add(call);
+		public void expectCall(String lastSig, Object[] lastArgs) {
+			calls.add(new Call(lastSig, lastArgs));
 		}
 
 		public boolean hasCalls() {
 			return !calls.isEmpty();
 		}
 
 		public void expectReturn(Object retVal) {
-			Call call = calls.get(calls.size() - 1);
-			if (call.hasResponseSpecified()) {
-				throw new IllegalStateException("No static method invoked before setting return value");
+			Call c = calls.getLast();
+			if (c.hasResponseSpecified()) {
+				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);
-			if (call.hasResponseSpecified()) {
-				throw new IllegalStateException("No static method invoked before setting throwable");
+			Call c = calls.getLast();
+			if (c.hasResponseSpecified()) {
+				throw new IllegalStateException("No method invoked before setting throwable");
 			}
-			call.setThrow(throwable);
+			c.setThrow(throwable);
 		}
 	}
 
-	private Expectations expectations = new Expectations();
+
+	private final Expectations expectations = new Expectations();
+
 
 	after() returning : mockStaticsTestMethod() {
 		if (recording && (expectations.hasCalls())) {
 			throw new IllegalStateException(
-					"Calls recorded, yet playback state never reached: Create expectations then call "
-							+ this.getClass().getSimpleName() + ".playback()");
+				"Calls recorded, yet playback state never reached: Create expectations then call "
+						+ this.getClass().getSimpleName() + ".playback()");
 		}
 		expectations.verify();
 	}

spring-aspects/src/main/java/org/springframework/mock/staticmock/AnnotationDrivenStaticEntityMockingControl.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.
@@ -17,50 +17,69 @@
 package org.springframework.mock.staticmock;
 
 /**
- * Annotation-based aspect to use in test build to enable mocking static methods
- * on JPA-annotated {@code @Entity} classes, as used by Roo for finders.
+ * 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 so-called
+ * <em>finder methods</em>.
  *
- * <p>Mocking will occur in the call stack of any method in a class (typically a test class)
- * that is annotated with the @MockStaticEntityMethods annotation.
+ * <p>Mocking will occur within the call stack of any method in a class (typically a
+ * test class) that is annotated with {@code @MockStaticEntityMethods}.
  *
- * <p>Also provides static methods to simplify the programming model for
- * entering playback mode and setting expected return values.
+ * <p>This aspect also provides static methods to simplify the programming model for
+ * setting expectations and entering playback mode.
  *
  * <p>Usage:
  * <ol>
- * <li>Annotate a test class with @MockStaticEntityMethods.
- * <li>In each test method, AnnotationDrivenStaticEntityMockingControl will begin in recording mode.
- * Invoke static methods on Entity classes, with each recording-mode invocation
- * being followed by an invocation to the static expectReturn() or expectThrow()
- * method on AnnotationDrivenStaticEntityMockingControl.
- * <li>Invoke the static AnnotationDrivenStaticEntityMockingControl() method.
- * <li>Call the code you wish to test that uses the static methods. Verification will
- * occur automatically.
+ * <li>Annotate a test class with {@code @MockStaticEntityMethods}.
+ * <li>In each test method, {@code AnnotationDrivenStaticEntityMockingControl}
+ * 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)}
+ * method on {@code AnnotationDrivenStaticEntityMockingControl}.
+ * <li>Invoke the static {@link #playback()} method.
+ * <li>Call the code you wish to test that uses the static methods.
+ * <li>Verification will occur automatically.
  * </ol>
  *
  * @author Rod Johnson
  * @author Ramnivas Laddad
+ * @author Sam Brannen
  * @see MockStaticEntityMethods
  */
 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

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