Ignore duplicate config metadata for cache key in TestContext framework

Prior to this commit, if a developer accidentally copied and pasted the
same @ContextConfiguration or @TestPropertySource declaration from a
test class to one of its subclasses or nested test classes, the Spring
TestContext Framework (TCF) would merge the inherited configuration
with the local configuration, resulting in different sets of
configuration metadata which in turn resulted in a different
ApplicationContext instance being loaded for the test classes. This
behavior led to unnecessary creation of identical application contexts
in the context cache for the TCF stored under different keys.

This commit ignores duplicate configuration metadata when generating
the ApplicationContext cache key (i.e., MergedContextConfiguration) in
the TCF. This is performed for the following annotations.

- @ContextConfiguration
- @activeprofiles (support already existed prior to this commit)
- @TestPropertySource

Specifically, if @ContextConfiguration or @TestPropertySource is
declared on a test class and its subclass or nested test class with the
exact same attributes, only one instance of the annotation will be used
to generate the cache key for the resulting ApplicationContext. The
exception to this rule is an "empty" annotation declaration. An empty
@ContextConfiguration or @TestPropertySource declaration signals that
Spring (or a third-party SmartContextLoader) should detect default
configuration specific to the annotated class. Thus, multiple empty
@ContextConfiguration or @TestPropertySource declarations within a test
class hierarchy are not considered to be duplicate configuration and
are therefore not ignored.

Since @TestPropertySource is a @repeatable annotation, the same
duplicate configuration detection logic is applied for multiple
@TestPropertySource declarations on a single test class or test
interface.

In addition, this commit reinstates validation of the rules for
repeated @TestPropertySource annotations that was removed when support
for @NestedTestConfiguration was introduced.

Closes gh-25800

spring-test/src/main/java/org/springframework/test/context/support/ContextLoaderUtils.java

@@ -242,14 +242,35 @@ static List<ContextConfigurationAttributes> resolveContextConfigurationAttribute
 					annotationType.getName(), testClass.getName()));
 
 		List<ContextConfigurationAttributes> attributesList = new ArrayList<>();
+		ContextConfiguration previousAnnotation = null;
+		Class<?> previousDeclaringClass = null;
 		while (descriptor != null) {
-			convertContextConfigToConfigAttributesAndAddToList(descriptor.synthesizeAnnotation(),
-					descriptor.getRootDeclaringClass(), attributesList);
+			ContextConfiguration currentAnnotation = descriptor.synthesizeAnnotation();
+			// Don't ignore duplicate @ContextConfiguration declaration without resources,
+			// because the ContextLoader will likely detect default resources specific to the
+			// annotated class.
+			if (currentAnnotation.equals(previousAnnotation) && hasResources(currentAnnotation)) {
+				if (logger.isDebugEnabled()) {
+					logger.debug(String.format("Ignoring duplicate %s declaration on [%s], "
+							+ "since it is also declared on [%s].", currentAnnotation,
+							previousDeclaringClass.getName(), descriptor.getRootDeclaringClass().getName()));
+				}
+			}
+			else {
+				convertContextConfigToConfigAttributesAndAddToList(currentAnnotation,
+						descriptor.getRootDeclaringClass(), attributesList);
+			}
+			previousAnnotation = currentAnnotation;
+			previousDeclaringClass = descriptor.getRootDeclaringClass();
 			descriptor = descriptor.next();
 		}
 		return attributesList;
 	}
 
+	private static boolean hasResources(ContextConfiguration contextConfiguration) {
+		return (contextConfiguration.locations().length > 0 || contextConfiguration.classes().length > 0);
+	}
+
 	/**
 	 * Convenience method for creating a {@link ContextConfigurationAttributes}
 	 * instance from the supplied {@link ContextConfiguration} annotation and

spring-test/src/main/java/org/springframework/test/context/support/TestPropertySourceAttributes.java

@@ -17,15 +17,17 @@
 package org.springframework.test.context.support;
 
 import java.util.ArrayList;
-import java.util.Collections;
+import java.util.Arrays;
 import java.util.List;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
 import org.springframework.core.annotation.MergedAnnotation;
 import org.springframework.core.io.ClassPathResource;
+import org.springframework.core.log.LogMessage;
 import org.springframework.core.style.ToStringCreator;
+import org.springframework.lang.Nullable;
 import org.springframework.test.context.TestPropertySource;
 import org.springframework.util.Assert;
 import org.springframework.util.ClassUtils;
@@ -53,6 +55,8 @@ class TestPropertySourceAttributes {
 
 	private final Class<?> declaringClass;
 
+	private final MergedAnnotation<?> rootAnnotation;
+
 	private final List<String> locations = new ArrayList<>();
 
 	private final boolean inheritLocations;
@@ -64,25 +68,83 @@ class TestPropertySourceAttributes {
 
 	TestPropertySourceAttributes(MergedAnnotation<TestPropertySource> annotation) {
 		this.declaringClass = declaringClass(annotation);
+		this.rootAnnotation = annotation.getRoot();
 		this.inheritLocations = annotation.getBoolean("inheritLocations");
 		this.inheritProperties = annotation.getBoolean("inheritProperties");
-		mergePropertiesAndLocations(annotation);
+		addPropertiesAndLocationsFrom(annotation);
+	}
+
+	/**
+	 * Merge this {@code TestPropertySourceAttributes} instance with the
+	 * supplied {@code TestPropertySourceAttributes}, asserting that the two sets
+	 * of test property source attributes have identical values for the
+	 * {@link TestPropertySource#inheritLocations} and
+	 * {@link TestPropertySource#inheritProperties} flags and that the two
+	 * underlying annotations were declared on the same class.
+	 * @since 5.2
+	 */
+	void mergeWith(TestPropertySourceAttributes attributes) {
+		Assert.state(attributes.declaringClass == this.declaringClass,
+				() -> "Detected @TestPropertySource declarations within an aggregate index "
+						+ "with different sources: " + this.declaringClass.getName() + " and "
+						+ attributes.declaringClass.getName());
+		logger.trace(LogMessage.format("Retrieved %s for declaring class [%s].",
+				attributes, this.declaringClass.getName()));
+		assertSameBooleanAttribute(this.inheritLocations, attributes.inheritLocations,
+				"inheritLocations", attributes);
+		assertSameBooleanAttribute(this.inheritProperties, attributes.inheritProperties,
+				"inheritProperties", attributes);
+		mergePropertiesAndLocationsFrom(attributes);
 	}
 
-	private void mergePropertiesAndLocations(MergedAnnotation<TestPropertySource> annotation) {
-		String[] locations = annotation.getStringArray("locations");
-		String[] properties = annotation.getStringArray("properties");
+	private void assertSameBooleanAttribute(boolean expected, boolean actual,
+			String attributeName, TestPropertySourceAttributes that) {
+
+		Assert.isTrue(expected == actual, () -> String.format(
+				"@%s on %s and @%s on %s must declare the same value for '%s' as other " +
+				"directly present or meta-present @TestPropertySource annotations",
+			this.rootAnnotation.getType().getSimpleName(), this.declaringClass.getSimpleName(),
+			that.rootAnnotation.getType().getSimpleName(), that.declaringClass.getSimpleName(),
+			attributeName));
+	}
+
+	private void addPropertiesAndLocationsFrom(MergedAnnotation<TestPropertySource> mergedAnnotation) {
+		String[] locations = mergedAnnotation.getStringArray("locations");
+		String[] properties = mergedAnnotation.getStringArray("properties");
+		addPropertiesAndLocations(locations, properties, declaringClass(mergedAnnotation), false);
+	}
+
+	private void mergePropertiesAndLocationsFrom(TestPropertySourceAttributes attributes) {
+		addPropertiesAndLocations(attributes.getLocations(), attributes.getProperties(),
+				attributes.getDeclaringClass(), true);
+	}
+
+	private void addPropertiesAndLocations(String[] locations, String[] properties,
+			Class<?> declaringClass, boolean prepend) {
+
 		if (ObjectUtils.isEmpty(locations) && ObjectUtils.isEmpty(properties)) {
-			Collections.addAll(this.locations, detectDefaultPropertiesFile(annotation));
+			addAll(prepend, this.locations, detectDefaultPropertiesFile(declaringClass));
 		}
 		else {
-			Collections.addAll(this.locations, locations);
-			Collections.addAll(this.properties, properties);
+			addAll(prepend, this.locations, locations);
+			addAll(prepend, this.properties, properties);
 		}
 	}
 
-	private String detectDefaultPropertiesFile(MergedAnnotation<TestPropertySource> annotation) {
-		Class<?> testClass = declaringClass(annotation);
+	/**
+	 * Add all of the supplied elements to the provided list, honoring the
+	 * {@code prepend} flag.
+	 * <p>If the {@code prepend} flag is {@code false}, the elements will appended
+	 * to the list.
+	 * @param prepend whether the elements should be prepended to the list
+	 * @param list the list to which to add the elements
+	 * @param elements the elements to add to the list
+	 */
+	private void addAll(boolean prepend, List<String> list, String... elements) {
+		list.addAll((prepend ? 0 : list.size()), Arrays.asList(elements));
+	}
+
+	private String detectDefaultPropertiesFile(Class<?> testClass) {
 		String resourcePath = ClassUtils.convertClassNameToResourcePath(testClass.getName()) + ".properties";
 		ClassPathResource classPathResource = new ClassPathResource(resourcePath);
 		if (!classPathResource.exists()) {
@@ -153,6 +215,45 @@ boolean isInheritProperties() {
 		return this.inheritProperties;
 	}
 
+	boolean isEmpty() {
+		return (this.locations.isEmpty() && this.properties.isEmpty());
+	}
+
+	@Override
+	public boolean equals(@Nullable Object other) {
+		if (this == other) {
+			return true;
+		}
+		if (other == null || other.getClass() != getClass()) {
+			return false;
+		}
+
+		TestPropertySourceAttributes that = (TestPropertySourceAttributes) other;
+		if (!this.locations.equals(that.locations)) {
+			return false;
+		}
+		if (!this.properties.equals(that.properties)) {
+			return false;
+		}
+		if (this.inheritLocations != that.inheritLocations) {
+			return false;
+		}
+		if (this.inheritProperties != that.inheritProperties) {
+			return false;
+		}
+
+		return true;
+	}
+
+	@Override
+	public int hashCode() {
+		int result = this.locations.hashCode();
+		result = 31 * result + this.properties.hashCode();
+		result = 31 * result + (this.inheritLocations ? 1231 : 1237);
+		result = 31 * result + (this.inheritProperties ? 1231 : 1237);
+		return result;
+	}
+
 	/**
 	 * Provide a String representation of the {@code @TestPropertySource}
 	 * attributes and declaring class.

spring-test/src/main/java/org/springframework/test/context/support/TestPropertySourceUtils.java

@@ -27,7 +27,6 @@
 import java.util.Map;
 import java.util.Properties;
 import java.util.stream.Collectors;
-import java.util.stream.Stream;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
@@ -44,6 +43,7 @@
 import org.springframework.core.io.Resource;
 import org.springframework.core.io.ResourceLoader;
 import org.springframework.core.io.support.ResourcePropertySource;
+import org.springframework.lang.Nullable;
 import org.springframework.test.context.TestContextAnnotationUtils;
 import org.springframework.test.context.TestPropertySource;
 import org.springframework.test.context.util.TestContextResourceUtils;
@@ -76,18 +76,71 @@ public abstract class TestPropertySourceUtils {
 
 
 	static MergedTestPropertySources buildMergedTestPropertySources(Class<?> testClass) {
-		List<TestPropertySourceAttributes> attributesList =
-				findRepeatableAnnotations(testClass, TestPropertySource.class)
-					.map(TestPropertySourceAttributes::new)
-					.collect(Collectors.toList());
+		List<TestPropertySourceAttributes> attributesList = new ArrayList<>();
+
+		TestPropertySourceAttributes previousAttributes = null;
+		// Iterate over all aggregate levels, where each level is represented by
+		// a list of merged annotations found at that level (e.g., on a test
+		// class in the class hierarchy).
+		for (List<MergedAnnotation<TestPropertySource>> aggregatedAnnotations :
+				findRepeatableAnnotations(testClass, TestPropertySource.class)) {
+
+			// Convert all of the merged annotations for the current aggregate
+			// level to a list of TestPropertySourceAttributes.
+			List<TestPropertySourceAttributes> aggregatedAttributesList =
+					aggregatedAnnotations.stream().map(TestPropertySourceAttributes::new).collect(Collectors.toList());
+			// Merge all TestPropertySourceAttributes instances for the current
+			// aggregate level into a single TestPropertySourceAttributes instance.
+			TestPropertySourceAttributes mergedAttributes = mergeTestPropertySourceAttributes(aggregatedAttributesList);
+			if (mergedAttributes != null) {
+				if (!duplicationDetected(mergedAttributes, previousAttributes)) {
+					attributesList.add(mergedAttributes);
+				}
+				previousAttributes = mergedAttributes;
+			}
+		}
 
 		if (attributesList.isEmpty()) {
 			return MergedTestPropertySources.empty();
 		}
-
 		return new MergedTestPropertySources(mergeLocations(attributesList), mergeProperties(attributesList));
 	}
 
+	@Nullable
+	private static TestPropertySourceAttributes mergeTestPropertySourceAttributes(
+			List<TestPropertySourceAttributes> aggregatedAttributesList) {
+
+		TestPropertySourceAttributes mergedAttributes = null;
+		TestPropertySourceAttributes previousAttributes = null;
+		for (TestPropertySourceAttributes currentAttributes : aggregatedAttributesList) {
+			if (mergedAttributes == null) {
+				mergedAttributes = currentAttributes;
+			}
+			else if (!duplicationDetected(currentAttributes, previousAttributes)) {
+				mergedAttributes.mergeWith(currentAttributes);
+			}
+			previousAttributes = currentAttributes;
+		}
+
+		return mergedAttributes;
+	}
+
+	private static boolean duplicationDetected(TestPropertySourceAttributes currentAttributes,
+			TestPropertySourceAttributes previousAttributes) {
+
+		boolean duplicationDetected =
+				(currentAttributes.equals(previousAttributes) && !currentAttributes.isEmpty());
+
+		if (duplicationDetected && logger.isDebugEnabled()) {
+			logger.debug(String.format("Ignoring duplicate %s declaration on %s, "
+					+ "since it is also declared on %s.", currentAttributes,
+					previousAttributes.getDeclaringClass().getName(),
+					currentAttributes.getDeclaringClass().getName()));
+		}
+
+		return duplicationDetected;
+	}
+
 	private static String[] mergeLocations(List<TestPropertySourceAttributes> attributesList) {
 		List<String> locations = new ArrayList<>();
 		for (TestPropertySourceAttributes attrs : attributesList) {
@@ -273,12 +326,12 @@ public static Map<String, Object> convertInlinedPropertiesToMap(String... inline
 		return map;
 	}
 
-	private static <T extends Annotation> Stream<MergedAnnotation<T>> findRepeatableAnnotations(
+	private static <T extends Annotation> List<List<MergedAnnotation<T>>> findRepeatableAnnotations(
 			Class<?> clazz, Class<T> annotationType) {
 
 		List<List<MergedAnnotation<T>>> listOfLists = new ArrayList<>();
 		findRepeatableAnnotations(clazz, annotationType, listOfLists, new int[] {0});
-		return listOfLists.stream().flatMap(List::stream);
+		return listOfLists;
 	}
 
 	private static <T extends Annotation> void findRepeatableAnnotations(

spring-test/src/test/java/org/springframework/test/context/MergedContextConfigurationTests.java

@@ -345,7 +345,7 @@ void equalsWithSameProfilesReversed() {
 	@Test
 	void equalsWithSameDuplicateProfiles() {
 		String[] activeProfiles1 = new String[] { "catbert", "dogbert" };
-		String[] activeProfiles2 = new String[] { "catbert", "dogbert", "catbert", "dogbert", "catbert" };
+		String[] activeProfiles2 = new String[] { "dogbert", "catbert", "dogbert", "catbert" };
 		MergedContextConfiguration mergedConfig1 = new MergedContextConfiguration(getClass(),
 				EMPTY_STRING_ARRAY, EMPTY_CLASS_ARRAY, activeProfiles1, loader);
 		MergedContextConfiguration mergedConfig2 = new MergedContextConfiguration(getClass(),