From 33d5b011d3e6e85767e910896f5dacf5930eb08a Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Sat, 27 Oct 2012 22:29:55 +0200 Subject: [PATCH] Reduce code duplication in ContextLoaderUtils Prior to this commit, the following two methods in ContextLoaderUtils contained almost identical loops for traversing the test class hierarchy: - resolveContextLoaderClass(Class, String) - resolveContextConfigurationAttributes(Class) With this commit, resolveContextLoaderClass() no longer traverses the class hierarchy. Instead, it now works directly with the resolved list of ContextConfigurationAttributes, thereby removing code duplication. Issue: SPR-9918 --- .../test/context/ContextLoaderUtils.java | 183 ++++++++---------- .../test/context/ContextLoaderUtilsTests.java | 10 +- 2 files changed, 90 insertions(+), 103 deletions(-) diff --git a/spring-test/src/main/java/org/springframework/test/context/ContextLoaderUtils.java b/spring-test/src/main/java/org/springframework/test/context/ContextLoaderUtils.java index 781f7bd7a2c..5e2e2357076 100644 --- a/spring-test/src/main/java/org/springframework/test/context/ContextLoaderUtils.java +++ b/spring-test/src/main/java/org/springframework/test/context/ContextLoaderUtils.java @@ -21,7 +21,6 @@ import static org.springframework.core.annotation.AnnotationUtils.*; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Set; @@ -64,58 +63,70 @@ abstract class ContextLoaderUtils { } /** - * Resolve the {@link ContextLoader} {@link Class class} to use for the - * supplied {@link Class testClass} and then instantiate and return that - * {@code ContextLoader}. + * Resolve the {@link ContextLoader} {@linkplain Class class} to use for the + * supplied list of {@link ContextConfigurationAttributes} and then + * instantiate and return that {@code ContextLoader}. * *

If the supplied defaultContextLoaderClassName is - * null or empty, the standard - * default context loader class name {@value #DEFAULT_CONTEXT_LOADER_CLASS_NAME} - * will be used. For details on the class resolution process, see - * {@link #resolveContextLoaderClass()}. + * {@code null} or empty, depending on the absence or presence + * of @{@link WebAppConfiguration} either {@value #DEFAULT_CONTEXT_LOADER_CLASS_NAME} + * or {@value #DEFAULT_WEB_CONTEXT_LOADER_CLASS_NAME} will be used as the + * default context loader class name. For details on the class resolution + * process, see {@link #resolveContextLoaderClass()}. * * @param testClass the test class for which the {@code ContextLoader} - * should be resolved (must not be null) + * should be resolved; must not be {@code null} + * @param configAttributesList the list of configuration attributes to process; + * must not be {@code null} or empty; must be ordered bottom-up + * (i.e., as if we were traversing up the class hierarchy) * @param defaultContextLoaderClassName the name of the default - * {@code ContextLoader} class to use (may be null) + * {@code ContextLoader} class to use; may be {@code null} or empty * @return the resolved {@code ContextLoader} for the supplied - * testClass (never null) + * testClass (never {@code null}) * @see #resolveContextLoaderClass() */ - static ContextLoader resolveContextLoader(Class testClass, String defaultContextLoaderClassName) { - Assert.notNull(testClass, "Test class must not be null"); + static ContextLoader resolveContextLoader(Class testClass, + List configAttributesList, String defaultContextLoaderClassName) { + Assert.notNull(testClass, "Class must not be null"); + Assert.notEmpty(configAttributesList, "ContextConfigurationAttributes list must not be empty"); if (!StringUtils.hasText(defaultContextLoaderClassName)) { defaultContextLoaderClassName = testClass.isAnnotationPresent(WebAppConfiguration.class) ? DEFAULT_WEB_CONTEXT_LOADER_CLASS_NAME : DEFAULT_CONTEXT_LOADER_CLASS_NAME; } - Class contextLoaderClass = resolveContextLoaderClass(testClass, + Class contextLoaderClass = resolveContextLoaderClass(testClass, configAttributesList, defaultContextLoaderClassName); return instantiateClass(contextLoaderClass, ContextLoader.class); } /** - * Resolve the {@link ContextLoader} {@link Class} to use for the supplied - * {@link Class testClass}. + * Resolve the {@link ContextLoader} {@linkplain Class class} to use for the + * supplied list of {@link ContextConfigurationAttributes}. + * + *

Beginning with the first level in the context configuration attributes + * hierarchy: * *

    - *
  1. If the {@link ContextConfiguration#loader() loader} attribute of - * {@link ContextConfiguration @ContextConfiguration} is configured - * with an explicit class, that class will be returned.
    2. - *
  2. If a loader class is not specified, the class hierarchy - * will be traversed to find a parent class annotated with - * {@code @ContextConfiguration}; go to step #1.
    3. - *
  3. If no explicit loader class is found after traversing - * the class hierarchy, an attempt will be made to load and return the class + *
  4. If the {@link ContextConfigurationAttributes#getContextLoaderClass() + * contextLoaderClass} property of {@link ContextConfigurationAttributes} is + * configured with an explicit class, that class will be returned.
    5. + *
  5. If an explicit {@code ContextLoader} class is not specified at the + * current level in the hierarchy, traverse to the next level in the hierarchy + * and return to step #1.
    6. + *
  6. If no explicit {@code ContextLoader} class is found after traversing + * the hierarchy, an attempt will be made to load and return the class * with the supplied defaultContextLoaderClassName.
    7. *
* * @param testClass the class for which to resolve the {@code ContextLoader} - * class; must not be null + * class; must not be {@code null}; only used for logging purposes + * @param configAttributesList the list of configuration attributes to process; + * must not be {@code null} or empty; must be ordered bottom-up + * (i.e., as if we were traversing up the class hierarchy) * @param defaultContextLoaderClassName the name of the default - * {@code ContextLoader} class to use; must not be null or empty + * {@code ContextLoader} class to use; must not be {@code null} or empty * @return the {@code ContextLoader} class to use for the supplied test class * @throws IllegalArgumentException if {@code @ContextConfiguration} is not * present on the supplied test class @@ -124,46 +135,37 @@ abstract class ContextLoaderUtils { */ @SuppressWarnings("unchecked") static Class resolveContextLoaderClass(Class testClass, - String defaultContextLoaderClassName) { + List configAttributesList, String defaultContextLoaderClassName) { Assert.notNull(testClass, "Class must not be null"); + Assert.notEmpty(configAttributesList, "ContextConfigurationAttributes list must not be empty"); Assert.hasText(defaultContextLoaderClassName, "Default ContextLoader class name must not be null or empty"); - Class annotationType = ContextConfiguration.class; - Class declaringClass = findAnnotationDeclaringClass(annotationType, testClass); - Assert.notNull(declaringClass, String.format( - "Could not find an 'annotation declaring class' for annotation type [%s] and test class [%s]", - annotationType, testClass)); - - while (declaringClass != null) { - ContextConfiguration contextConfiguration = declaringClass.getAnnotation(annotationType); - + for (ContextConfigurationAttributes configAttributes : configAttributesList) { if (logger.isTraceEnabled()) { - logger.trace(String.format( - "Processing ContextLoader for @ContextConfiguration [%s] and declaring class [%s]", - contextConfiguration, declaringClass)); + logger.trace(String.format("Processing ContextLoader for context configuration attributes %s", + configAttributes)); } - Class contextLoaderClass = contextConfiguration.loader(); + Class contextLoaderClass = configAttributes.getContextLoaderClass(); if (!ContextLoader.class.equals(contextLoaderClass)) { if (logger.isDebugEnabled()) { logger.debug(String.format( - "Found explicit ContextLoader class [%s] for @ContextConfiguration [%s] and declaring class [%s]", - contextLoaderClass, contextConfiguration, declaringClass)); + "Found explicit ContextLoader class [%s] for context configuration attributes %s", + contextLoaderClass.getName(), configAttributes)); } return contextLoaderClass; } - - declaringClass = findAnnotationDeclaringClass(annotationType, declaringClass.getSuperclass()); } try { if (logger.isTraceEnabled()) { logger.trace(String.format("Using default ContextLoader class [%s] for test class [%s]", - defaultContextLoaderClassName, testClass)); + defaultContextLoaderClassName, testClass.getName())); } return (Class) ContextLoaderUtils.class.getClassLoader().loadClass( defaultContextLoaderClassName); - } catch (ClassNotFoundException ex) { + } + catch (ClassNotFoundException ex) { throw new IllegalStateException("Could not load default ContextLoader class [" + defaultContextLoaderClassName + "]. Specify @ContextConfiguration's 'loader' " + "attribute or make the default loader class available."); @@ -181,30 +183,29 @@ abstract class ContextLoaderUtils { * consideration. If these flags need to be honored, that must be handled * manually when traversing the list returned by this method. * - * @param clazz the class for which to resolve the configuration attributes (must - * not be null) - * @return the list of configuration attributes for the specified class - * (never null) - * @throws IllegalArgumentException if the supplied class is null or + * @param testClass the class for which to resolve the configuration attributes (must + * not be {@code null}) + * @return the list of configuration attributes for the specified class, ordered bottom-up + * (i.e., as if we were traversing up the class hierarchy); never {@code null} + * @throws IllegalArgumentException if the supplied class is {@code null} or * if {@code @ContextConfiguration} is not present on the supplied class */ - static List resolveContextConfigurationAttributes(Class clazz) { - Assert.notNull(clazz, "Class must not be null"); + static List resolveContextConfigurationAttributes(Class testClass) { + Assert.notNull(testClass, "Class must not be null"); final List attributesList = new ArrayList(); Class annotationType = ContextConfiguration.class; - Class declaringClass = findAnnotationDeclaringClass(annotationType, clazz); + Class declaringClass = findAnnotationDeclaringClass(annotationType, testClass); Assert.notNull(declaringClass, String.format( - "Could not find an 'annotation declaring class' for annotation type [%s] and class [%s]", annotationType, - clazz)); + "Could not find an 'annotation declaring class' for annotation type [%s] and class [%s]", + annotationType.getName(), testClass.getName())); while (declaringClass != null) { ContextConfiguration contextConfiguration = declaringClass.getAnnotation(annotationType); - if (logger.isTraceEnabled()) { logger.trace(String.format("Retrieved @ContextConfiguration [%s] for declaring class [%s].", - contextConfiguration, declaringClass)); + contextConfiguration, declaringClass.getName())); } ContextConfigurationAttributes attributes = new ContextConfigurationAttributes(declaringClass, @@ -213,7 +214,7 @@ abstract class ContextLoaderUtils { logger.trace("Resolved context configuration attributes: " + attributes); } - attributesList.add(0, attributes); + attributesList.add(attributes); declaringClass = findAnnotationDeclaringClass(annotationType, declaringClass.getSuperclass()); } @@ -221,20 +222,6 @@ abstract class ContextLoaderUtils { return attributesList; } - /** - * Create a copy of the supplied list of {@code ContextConfigurationAttributes} - * in reverse order. - * - * @since 3.2 - */ - private static List reverseContextConfigurationAttributes( - List configAttributesList) { - List configAttributesListReversed = new ArrayList( - configAttributesList); - Collections.reverse(configAttributesListReversed); - return configAttributesListReversed; - } - /** * Resolve the list of merged {@code ApplicationContextInitializer} classes * for the supplied list of {@code ContextConfigurationAttributes}. @@ -247,22 +234,21 @@ abstract class ContextLoaderUtils { * at the given level will be merged with those defined in higher levels * of the class hierarchy. * - * @param configAttributesList the list of configuration attributes to process - * (must not be null) - * @return the list of merged context initializer classes, including those - * from superclasses if appropriate (never null) + * @param configAttributesList the list of configuration attributes to process; + * must not be {@code null} or empty; must be ordered bottom-up + * (i.e., as if we were traversing up the class hierarchy) + * @return the set of merged context initializer classes, including those + * from superclasses if appropriate (never {@code null}) * @since 3.2 */ static Set>> resolveInitializerClasses( List configAttributesList) { - Assert.notNull(configAttributesList, "configAttributesList must not be null"); + Assert.notEmpty(configAttributesList, "ContextConfigurationAttributes list must not be empty"); final Set>> initializerClasses = // new HashSet>>(); - // Traverse config attributes in reverse order (i.e., as if we were traversing up - // the class hierarchy). - for (ContextConfigurationAttributes configAttributes : reverseContextConfigurationAttributes(configAttributesList)) { + for (ContextConfigurationAttributes configAttributes : configAttributesList) { if (logger.isTraceEnabled()) { logger.trace(String.format("Processing context initializers for context configuration attributes %s", configAttributes)); @@ -287,23 +273,23 @@ abstract class ContextLoaderUtils { * set to true, profiles defined in the test class will be * merged with those defined in superclasses. * - * @param clazz the class for which to resolve the active profiles (must - * not be null) + * @param testClass the class for which to resolve the active profiles (must + * not be {@code null}) * @return the set of active profiles for the specified class, including - * active profiles from superclasses if appropriate (never null) + * active profiles from superclasses if appropriate (never {@code null}) * @see org.springframework.test.context.ActiveProfiles * @see org.springframework.context.annotation.Profile */ - static String[] resolveActiveProfiles(Class clazz) { - Assert.notNull(clazz, "Class must not be null"); + static String[] resolveActiveProfiles(Class testClass) { + Assert.notNull(testClass, "Class must not be null"); Class annotationType = ActiveProfiles.class; - Class declaringClass = findAnnotationDeclaringClass(annotationType, clazz); + Class declaringClass = findAnnotationDeclaringClass(annotationType, testClass); if (declaringClass == null && logger.isDebugEnabled()) { logger.debug(String.format( "Could not find an 'annotation declaring class' for annotation type [%s] and class [%s]", - annotationType, clazz)); + annotationType.getName(), testClass.getName())); } final Set activeProfiles = new HashSet(); @@ -313,7 +299,7 @@ abstract class ContextLoaderUtils { if (logger.isTraceEnabled()) { logger.trace(String.format("Retrieved @ActiveProfiles [%s] for declaring class [%s].", annotation, - declaringClass)); + declaringClass.getName())); } String[] profiles = annotation.profiles(); @@ -322,11 +308,12 @@ abstract class ContextLoaderUtils { if (!ObjectUtils.isEmpty(valueProfiles) && !ObjectUtils.isEmpty(profiles)) { String msg = String.format("Test class [%s] has been configured with @ActiveProfiles' 'value' [%s] " + "and 'profiles' [%s] attributes. Only one declaration of active bean " - + "definition profiles is permitted per @ActiveProfiles annotation.", declaringClass, + + "definition profiles is permitted per @ActiveProfiles annotation.", declaringClass.getName(), ObjectUtils.nullSafeToString(valueProfiles), ObjectUtils.nullSafeToString(profiles)); logger.error(msg); throw new IllegalStateException(msg); - } else if (!ObjectUtils.isEmpty(valueProfiles)) { + } + else if (!ObjectUtils.isEmpty(valueProfiles)) { profiles = valueProfiles; } @@ -349,9 +336,9 @@ abstract class ContextLoaderUtils { * defaultContextLoaderClassName. * * @param testClass the test class for which the {@code MergedContextConfiguration} - * should be built (must not be null) + * should be built (must not be {@code null}) * @param defaultContextLoaderClassName the name of the default - * {@code ContextLoader} class to use (may be null) + * {@code ContextLoader} class to use (may be {@code null}) * @return the merged context configuration * @see #resolveContextLoader() * @see #resolveContextConfigurationAttributes() @@ -363,14 +350,13 @@ abstract class ContextLoaderUtils { static MergedContextConfiguration buildMergedContextConfiguration(Class testClass, String defaultContextLoaderClassName) { - final ContextLoader contextLoader = resolveContextLoader(testClass, defaultContextLoaderClassName); final List configAttributesList = resolveContextConfigurationAttributes(testClass); + final ContextLoader contextLoader = resolveContextLoader(testClass, configAttributesList, + defaultContextLoaderClassName); final List locationsList = new ArrayList(); final List> classesList = new ArrayList>(); - // Traverse config attributes in reverse order (i.e., as if we were traversing up - // the class hierarchy). - for (ContextConfigurationAttributes configAttributes : reverseContextConfigurationAttributes(configAttributesList)) { + for (ContextConfigurationAttributes configAttributes : configAttributesList) { if (logger.isTraceEnabled()) { logger.trace(String.format("Processing locations and classes for context configuration attributes %s", configAttributes)); @@ -381,7 +367,8 @@ abstract class ContextLoaderUtils { smartContextLoader.processContextConfiguration(configAttributes); locationsList.addAll(0, Arrays.asList(configAttributes.getLocations())); classesList.addAll(0, Arrays.asList(configAttributes.getClasses())); - } else { + } + else { String[] processedLocations = contextLoader.processLocations(configAttributes.getDeclaringClass(), configAttributes.getLocations()); locationsList.addAll(0, Arrays.asList(processedLocations)); diff --git a/spring-test/src/test/java/org/springframework/test/context/ContextLoaderUtilsTests.java b/spring-test/src/test/java/org/springframework/test/context/ContextLoaderUtilsTests.java index 9ac2afd91d3..5b386cfc943 100644 --- a/spring-test/src/test/java/org/springframework/test/context/ContextLoaderUtilsTests.java +++ b/spring-test/src/test/java/org/springframework/test/context/ContextLoaderUtilsTests.java @@ -16,8 +16,8 @@ package org.springframework.test.context; -import static org.springframework.test.context.ContextLoaderUtils.*; import static org.junit.Assert.*; +import static org.springframework.test.context.ContextLoaderUtils.*; import java.util.Arrays; import java.util.Collections; @@ -140,8 +140,8 @@ public class ContextLoaderUtilsTests { List attributesList = resolveContextConfigurationAttributes(LocationsBar.class); assertNotNull(attributesList); assertEquals(2, attributesList.size()); - assertLocationsFooAttributes(attributesList.get(0)); - assertLocationsBarAttributes(attributesList.get(1)); + assertLocationsBarAttributes(attributesList.get(0)); + assertLocationsFooAttributes(attributesList.get(1)); } @Test @@ -149,8 +149,8 @@ public class ContextLoaderUtilsTests { List attributesList = resolveContextConfigurationAttributes(ClassesBar.class); assertNotNull(attributesList); assertEquals(2, attributesList.size()); - assertClassesFooAttributes(attributesList.get(0)); - assertClassesBarAttributes(attributesList.get(1)); + assertClassesBarAttributes(attributesList.get(0)); + assertClassesFooAttributes(attributesList.get(1)); } @Test(expected = IllegalArgumentException.class)