From 5c9350b8cb8678292ff5887da4ba207fbbeb67d1 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Thu, 27 Oct 2016 16:46:05 +0200 Subject: [PATCH] Support @EnabledIf & @DisabledIf w/o loading ApplicationContext Prior to this commit, when using @EnabledIf or @DisabledIf in Spring's JUnit Jupiter support, the test's ApplicationContext was always eagerly loaded, even if the ApplicationContext would never be used (i.e., the test was disabled). This behavior can lead to undesirable side effects -- for example, attempting to load an application context that requires services only available on the CI server when the test is not actually running on the CI server. This commit addresses this issue by introducing new boolean `loadContext` flags in @EnabledIf and @DisabledIf. By default these flags are set to false which means that the user's test application context will not be loaded to evaluate the expression. On the contrary, a dummy application context will be loaded instead, and expressions will be evaluated against that dummy context. Consequently, if the user wishes to interact with properties from the Spring Environment or with beans from the test application context, the `loadContext` must be explicitly set to true. In addition, expressions which evaluate to a String must now evaluate to exactly "true" or "false" (ignoring case and surrounding whitespace). Issue: SPR-14649 --- ...AbstractExpressionEvaluatingCondition.java | 55 +++++++++++++++---- .../context/junit/jupiter/DisabledIf.java | 20 ++++++- .../junit/jupiter/DisabledIfCondition.java | 3 +- .../test/context/junit/jupiter/EnabledIf.java | 20 ++++++- .../junit/jupiter/EnabledIfCondition.java | 3 +- .../jupiter/DisabledIfConditionTestCase.java | 23 +++++++- .../junit/jupiter/DisabledIfTestCase.java | 17 ++++-- .../junit/jupiter/EnabledIfTestCase.java | 15 +++-- 8 files changed, 127 insertions(+), 29 deletions(-) diff --git a/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/AbstractExpressionEvaluatingCondition.java b/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/AbstractExpressionEvaluatingCondition.java index f5e4e45353..1111e7ec08 100644 --- a/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/AbstractExpressionEvaluatingCondition.java +++ b/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/AbstractExpressionEvaluatingCondition.java @@ -34,6 +34,7 @@ import org.springframework.beans.factory.config.BeanExpressionResolver; import org.springframework.beans.factory.config.ConfigurableBeanFactory; import org.springframework.context.ApplicationContext; import org.springframework.context.ConfigurableApplicationContext; +import org.springframework.context.support.GenericApplicationContext; import org.springframework.core.annotation.AnnotatedElementUtils; import org.springframework.util.Assert; import org.springframework.util.StringUtils; @@ -75,6 +76,8 @@ abstract class AbstractExpressionEvaluatingCondition implements ContainerExecuti * the annotation * @param reasonExtractor a function that extracts the reason from the * annotation + * @param loadContextExtractor a function that extracts the {@code loadContext} + * flag from the annotation * @param enabledOnTrue indicates whether the returned {@code ConditionEvaluationResult} * should be {@link ConditionEvaluationResult#enabled enabled} if the expression * evaluates to {@code true} @@ -83,8 +86,8 @@ abstract class AbstractExpressionEvaluatingCondition implements ContainerExecuti * or test should be enabled; otherwise {@link ConditionEvaluationResult#disabled disabled} */ protected ConditionEvaluationResult evaluateAnnotation(Class annotationType, - Function expressionExtractor, Function reasonExtractor, boolean enabledOnTrue, - ExtensionContext context) { + Function expressionExtractor, Function reasonExtractor, + Function loadContextExtractor, boolean enabledOnTrue, ExtensionContext context) { AnnotatedElement element = context.getElement().get(); Optional annotation = findMergedAnnotation(element, annotationType); @@ -104,7 +107,8 @@ abstract class AbstractExpressionEvaluatingCondition implements ContainerExecuti "The expression in @%s on [%s] must not be blank", annotationType.getSimpleName(), element))); // @formatter:on - boolean evaluatedToTrue = evaluateExpression(expression, annotationType, context); + boolean loadContext = annotation.map(loadContextExtractor).get(); + boolean evaluatedToTrue = evaluateExpression(expression, loadContext, annotationType, context); if (evaluatedToTrue) { String adjective = (enabledOnTrue ? "enabled" : "disabled"); @@ -129,17 +133,27 @@ abstract class AbstractExpressionEvaluatingCondition implements ContainerExecuti } } - private boolean evaluateExpression(String expression, Class annotationType, - ExtensionContext extensionContext) { + private boolean evaluateExpression(String expression, boolean loadContext, + Class annotationType, ExtensionContext extensionContext) { - ApplicationContext applicationContext = SpringExtension.getApplicationContext(extensionContext); + AnnotatedElement element = extensionContext.getElement().get(); + GenericApplicationContext gac = null; + ApplicationContext applicationContext; + + if (loadContext) { + applicationContext = SpringExtension.getApplicationContext(extensionContext); + } else { + gac = new GenericApplicationContext(); + gac.refresh(); + applicationContext = gac; + } if (!(applicationContext instanceof ConfigurableApplicationContext)) { if (logger.isWarnEnabled()) { String contextType = (applicationContext != null ? applicationContext.getClass().getName() : "null"); logger.warn(String.format("@%s(\"%s\") could not be evaluated on [%s] since the test " + "ApplicationContext [%s] is not a ConfigurableApplicationContext", - annotationType.getSimpleName(), expression, extensionContext.getElement(), contextType)); + annotationType.getSimpleName(), expression, element, contextType)); } return false; } @@ -151,12 +165,29 @@ abstract class AbstractExpressionEvaluatingCondition implements ContainerExecuti Object result = expressionResolver.evaluate(configurableBeanFactory.resolveEmbeddedValue(expression), beanExpressionContext); - Assert.state((result instanceof Boolean || result instanceof String), - () -> String.format("@%s(\"%s\") must evaluate to a String or a Boolean, not %s", - annotationType.getSimpleName(), expression, (result != null ? result.getClass().getName() : "null"))); + if (gac != null) { + gac.close(); + } - return (result instanceof Boolean && ((Boolean) result).booleanValue()) - || (result instanceof String && Boolean.parseBoolean((String) result)); + if (result instanceof Boolean) { + return ((Boolean) result).booleanValue(); + } + else if (result instanceof String) { + String str = ((String) result).trim().toLowerCase(); + if ("true".equals(str)) { + return true; + } + Assert.state("false".equals(str), + () -> String.format("@%s(\"%s\") on %s must evaluate to \"true\" or \"false\", not \"%s\"", + annotationType.getSimpleName(), expression, element, result)); + return false; + } + else { + String message = String.format("@%s(\"%s\") on %s must evaluate to a String or a Boolean, not %s", + annotationType.getSimpleName(), expression, element, + (result != null ? result.getClass().getName() : "null")); + throw new IllegalStateException(message); + } } private static Optional findMergedAnnotation(AnnotatedElement element, diff --git a/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/DisabledIf.java b/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/DisabledIf.java index 47551ca2b8..3be0137d39 100644 --- a/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/DisabledIf.java +++ b/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/DisabledIf.java @@ -64,8 +64,8 @@ import org.springframework.core.annotation.AliasFor; public @interface DisabledIf { /** - * Alias for {@link #expression}; only intended to be used if an - * explicit {@link #reason} is not provided. + * Alias for {@link #expression}; only intended to be used if {@link #reason} + * and {@link #loadContext} are not specified. * * @see #expression */ @@ -97,6 +97,7 @@ public @interface DisabledIf { * and {@code @DisabledIf("false")} is logically meaningless. * * @see #reason + * @see #loadContext * @see #value */ @AliasFor("value") @@ -109,4 +110,19 @@ public @interface DisabledIf { */ String reason() default ""; + /** + * Whether the {@code ApplicationContext} associated with the current test + * should be eagerly loaded in order to evaluate the {@link #expression}. + * + *

Defaults to {@code false} so that test application contexts are not + * eagerly loaded unnecessarily. If an expression is based solely on system + * properties or environment variables or does not interact with beans in + * the test's application context, there is no need to load the context + * prematurely since doing so would be a waste of time if the test ends up + * being disabled. + * + * @see #expression + */ + boolean loadContext() default false; + } diff --git a/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/DisabledIfCondition.java b/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/DisabledIfCondition.java index 8185346574..1b9392a398 100644 --- a/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/DisabledIfCondition.java +++ b/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/DisabledIfCondition.java @@ -61,7 +61,8 @@ public class DisabledIfCondition extends AbstractExpressionEvaluatingCondition { } private ConditionEvaluationResult evaluateDisabledIf(ExtensionContext context) { - return evaluateAnnotation(DisabledIf.class, DisabledIf::expression, DisabledIf::reason, false, context); + return evaluateAnnotation(DisabledIf.class, DisabledIf::expression, DisabledIf::reason, + DisabledIf::loadContext, false, context); } } diff --git a/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/EnabledIf.java b/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/EnabledIf.java index bd8602d07c..f9d2c18dc7 100644 --- a/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/EnabledIf.java +++ b/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/EnabledIf.java @@ -63,8 +63,8 @@ import org.springframework.core.annotation.AliasFor; public @interface EnabledIf { /** - * Alias for {@link #expression}; only intended to be used if an - * explicit {@link #reason} is not provided. + * Alias for {@link #expression}; only intended to be used if {@link #reason} + * and {@link #loadContext} are not specified. * * @see #expression */ @@ -96,6 +96,7 @@ public @interface EnabledIf { * and {@code @EnabledIf("true")} is logically meaningless. * * @see #reason + * @see #loadContext * @see #value */ @AliasFor("value") @@ -108,4 +109,19 @@ public @interface EnabledIf { */ String reason() default ""; + /** + * Whether the {@code ApplicationContext} associated with the current test + * should be eagerly loaded in order to evaluate the {@link #expression}. + * + *

Defaults to {@code false} so that test application contexts are not + * eagerly loaded unnecessarily. If an expression is based solely on system + * properties or environment variables or does not interact with beans in + * the test's application context, there is no need to load the context + * prematurely since doing so would be a waste of time if the test ends up + * being disabled. + * + * @see #expression + */ + boolean loadContext() default false; + } diff --git a/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/EnabledIfCondition.java b/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/EnabledIfCondition.java index aaba6b1f7a..b9eb00c48e 100644 --- a/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/EnabledIfCondition.java +++ b/spring-test/src/main/java/org/springframework/test/context/junit/jupiter/EnabledIfCondition.java @@ -60,7 +60,8 @@ public class EnabledIfCondition extends AbstractExpressionEvaluatingCondition { } private ConditionEvaluationResult evaluateEnabledIf(ExtensionContext context) { - return evaluateAnnotation(EnabledIf.class, EnabledIf::expression, EnabledIf::reason, true, context); + return evaluateAnnotation(EnabledIf.class, EnabledIf::expression, EnabledIf::reason, + EnabledIf::loadContext, true, context); } } diff --git a/spring-test/src/test/java/org/springframework/test/context/junit/jupiter/DisabledIfConditionTestCase.java b/spring-test/src/test/java/org/springframework/test/context/junit/jupiter/DisabledIfConditionTestCase.java index 1104215e17..60b86bd3a7 100644 --- a/spring-test/src/test/java/org/springframework/test/context/junit/jupiter/DisabledIfConditionTestCase.java +++ b/spring-test/src/test/java/org/springframework/test/context/junit/jupiter/DisabledIfConditionTestCase.java @@ -76,11 +76,26 @@ class DisabledIfConditionTestCase { @Test void invalidExpressionEvaluationType() { + String methodName = "nonBooleanOrStringExpression"; IllegalStateException exception = expectThrows(IllegalStateException.class, - () -> condition.evaluate(buildExtensionContext("nonBooleanOrStringExpression"))); + () -> condition.evaluate(buildExtensionContext(methodName))); + + Method method = ReflectionUtils.findMethod(getClass(), methodName); + + assertThat(exception.getMessage(), + is(equalTo("@DisabledIf(\"#{6 * 7}\") on " + method + " must evaluate to a String or a Boolean, not java.lang.Integer"))); + } + + @Test + void unsupportedStringEvaluationValue() { + String methodName = "stringExpressionThatIsNeitherTrueNorFalse"; + IllegalStateException exception = expectThrows(IllegalStateException.class, + () -> condition.evaluate(buildExtensionContext(methodName))); + + Method method = ReflectionUtils.findMethod(getClass(), methodName); assertThat(exception.getMessage(), - is(equalTo("@DisabledIf(\"#{6 * 7}\") must evaluate to a String or a Boolean, not java.lang.Integer"))); + is(equalTo("@DisabledIf(\"#{'enigma'}\") on " + method + " must evaluate to \"true\" or \"false\", not \"enigma\""))); } @Test @@ -151,6 +166,10 @@ class DisabledIfConditionTestCase { private void nonBooleanOrStringExpression() { } + @DisabledIf("#{'enigma'}") + private void stringExpressionThatIsNeitherTrueNorFalse() { + } + @DisabledIf(expression = "#{6 * 7 == 42}", reason = "Because... 42!") private void customReason() { } diff --git a/spring-test/src/test/java/org/springframework/test/context/junit/jupiter/DisabledIfTestCase.java b/spring-test/src/test/java/org/springframework/test/context/junit/jupiter/DisabledIfTestCase.java index 0f7c9e1c18..497324be54 100644 --- a/spring-test/src/test/java/org/springframework/test/context/junit/jupiter/DisabledIfTestCase.java +++ b/spring-test/src/test/java/org/springframework/test/context/junit/jupiter/DisabledIfTestCase.java @@ -23,7 +23,8 @@ import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.test.context.TestPropertySource; -import static org.junit.jupiter.api.Assertions.*; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.fail; /** * Integration tests which verify support for {@link DisabledIf @DisabledIf} @@ -63,13 +64,19 @@ class DisabledIfTestCase { } @Test - @DisabledIf("${foo}") + @DisabledIf("${__EnigmaPropertyShouldNotExist__:true}") + void disabledIfWithPropertyPlaceholderForNonexistentPropertyWithDefaultValue() { + fail("This test must be disabled"); + } + + @Test + @DisabledIf(expression = "${foo}", loadContext = true) void disabledIfWithPropertyPlaceholder() { fail("This test must be disabled"); } @Test - @DisabledIf("\t${foo} ") + @DisabledIf(expression = "\t${foo} ", loadContext = true) void disabledIfWithPropertyPlaceholderWithSurroundingWhitespace() { fail("This test must be disabled"); } @@ -105,13 +112,13 @@ class DisabledIfTestCase { } @Test - @DisabledIf("#{@booleanTrueBean}") + @DisabledIf(expression = "#{@booleanTrueBean}", loadContext = true) void disabledIfWithSpelBooleanTrueBean() { fail("This test must be disabled"); } @Test - @DisabledIf("#{@stringTrueBean}") + @DisabledIf(expression = "#{@stringTrueBean}", loadContext = true) void disabledIfWithSpelStringTrueBean() { fail("This test must be disabled"); } diff --git a/spring-test/src/test/java/org/springframework/test/context/junit/jupiter/EnabledIfTestCase.java b/spring-test/src/test/java/org/springframework/test/context/junit/jupiter/EnabledIfTestCase.java index 13631320f6..cb6a8753bb 100644 --- a/spring-test/src/test/java/org/springframework/test/context/junit/jupiter/EnabledIfTestCase.java +++ b/spring-test/src/test/java/org/springframework/test/context/junit/jupiter/EnabledIfTestCase.java @@ -18,6 +18,7 @@ package org.springframework.test.context.junit.jupiter; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; + import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.test.context.TestPropertySource; @@ -64,13 +65,19 @@ class EnabledIfTestCase { } @Test - @EnabledIf("${foo}") + @EnabledIf("${__EnigmaPropertyShouldNotExist__:false}") + void enabledIfWithPropertyPlaceholderForNonexistentPropertyWithDefaultValue() { + fail("This test must be disabled"); + } + + @Test + @EnabledIf(expression = "${foo}", loadContext = true) void enabledIfWithPropertyPlaceholder() { fail("This test must be disabled"); } @Test - @EnabledIf("\t${foo} ") + @EnabledIf(expression = "\t${foo} ", loadContext = true) void enabledIfWithPropertyPlaceholderWithSurroundingWhitespace() { fail("This test must be disabled"); } @@ -108,13 +115,13 @@ class EnabledIfTestCase { } @Test - @EnabledIf("#{@booleanFalseBean}") + @EnabledIf(expression = "#{@booleanFalseBean}", loadContext = true) void enabledIfWithSpelBooleanFalseBean() { fail("This test must be disabled"); } @Test - @EnabledIf("#{@stringFalseBean}") + @EnabledIf(expression = "#{@stringFalseBean}", loadContext = true) void enabledIfWithSpelStringFalseBean() { fail("This test must be disabled"); }