Rework @TestPropertySource compatibility

Rework @TestPropertySource changes introduced in commit d251b513
to restore compatibility with Spring Boot 1.1

- Only add the `server.port` property when no @IntegrationTest
  annotation is found.
- Always add a default `spring.jmx.enabled=false` property.
- Restore the SpringApplicationContextLoader.getEnvironmentProperties
  protected method.
- Remove the @IntegrationTest.properties attribute.

See gh-1697

spring-boot/src/main/java/org/springframework/boot/test/IntegrationTest.java

@@ -25,7 +25,6 @@ import java.lang.annotation.Target;
 
 import org.springframework.core.env.Environment;
 import org.springframework.test.context.TestExecutionListeners;
-import org.springframework.test.context.TestPropertySource;
 import org.springframework.test.context.support.DependencyInjectionTestExecutionListener;
 import org.springframework.test.context.support.DirtiesContextTestExecutionListener;
 import org.springframework.test.context.transaction.TransactionalTestExecutionListener;
@@ -42,21 +41,16 @@ import org.springframework.test.context.transaction.TransactionalTestExecutionLi
 @Target(ElementType.TYPE)
 // Leave out the ServletTestExecutionListener because it only deals with Mock* servlet
 // stuff. A real embedded application will not need the mocks.
-@TestExecutionListeners(listeners = { IntegrationTestPropertiesListener.class, DependencyInjectionTestExecutionListener.class,
+@TestExecutionListeners(listeners = { IntegrationTestPropertiesListener.class,
+		DependencyInjectionTestExecutionListener.class,
 		DirtiesContextTestExecutionListener.class,
 		TransactionalTestExecutionListener.class })
-@TestPropertySource
 public @interface IntegrationTest {
 
-	/**
-	 * Synonym for properties().
-	 */
-	String[] value() default {};
-
 	/**
 	 * Properties in form {@literal key=value} that should be added to the Spring
 	 * {@link Environment} before the test runs.
 	 */
-	String[] properties() default {"server.port=-1", "spring.jmx.enabled=false"};
+	String[] value() default {};
 
 }

spring-boot/src/main/java/org/springframework/boot/test/IntegrationTestPropertiesListener.java

@@ -19,96 +19,59 @@ import java.util.Arrays;
 import java.util.LinkedHashSet;
 import java.util.Set;
 
-import org.springframework.core.annotation.AnnotationUtils;
+import org.springframework.core.annotation.AnnotatedElementUtils;
+import org.springframework.core.annotation.AnnotationAttributes;
 import org.springframework.test.context.MergedContextConfiguration;
 import org.springframework.test.context.TestContext;
 import org.springframework.test.context.support.AbstractTestExecutionListener;
 import org.springframework.test.util.ReflectionTestUtils;
 
 /**
- * Manipulate the TestContext to merge properties from <code>@IntegrationTest</code> value
- * and properties attributes.
- * 
- * @author Dave Syer
+ * Manipulate the TestContext to merge properties from {@code @IntegrationTest}.
  *
+ * @author Dave Syer
+ * @author Phillip Webb
+ * @since 1.2.0
  */
-public class IntegrationTestPropertiesListener extends AbstractTestExecutionListener {
+class IntegrationTestPropertiesListener extends AbstractTestExecutionListener {
 
-	private String[] defaultValues = (String[]) AnnotationUtils.getDefaultValue(
-			IntegrationTest.class, "properties");
+	private static final String ANNOTATION_TYPE = IntegrationTest.class.getName();
 
 	@Override
 	public void prepareTestInstance(TestContext testContext) throws Exception {
-		MergedContextConfiguration config = null;
-		try {
-			// Here be hacks...
-			config = (MergedContextConfiguration) ReflectionTestUtils.getField(
-					testContext, "mergedContextConfiguration");
-			ReflectionTestUtils.setField(config, "propertySourceProperties",
-					getEnvironmentProperties(config));
-		}
-		catch (IllegalStateException e) {
-			throw e;
-		}
-		catch (Exception e) {
+		Class<?> testClass = testContext.getTestClass();
+		if (AnnotatedElementUtils.isAnnotated(testClass, ANNOTATION_TYPE)) {
+			AnnotationAttributes annotationAttributes = AnnotatedElementUtils
+					.getAnnotationAttributes(testClass, ANNOTATION_TYPE);
+			addPropertySourceProperties(testContext,
+					annotationAttributes.getStringArray("value"));
 		}
 	}
 
-	protected String[] getEnvironmentProperties(MergedContextConfiguration config) {
-		IntegrationTest annotation = AnnotationUtils.findAnnotation(
-				config.getTestClass(), IntegrationTest.class);
-		return mergeProperties(
-				getDefaultEnvironmentProperties(config.getPropertySourceProperties(),
-						annotation), getEnvironmentProperties(annotation));
-	}
-
-	private String[] getDefaultEnvironmentProperties(String[] original,
-			IntegrationTest annotation) {
-		String[] defaults = mergeProperties(original, defaultValues);
-		if (annotation == null || defaults.length == 0) {
-			// Without an @IntegrationTest we can assume the defaults are fine
-			return defaults;
-		}
-		// If @IntegrationTest is present we don't provide a default for the server.port
-		return filterPorts((String[]) AnnotationUtils.getDefaultValue(annotation,
-				"properties"));
-	}
-
-	private String[] filterPorts(String[] values) {
-
-		Set<String> result = new LinkedHashSet<String>();
-		for (String value : values) {
-			if (!value.contains(".port")) {
-				result.add(value);
-			}
-		}
-		return result.toArray(new String[0]);
-
-	}
-
-	private String[] getEnvironmentProperties(IntegrationTest annotation) {
-		if (annotation == null) {
-			return new String[0];
+	private void addPropertySourceProperties(TestContext testContext, String[] properties) {
+		try {
+			addPropertySourcePropertiesUsingReflection(testContext, properties);
 		}
-		if (Arrays.asList(annotation.properties()).equals(Arrays.asList(defaultValues))) {
-			return annotation.value();
+		catch (RuntimeException ex) {
+			throw ex;
 		}
-		if (annotation.value().length == 0) {
-			return annotation.properties();
+		catch (Exception ex) {
+			throw new IllegalStateException(ex);
 		}
-		throw new IllegalStateException(
-				"Either properties or value can be provided but not both");
 	}
 
-	private String[] mergeProperties(String[] original, String[] extra) {
-		Set<String> result = new LinkedHashSet<String>();
-		for (String value : original) {
-			result.add(value);
-		}
-		for (String value : extra) {
-			result.add(value);
+	private void addPropertySourcePropertiesUsingReflection(TestContext testContext,
+			String[] properties) throws Exception {
+		if (properties.length == 0) {
+			return;
 		}
-		return result.toArray(new String[0]);
+		MergedContextConfiguration configuration = (MergedContextConfiguration) ReflectionTestUtils
+				.getField(testContext, "mergedContextConfiguration");
+		Set<String> merged = new LinkedHashSet<String>((Arrays.asList(configuration
+				.getPropertySourceProperties())));
+		merged.addAll(Arrays.asList(properties));
+		ReflectionTestUtils.setField(configuration, "propertySourceProperties",
+				merged.toArray(new String[merged.size()]));
 	}
 
 }

spring-boot/src/main/java/org/springframework/boot/test/SpringApplicationContextLoader.java

@@ -20,7 +20,8 @@ import java.io.IOException;
 import java.io.StringReader;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.HashMap;
+import java.util.Collections;
+import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
@@ -90,7 +91,7 @@ public class SpringApplicationContextLoader extends AbstractContextLoader {
 				.addAfter(
 						StandardEnvironment.SYSTEM_ENVIRONMENT_PROPERTY_SOURCE_NAME,
 						new MapPropertySource("integrationTest",
-								extractEnvironmentProperties(config.getPropertySourceProperties())));
+								getEnvironmentProperties(config)));
 		application.setEnvironment(environment);
 		List<ApplicationContextInitializer<?>> initializers = getInitializers(config,
 				application);
@@ -101,36 +102,9 @@ public class SpringApplicationContextLoader extends AbstractContextLoader {
 			application.setWebEnvironment(false);
 		}
 		application.setInitializers(initializers);
-
 		return application.run();
 	}
 
-	// Instead of parsing the keys ourselves, we rely on standard handling
-	protected Map<String, Object> extractEnvironmentProperties(String[] values) {
-		Map<String, Object> properties = new HashMap<String, Object>();
-		if (values==null) {
-			return properties;
-		}
-		StringBuilder sb = new StringBuilder();
-		for (String value : values) {
-			sb.append(value).append(LINE_SEPARATOR);
-		}
-		String content = sb.toString();
-		Properties props = new Properties();
-		try {
-			props.load(new StringReader(content));
-		}
-		catch (IOException e) {
-			throw new IllegalStateException("Unexpected could not load properties from '"
-					+ content + "'", e);
-		}
-
-		for (String name : props.stringPropertyNames()) {
-			properties.put(name, props.getProperty(name));
-		}
-		return properties;
-	}
-
 	@Override
 	public void processContextConfiguration(
 			ContextConfigurationAttributes configAttributes) {
@@ -176,8 +150,52 @@ public class SpringApplicationContextLoader extends AbstractContextLoader {
 		return AnnotationConfigContextLoaderUtils
 				.detectDefaultConfigurationClasses(declaringClass);
 	}
-	
-	
+
+	protected Map<String, Object> getEnvironmentProperties(
+			MergedContextConfiguration config) {
+		Map<String, Object> properties = new LinkedHashMap<String, Object>();
+		// JMX bean names will clash if the same bean is used in multiple contexts
+		disableJmx(properties);
+		properties.putAll(extractEnvironmentProperties(config
+				.getPropertySourceProperties()));
+		if (AnnotationUtils.findAnnotation(config.getTestClass(), IntegrationTest.class) == null) {
+			properties.putAll(getDefaultEnvironmentProperties());
+		}
+		return properties;
+	}
+
+	private void disableJmx(Map<String, Object> properties) {
+		properties.put("spring.jmx.enabled", "false");
+	}
+
+	private Map<String, String> getDefaultEnvironmentProperties() {
+		return Collections.singletonMap("server.port", "-1");
+	}
+
+	Map<String, Object> extractEnvironmentProperties(String[] values) {
+		// Instead of parsing the keys ourselves, we rely on standard handling
+		if (values == null) {
+			return Collections.emptyMap();
+		}
+		String content = StringUtils.arrayToDelimitedString(values, LINE_SEPARATOR);
+		Properties properties = new Properties();
+		try {
+			properties.load(new StringReader(content));
+			return asMap(properties);
+		}
+		catch (IOException ex) {
+			throw new IllegalStateException("Unexpected could not load properties from '"
+					+ content + "'", ex);
+		}
+	}
+
+	private Map<String, Object> asMap(Properties properties) {
+		Map<String, Object> map = new LinkedHashMap<String, Object>();
+		for (String name : properties.stringPropertyNames()) {
+			map.put(name, properties.getProperty(name));
+		}
+		return map;
+	}
 
 	private List<ApplicationContextInitializer<?>> getInitializers(
 			MergedContextConfiguration mergedConfig, SpringApplication application) {

spring-boot/src/test/java/org/springframework/boot/test/SpringApplicationConfigurationJmxTests.java

@@ -16,8 +16,6 @@
 
 package org.springframework.boot.test;
 
-import static org.junit.Assert.assertFalse;
-
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.springframework.beans.factory.annotation.Value;
@@ -27,6 +25,8 @@ import org.springframework.context.annotation.Configuration;
 import org.springframework.context.support.PropertySourcesPlaceholderConfigurer;
 import org.springframework.test.context.junit4.SpringJUnit4ClassRunner;
 
+import static org.junit.Assert.assertFalse;
+
 /**
  * Tests for disabling JMX by default
  *

spring-boot/src/test/java/org/springframework/boot/test/SpringApplicationContextLoaderTests.java

@@ -16,9 +16,6 @@
 
 package org.springframework.boot.test;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-
 import java.util.Map;
 
 import org.junit.Test;
@@ -27,6 +24,9 @@ import org.springframework.test.context.TestContext;
 import org.springframework.test.context.TestContextManager;
 import org.springframework.test.util.ReflectionTestUtils;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
 /**
  * Tests for {@link SpringApplicationContextLoader}
  *
@@ -43,24 +43,12 @@ public class SpringApplicationContextLoaderTests {
 		assertKey(config, "anotherKey", "anotherValue");
 	}
 
-	@Test
-	public void environmentPropertiesDefaults() throws Exception {
-		Map<String, Object> config = getEnvironmentProperties(SimpleConfig.class);
-		assertMissingKey(config, "server.port");
-		assertKey(config, "spring.jmx.enabled", "false");
-	}
-
 	@Test
 	public void environmentPropertiesOverrideDefaults() throws Exception {
 		Map<String, Object> config = getEnvironmentProperties(OverrideConfig.class);
 		assertKey(config, "server.port", "2345");
 	}
 
-	@Test(expected=IllegalStateException.class)
-	public void environmentPropertiesIllegal() throws Exception {
-		getEnvironmentProperties(IllegalConfig.class);
-	}
-
 	@Test
 	public void environmentPropertiesAppend() throws Exception {
 		Map<String, Object> config = getEnvironmentProperties(AppendConfig.class);
@@ -82,12 +70,15 @@ public class SpringApplicationContextLoaderTests {
 		assertKey(config, "anotherKey", "another=Value");
 	}
 
-	private Map<String, Object> getEnvironmentProperties(Class<?> testClass) throws Exception {
-		TestContext context = new ExposedTestContextManager(testClass).getExposedTestContext();
+	private Map<String, Object> getEnvironmentProperties(Class<?> testClass)
+			throws Exception {
+		TestContext context = new ExposedTestContextManager(testClass)
+				.getExposedTestContext();
 		new IntegrationTestPropertiesListener().prepareTestInstance(context);
-		MergedContextConfiguration config = (MergedContextConfiguration) ReflectionTestUtils.getField(
-				context, "mergedContextConfiguration");
-		return this.loader.extractEnvironmentProperties(config.getPropertySourceProperties());
+		MergedContextConfiguration config = (MergedContextConfiguration) ReflectionTestUtils
+				.getField(context, "mergedContextConfiguration");
+		return this.loader.extractEnvironmentProperties(config
+				.getPropertySourceProperties());
 	}
 
 	private void assertKey(Map<String, Object> actual, String key, Object value) {
@@ -95,10 +86,6 @@ public class SpringApplicationContextLoaderTests {
 		assertEquals(value, actual.get(key));
 	}
 
-	private void assertMissingKey(Map<String, Object> actual, String key) {
-		assertTrue("Key '" + key + "' found", !actual.containsKey(key));
-	}
-
 	@IntegrationTest({ "key=myValue", "anotherKey:anotherValue" })
 	static class SimpleConfig {
 	}
@@ -107,11 +94,7 @@ public class SpringApplicationContextLoaderTests {
 	static class OverrideConfig {
 	}
 
-	@IntegrationTest(value = { "key=aValue", "anotherKey:anotherValue" }, properties = { "key=myValue", "otherKey=otherValue" })
-	static class IllegalConfig {
-	}
-
-	@IntegrationTest(properties = { "key=myValue", "otherKey=otherValue" })
+	@IntegrationTest({ "key=myValue", "otherKey=otherValue" })
 	static class AppendConfig {
 	}
 
@@ -122,18 +105,20 @@ public class SpringApplicationContextLoaderTests {
 	@IntegrationTest({ "key=my:Value", "anotherKey:another=Value" })
 	static class AnotherSeparatorInValue {
 	}
-	
+
+	/**
+	 * {@link TestContextManager} which exposes the {@link TestContext}.
+	 */
 	private static class ExposedTestContextManager extends TestContextManager {
 
 		public ExposedTestContextManager(Class<?> testClass) {
 			super(testClass);
 		}
-		
+
 		public final TestContext getExposedTestContext() {
 			return super.getTestContext();
 		}
-		
-		
+
 	}
 
 }