Honor presence of @WebAppConfiguration in ServTEL

The previous commit for issue SPR-11144 revealed a bug in
ServletTestExecutionListener (STEL). Specifically, STEL acted on the
fact that the ApplicationContext for a given TestContext was an
instance of WebApplicationContext. This behavior could potentially
break test code from previous releases of the Spring Framework that
relied on a custom setup of the RequestAttributes in the
RequestContextHolder with a custom WebApplicationContext ContextLoader.

This commit addresses this issue by ensuring that STEL only comes into
play if the test class is annotated with @WebAppConfiguration (for
prepareTestInstance() and beforeTestMethod()) or if the TestContext
attribute named RESET_REQUEST_CONTEXT_HOLDER_ATTRIBUTE is set to
Boolean.TRUE (for afterTestMethod()).

Issue: SPR-11144

spring-test/src/main/java/org/springframework/test/context/web/ServletTestExecutionListener.java

@@ -25,12 +25,14 @@ import org.springframework.beans.factory.config.ConfigurableListableBeanFactory;
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.ConfigurableApplicationContext;
 import org.springframework.core.Conventions;
+import org.springframework.core.annotation.AnnotationUtils;
 import org.springframework.mock.web.MockHttpServletRequest;
 import org.springframework.mock.web.MockHttpServletResponse;
 import org.springframework.mock.web.MockServletContext;
 import org.springframework.test.context.TestContext;
 import org.springframework.test.context.TestExecutionListener;
 import org.springframework.test.context.support.AbstractTestExecutionListener;
+import org.springframework.util.Assert;
 import org.springframework.web.context.WebApplicationContext;
 import org.springframework.web.context.request.RequestContextHolder;
 import org.springframework.web.context.request.ServletWebRequest;
@@ -52,8 +54,9 @@ import org.springframework.web.context.request.ServletWebRequest;
  * #afterTestMethod(TestContext) cleans up} thread-local state.
  *
  * <p>Note that {@code ServletTestExecutionListener} is enabled by default but
- * takes no action if the {@link ApplicationContext} loaded for the current test
- * is not a {@link WebApplicationContext}.
+ * generally takes no action if the {@linkplain TestContext#getTestClass() test
+ * class} is not annotated with {@link WebAppConfiguration @WebAppConfiguration}.
+ * See the Javadoc for individual methods in this class for details.
  *
  * @author Sam Brannen
  * @since 3.2
@@ -76,7 +79,9 @@ public class ServletTestExecutionListener extends AbstractTestExecutionListener
 
 	/**
 	 * Sets up thread-local state during the <em>test instance preparation</em>
-	 * callback phase via Spring Web's {@link RequestContextHolder}.
+	 * callback phase via Spring Web's {@link RequestContextHolder}, but only if
+	 * the {@linkplain TestContext#getTestClass() test class} is annotated with
+	 * {@link WebAppConfiguration @WebAppConfiguration}.
 	 *
 	 * @see TestExecutionListener#prepareTestInstance(TestContext)
 	 * @see #setUpRequestContextIfNecessary(TestContext)
@@ -88,7 +93,9 @@ public class ServletTestExecutionListener extends AbstractTestExecutionListener
 
 	/**
 	 * Sets up thread-local state before each test method via Spring Web's
-	 * {@link RequestContextHolder}.
+	 * {@link RequestContextHolder}, but only if the
+	 * {@linkplain TestContext#getTestClass() test class} is annotated with
+	 * {@link WebAppConfiguration @WebAppConfiguration}.
 	 *
 	 * @see TestExecutionListener#beforeTestMethod(TestContext)
 	 * @see #setUpRequestContextIfNecessary(TestContext)
@@ -101,7 +108,11 @@ public class ServletTestExecutionListener extends AbstractTestExecutionListener
 	/**
 	 * Cleans up thread-local state after each test method by {@linkplain
 	 * RequestContextHolder#resetRequestAttributes() resetting} Spring Web's
-	 * {@code RequestContextHolder}.
+	 * {@code RequestContextHolder}, but only if the {@link
+	 * #RESET_REQUEST_CONTEXT_HOLDER_ATTRIBUTE} in the supplied {@code TestContext}
+	 * has a value of {@link Boolean#TRUE}.
+	 * <p>The {@link #RESET_REQUEST_CONTEXT_HOLDER_ATTRIBUTE} will be
+	 * subsequently removed from the test context, regardless of its value.
 	 *
 	 * @see TestExecutionListener#afterTestMethod(TestContext)
 	 */
@@ -112,22 +123,27 @@ public class ServletTestExecutionListener extends AbstractTestExecutionListener
 				logger.debug(String.format("Resetting RequestContextHolder for test context %s.", testContext));
 			}
 			RequestContextHolder.resetRequestAttributes();
-			testContext.removeAttribute(RESET_REQUEST_CONTEXT_HOLDER_ATTRIBUTE);
 		}
+		testContext.removeAttribute(RESET_REQUEST_CONTEXT_HOLDER_ATTRIBUTE);
+	}
+
+	private boolean notAnnotatedWithWebAppConfiguration(TestContext testContext) {
+		return AnnotationUtils.findAnnotation(testContext.getTestClass(), WebAppConfiguration.class) == null;
 	}
 
 	private void setUpRequestContextIfNecessary(TestContext testContext) {
+		if (notAnnotatedWithWebAppConfiguration(testContext)) {
+			return;
+		}
 
 		ApplicationContext context = testContext.getApplicationContext();
 
 		if (context instanceof WebApplicationContext) {
 			WebApplicationContext wac = (WebApplicationContext) context;
 			ServletContext servletContext = wac.getServletContext();
-			if (!(servletContext instanceof MockServletContext)) {
-				throw new IllegalStateException(String.format(
-					"The WebApplicationContext for test context %s must be configured with a MockServletContext.",
-					testContext));
-			}
+			Assert.state(servletContext instanceof MockServletContext, String.format(
+				"The WebApplicationContext for test context %s must be configured with a MockServletContext.",
+				testContext));
 
 			if (logger.isDebugEnabled()) {
 				logger.debug(String.format(
@@ -135,22 +151,20 @@ public class ServletTestExecutionListener extends AbstractTestExecutionListener
 					testContext));
 			}
 
-			if (RequestContextHolder.getRequestAttributes() == null) {
-				MockServletContext mockServletContext = (MockServletContext) servletContext;
-				MockHttpServletRequest request = new MockHttpServletRequest(mockServletContext);
-				MockHttpServletResponse response = new MockHttpServletResponse();
-				ServletWebRequest servletWebRequest = new ServletWebRequest(request, response);
-
-				RequestContextHolder.setRequestAttributes(servletWebRequest);
-				testContext.setAttribute(RESET_REQUEST_CONTEXT_HOLDER_ATTRIBUTE, Boolean.TRUE);
-
-				if (wac instanceof ConfigurableApplicationContext) {
-					@SuppressWarnings("resource")
-					ConfigurableApplicationContext configurableApplicationContext = (ConfigurableApplicationContext) wac;
-					ConfigurableListableBeanFactory bf = configurableApplicationContext.getBeanFactory();
-					bf.registerResolvableDependency(MockHttpServletResponse.class, response);
-					bf.registerResolvableDependency(ServletWebRequest.class, servletWebRequest);
-				}
+			MockServletContext mockServletContext = (MockServletContext) servletContext;
+			MockHttpServletRequest request = new MockHttpServletRequest(mockServletContext);
+			MockHttpServletResponse response = new MockHttpServletResponse();
+			ServletWebRequest servletWebRequest = new ServletWebRequest(request, response);
+
+			RequestContextHolder.setRequestAttributes(servletWebRequest);
+			testContext.setAttribute(RESET_REQUEST_CONTEXT_HOLDER_ATTRIBUTE, Boolean.TRUE);
+
+			if (wac instanceof ConfigurableApplicationContext) {
+				@SuppressWarnings("resource")
+				ConfigurableApplicationContext configurableApplicationContext = (ConfigurableApplicationContext) wac;
+				ConfigurableListableBeanFactory bf = configurableApplicationContext.getBeanFactory();
+				bf.registerResolvableDependency(MockHttpServletResponse.class, response);
+				bf.registerResolvableDependency(ServletWebRequest.class, servletWebRequest);
 			}
 		}
 	}

spring-test/src/test/java/org/springframework/test/context/web/ServletTestExecutionListenerTests.java

@@ -18,6 +18,7 @@ package org.springframework.test.context.web;
 
 import org.junit.Before;
 import org.junit.Test;
+import org.mockito.Mockito;
 import org.springframework.context.ApplicationContext;
 import org.springframework.mock.web.MockHttpServletRequest;
 import org.springframework.mock.web.MockHttpServletResponse;
@@ -48,6 +49,14 @@ public class ServletTestExecutionListenerTests {
 	private final ServletTestExecutionListener listener = new ServletTestExecutionListener();
 
 
+	private void assertAttributesAvailable() {
+		assertNotNull("request attributes should be available", RequestContextHolder.getRequestAttributes());
+	}
+
+	private void assertAttributesNotAvailable() {
+		assertNull("request attributes should not be available", RequestContextHolder.getRequestAttributes());
+	}
+
 	private void assertAttributeExists() {
 		RequestAttributes requestAttributes = RequestContextHolder.getRequestAttributes();
 		assertNotNull("request attributes should exist", requestAttributes);
@@ -80,9 +89,13 @@ public class ServletTestExecutionListenerTests {
 	}
 
 	@Test
-	public void withStandardApplicationContext() throws Exception {
+	public void standardApplicationContext() throws Exception {
+		Mockito.<Class<?>> when(testContext.getTestClass()).thenReturn(getClass());
 		when(testContext.getApplicationContext()).thenReturn(mock(ApplicationContext.class));
 
+		listener.beforeTestClass(testContext);
+		assertAttributeExists();
+
 		listener.prepareTestInstance(testContext);
 		assertAttributeExists();
 
@@ -94,26 +107,33 @@ public class ServletTestExecutionListenerTests {
 	}
 
 	@Test
-	public void withWebApplicationContextWithoutExistingRequestAttributes() throws Exception {
-		assertAttributeExists();
+	public void legacyWebTestCaseWithoutExistingRequestAttributes() throws Exception {
+		Mockito.<Class<?>> when(testContext.getTestClass()).thenReturn(LegacyWebTestCase.class);
+
 		RequestContextHolder.resetRequestAttributes();
+		assertAttributesNotAvailable();
+
+		listener.beforeTestClass(testContext);
 
 		listener.prepareTestInstance(testContext);
-		assertAttributeDoesNotExist();
-		verify(testContext).setAttribute(RESET_REQUEST_CONTEXT_HOLDER_ATTRIBUTE, Boolean.TRUE);
-		when(testContext.getAttribute(RESET_REQUEST_CONTEXT_HOLDER_ATTRIBUTE)).thenReturn(Boolean.TRUE);
+		assertAttributesNotAvailable();
+		verify(testContext, times(0)).setAttribute(RESET_REQUEST_CONTEXT_HOLDER_ATTRIBUTE, Boolean.TRUE);
+		when(testContext.getAttribute(RESET_REQUEST_CONTEXT_HOLDER_ATTRIBUTE)).thenReturn(null);
 
 		listener.beforeTestMethod(testContext);
-		assertAttributeDoesNotExist();
-		verify(testContext).setAttribute(RESET_REQUEST_CONTEXT_HOLDER_ATTRIBUTE, Boolean.TRUE);
+		assertAttributesNotAvailable();
+		verify(testContext, times(0)).setAttribute(RESET_REQUEST_CONTEXT_HOLDER_ATTRIBUTE, Boolean.TRUE);
 
 		listener.afterTestMethod(testContext);
-		verify(testContext).removeAttribute(RESET_REQUEST_CONTEXT_HOLDER_ATTRIBUTE);
-		assertNull("request attributes should have been cleared", RequestContextHolder.getRequestAttributes());
+		verify(testContext, times(1)).removeAttribute(RESET_REQUEST_CONTEXT_HOLDER_ATTRIBUTE);
+		assertAttributesNotAvailable();
 	}
 
 	@Test
-	public void withWebApplicationContextWithPresetRequestAttributes() throws Exception {
+	public void legacyWebTestCaseWithPresetRequestAttributes() throws Exception {
+		Mockito.<Class<?>> when(testContext.getTestClass()).thenReturn(LegacyWebTestCase.class);
+
+		listener.beforeTestClass(testContext);
 		assertAttributeExists();
 
 		listener.prepareTestInstance(testContext);
@@ -127,7 +147,52 @@ public class ServletTestExecutionListenerTests {
 		when(testContext.getAttribute(RESET_REQUEST_CONTEXT_HOLDER_ATTRIBUTE)).thenReturn(null);
 
 		listener.afterTestMethod(testContext);
-		verify(testContext, times(0)).removeAttribute(RESET_REQUEST_CONTEXT_HOLDER_ATTRIBUTE);
+		verify(testContext, times(1)).removeAttribute(RESET_REQUEST_CONTEXT_HOLDER_ATTRIBUTE);
+		assertAttributeExists();
+	}
+
+	@Test
+	public void atWebAppConfigTestCaseWithoutExistingRequestAttributes() throws Exception {
+		Mockito.<Class<?>> when(testContext.getTestClass()).thenReturn(AtWebAppConfigWebTestCase.class);
+
+		RequestContextHolder.resetRequestAttributes();
+		listener.beforeTestClass(testContext);
+		assertAttributesNotAvailable();
+
+		assertWebAppConfigTestCase();
+	}
+
+	@Test
+	public void atWebAppConfigTestCaseWithPresetRequestAttributes() throws Exception {
+		Mockito.<Class<?>> when(testContext.getTestClass()).thenReturn(AtWebAppConfigWebTestCase.class);
+
+		listener.beforeTestClass(testContext);
+		assertAttributesAvailable();
+
+		assertWebAppConfigTestCase();
+	}
+
+	private void assertWebAppConfigTestCase() throws Exception {
+		listener.prepareTestInstance(testContext);
+		assertAttributeDoesNotExist();
+		verify(testContext, times(1)).setAttribute(RESET_REQUEST_CONTEXT_HOLDER_ATTRIBUTE, Boolean.TRUE);
+		when(testContext.getAttribute(RESET_REQUEST_CONTEXT_HOLDER_ATTRIBUTE)).thenReturn(Boolean.TRUE);
+
+		listener.beforeTestMethod(testContext);
+		assertAttributeDoesNotExist();
+		verify(testContext, times(2)).setAttribute(RESET_REQUEST_CONTEXT_HOLDER_ATTRIBUTE, Boolean.TRUE);
+
+		listener.afterTestMethod(testContext);
+		verify(testContext).removeAttribute(RESET_REQUEST_CONTEXT_HOLDER_ATTRIBUTE);
+		assertAttributesNotAvailable();
+	}
+
+
+	static class LegacyWebTestCase {
+	}
+
+	@WebAppConfiguration
+	static class AtWebAppConfigWebTestCase {
 	}
 
 }