Skip to content

Spike for expression language evaluation in display names #4293

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions gradle/libs.versions.toml
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ mavenSurefirePlugin = { module = "org.apache.maven.plugins:maven-surefire-plugin
memoryfilesystem = { module = "com.github.marschall:memoryfilesystem", version = "2.8.1" }
mockito-core = { module = "org.mockito:mockito-core", version.ref = "mockito" }
mockito-junit-jupiter = { module = "org.mockito:mockito-junit-jupiter", version.ref = "mockito" }
mustache = { module = "com.github.spullara.mustache.java:compiler", version = "0.9.10"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
mustache = { module = "com.github.spullara.mustache.java:compiler", version = "0.9.10"}
mustache = { module = "com.github.spullara.mustache.java:compiler", version = "0.9.10" }

nohttp-checkstyle = { module = "io.spring.nohttp:nohttp-checkstyle", version = "0.0.11" }
opentest4j = { module = "org.opentest4j:opentest4j", version.ref = "opentest4j" }
openTestReporting-cli = { module = "org.opentest4j.reporting:open-test-reporting-cli", version.ref = "openTestReporting" }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
package org.junit.jupiter.params;

import java.util.List;
import java.util.Optional;
import java.util.stream.IntStream;

import org.junit.jupiter.params.provider.Arguments;
Expand Down Expand Up @@ -47,7 +48,7 @@ public void formatTestNames(Blackhole blackhole) throws Exception {
var method = TestCase.class.getDeclaredMethod("parameterizedTest", int.class);
var formatter = new ParameterizedTestNameFormatter(
ParameterizedTest.DISPLAY_NAME_PLACEHOLDER + " " + ParameterizedTest.DEFAULT_DISPLAY_NAME + " ({0})",
"displayName", new ParameterizedTestMethodContext(method, method.getAnnotation(ParameterizedTest.class)),
"displayName", new ParameterizedTestMethodContext(method, method.getAnnotation(ParameterizedTest.class), Optional.ofNullable(method.getAnnotation(ExpressionLanguage.class))),
512);
for (int i = 0; i < argumentsList.size(); i++) {
Arguments arguments = argumentsList.get(i);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ private void validateArgumentCount(ExtensionContext extensionContext, Arguments
}

private ArgumentCountValidationMode getArgumentCountValidationMode(ExtensionContext extensionContext) {
ParameterizedTest parameterizedTest = methodContext.annotation;
ParameterizedTest parameterizedTest = methodContext.parameterizedTestAnnotation;
if (parameterizedTest.argumentCountValidation() != ArgumentCountValidationMode.DEFAULT) {
return parameterizedTest.argumentCountValidation();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package org.junit.jupiter.params;

import org.junit.jupiter.params.provider.Arguments;

public class ArgumentsContext {

final int invocationIndex;
final Arguments arguments;
final Object[] consumedArguments;

ArgumentsContext(int invocationIndex, Arguments arguments, Object[] consumedArguments) {
this.invocationIndex = invocationIndex;
this.arguments = arguments;
this.consumedArguments = consumedArguments;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@

package org.junit.jupiter.params;

import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;

@Retention(RetentionPolicy.RUNTIME)
public @interface ExpressionLanguage {

Class<? extends ExpressionLanguageAdapter> value();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@

package org.junit.jupiter.params;

public interface ExpressionLanguageAdapter {

void compile(String template);

void format(Object scope, StringBuffer result);
Comment on lines +6 to +8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should split this into two to allow (not necessarily in this PR) registering a global default expression language that would support instantiating the ExpressionLanguageAdapter at most once and also provide a hook to clean up resources by extending AutoCloseable.

Suggested change
void compile(String template);
void format(Object scope, StringBuffer result);
Expression compile(String template);
interface Expression {
	void evaluate(Map<String, Object> context, Appendable result);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,16 @@ public boolean supportsTestTemplate(ExtensionContext context) {
}

Method templateMethod = context.getTestMethod().get();
Optional<ParameterizedTest> annotation = findAnnotation(templateMethod, ParameterizedTest.class);
if (!annotation.isPresent()) {
Optional<ExpressionLanguage> expressionLanguageAnnotation = findAnnotation(templateMethod,
ExpressionLanguage.class);
Optional<ParameterizedTest> parameterizedTestAnnotation = findAnnotation(templateMethod,
ParameterizedTest.class);
if (!parameterizedTestAnnotation.isPresent()) {
return false;
}

ParameterizedTestMethodContext methodContext = new ParameterizedTestMethodContext(templateMethod,
annotation.get());
parameterizedTestAnnotation.get(), expressionLanguageAnnotation);

Preconditions.condition(methodContext.hasPotentiallyValidSignature(),
() -> String.format(
Expand Down Expand Up @@ -86,15 +89,15 @@ public Stream<TestTemplateInvocationContext> provideTestTemplateInvocationContex
return createInvocationContext(formatter, methodContext, arguments, invocationCount.intValue());
})
.onClose(() ->
Preconditions.condition(invocationCount.get() > 0 || methodContext.annotation.allowZeroInvocations(),
Preconditions.condition(invocationCount.get() > 0 || methodContext.parameterizedTestAnnotation.allowZeroInvocations(),
"Configuration error: You must configure at least one set of arguments for this @ParameterizedTest"));
// @formatter:on
}

@Override
public boolean mayReturnZeroTestTemplateInvocationContexts(ExtensionContext extensionContext) {
ParameterizedTestMethodContext methodContext = getMethodContext(extensionContext);
return methodContext.annotation.allowZeroInvocations();
return methodContext.parameterizedTestAnnotation.allowZeroInvocations();
}

private ParameterizedTestMethodContext getMethodContext(ExtensionContext extensionContext) {
Expand All @@ -115,7 +118,7 @@ private TestTemplateInvocationContext createInvocationContext(ParameterizedTestN
private ParameterizedTestNameFormatter createNameFormatter(ExtensionContext extensionContext,
ParameterizedTestMethodContext methodContext) {

String name = methodContext.annotation.name();
String name = methodContext.parameterizedTestAnnotation.name();
String pattern = name.equals(DEFAULT_DISPLAY_NAME)
? extensionContext.getConfigurationParameter(DISPLAY_NAME_PATTERN_KEY) //
.orElse(ParameterizedTest.DEFAULT_DISPLAY_NAME)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,19 @@
class ParameterizedTestMethodContext {

final Method method;
final ParameterizedTest annotation;
final ParameterizedTest parameterizedTestAnnotation;
final Optional<ExpressionLanguage> expressionLanguageAnnotation;

private final Parameter[] parameters;
private final Resolver[] resolvers;
private final List<ResolverType> resolverTypes;

ParameterizedTestMethodContext(Method method, ParameterizedTest annotation) {
ParameterizedTestMethodContext(Method method, ParameterizedTest parameterizedTestAnnotation,
Optional<ExpressionLanguage> expressionLanguageAnnotation) {
this.method = Preconditions.notNull(method, "method must not be null");
this.annotation = Preconditions.notNull(annotation, "annotation must not be null");
this.parameterizedTestAnnotation = Preconditions.notNull(parameterizedTestAnnotation,
"parameterizedTestAnnotation must not be null");
this.expressionLanguageAnnotation = expressionLanguageAnnotation;
this.parameters = method.getParameters();
this.resolvers = new Resolver[this.parameters.length];
this.resolverTypes = new ArrayList<>(this.parameters.length);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static org.junit.jupiter.params.ParameterizedTest.INDEX_PLACEHOLDER;
import static org.junit.platform.commons.util.StringUtils.isNotBlank;

import java.lang.reflect.InvocationTargetException;
import java.text.FieldPosition;
import java.text.Format;
import java.text.MessageFormat;
Expand All @@ -27,12 +28,14 @@
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.function.Function;
import java.util.stream.IntStream;

import org.jetbrains.annotations.NotNull;
import org.junit.jupiter.api.Named;
import org.junit.jupiter.api.extension.ExtensionConfigurationException;
import org.junit.jupiter.params.provider.Arguments;
Expand Down Expand Up @@ -96,12 +99,12 @@ private PartialFormatter[] parse(String pattern, String displayName, Parameteriz
while (isNotBlank(unparsedSegment)) {
PlaceholderPosition position = findFirstPlaceholder(formatters, unparsedSegment);
if (position == null) {
result.add(determineNonPlaceholderFormatter(unparsedSegment, argumentMaxLength));
result.add(determineNonPlaceholderFormatter(methodContext, unparsedSegment, argumentMaxLength));
break;
}
if (position.index > 0) {
String before = unparsedSegment.substring(0, position.index);
result.add(determineNonPlaceholderFormatter(before, argumentMaxLength));
result.add(determineNonPlaceholderFormatter(methodContext, before, argumentMaxLength));
}
result.add(formatters.get(position.placeholder));
unparsedSegment = unparsedSegment.substring(position.index + position.placeholder.length());
Expand All @@ -110,6 +113,24 @@ private PartialFormatter[] parse(String pattern, String displayName, Parameteriz
return result.toArray(new PartialFormatter[0]);
}

@NotNull
private static Optional<ExpressionLanguageAdapter> createExpressionLanguageAdapter(
ParameterizedTestMethodContext methodContext,
String segment
) {
return methodContext.expressionLanguageAnnotation.map(ExpressionLanguage::value).map(adapterClass -> {
try {
ExpressionLanguageAdapter adapterInstance = adapterClass.getDeclaredConstructor().newInstance();
adapterInstance.compile(segment);
return adapterInstance;
} catch (InstantiationException | IllegalAccessException | InvocationTargetException | NoSuchMethodException ex) {
String message = "Failed to initialize expression language for parameterized test. "
+ "See nested exception for further details.";
throw new JUnitException(message, ex);
}
Comment on lines +122 to +130
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
try {
ExpressionLanguageAdapter adapterInstance = adapterClass.getDeclaredConstructor().newInstance();
adapterInstance.compile(segment);
return adapterInstance;
} catch (InstantiationException | IllegalAccessException | InvocationTargetException | NoSuchMethodException ex) {
String message = "Failed to initialize expression language for parameterized test. "
+ "See nested exception for further details.";
throw new JUnitException(message, ex);
}
ExpressionLanguageAdapter adapterInstance = ReflectionSupport.newInstance(adapterClass);
adapterInstance.compile(segment);
return adapterInstance;

});
}

private static PlaceholderPosition findFirstPlaceholder(PartialFormatters formatters, String segment) {
if (segment.length() < formatters.minimumPlaceholderLength) {
return null;
Expand All @@ -129,10 +150,11 @@ else if (minimum == null || index < minimum.index) {
return minimum;
}

private static PartialFormatter determineNonPlaceholderFormatter(String segment, int argumentMaxLength) {
return segment.contains("{") //
? new MessageFormatPartialFormatter(segment, argumentMaxLength) //
: (context, result) -> result.append(segment);
private NonPlaceholderFormatter determineNonPlaceholderFormatter(
ParameterizedTestMethodContext methodContext, String segment, int argumentMaxLength) {
return createExpressionLanguageAdapter(methodContext, segment)
.map(expressionLanguageAdapter -> (NonPlaceholderFormatter) new ExpressionLanguageNonPlaceholderFormatter(expressionLanguageAdapter))
.orElseGet(() -> new DefaultNonPlaceholderFormatter(segment, argumentMaxLength));
}

private PartialFormatters createPartialFormatters(String displayName, ParameterizedTestMethodContext methodContext,
Expand All @@ -143,15 +165,15 @@ private PartialFormatters createPartialFormatters(String displayName, Parameteri
argumentMaxLength));

PartialFormatters formatters = new PartialFormatters();
formatters.put(INDEX_PLACEHOLDER, PartialFormatter.INDEX);
formatters.put(INDEX_PLACEHOLDER, PlaceholderFormatter.INDEX);
formatters.put(DISPLAY_NAME_PLACEHOLDER, (context, result) -> result.append(displayName));
formatters.put(ARGUMENT_SET_NAME_PLACEHOLDER, PartialFormatter.ARGUMENT_SET_NAME);
formatters.put(ARGUMENT_SET_NAME_PLACEHOLDER, PlaceholderFormatter.ARGUMENT_SET_NAME);
formatters.put(ARGUMENTS_WITH_NAMES_PLACEHOLDER, argumentsWithNamesFormatter);
formatters.put(ARGUMENTS_PLACEHOLDER, new CachingByArgumentsLengthPartialFormatter(
length -> new MessageFormatPartialFormatter(argumentsPattern(length), argumentMaxLength)));
formatters.put(ARGUMENT_SET_NAME_OR_ARGUMENTS_WITH_NAMES_PLACEHOLDER, (context, result) -> {
PartialFormatter formatterToUse = context.arguments instanceof ArgumentSet //
? PartialFormatter.ARGUMENT_SET_NAME //
? PlaceholderFormatter.ARGUMENT_SET_NAME //
: argumentsWithNamesFormatter;
formatterToUse.append(context, result);
});
Expand Down Expand Up @@ -183,37 +205,29 @@ private static class PlaceholderPosition {

}

private static class ArgumentsContext {

private final int invocationIndex;
private final Arguments arguments;
private final Object[] consumedArguments;
@FunctionalInterface
private interface PartialFormatter {

ArgumentsContext(int invocationIndex, Arguments arguments, Object[] consumedArguments) {
this.invocationIndex = invocationIndex;
this.arguments = arguments;
this.consumedArguments = consumedArguments;
}
void append(ArgumentsContext context, StringBuffer result);
}

@FunctionalInterface
private interface PartialFormatter {
private interface PlaceholderFormatter extends PartialFormatter {

PartialFormatter INDEX = (context, result) -> result.append(context.invocationIndex);

PartialFormatter ARGUMENT_SET_NAME = (context, result) -> {
if (!(context.arguments instanceof ArgumentSet)) {
throw new ExtensionConfigurationException(
String.format("When the display name pattern for a @ParameterizedTest contains %s, "
+ "the arguments must be supplied as an ArgumentSet.",
ARGUMENT_SET_NAME_PLACEHOLDER));
String.format("When the display name pattern for a @ParameterizedTest contains %s, "
+ "the arguments must be supplied as an ArgumentSet.",
ARGUMENT_SET_NAME_PLACEHOLDER));
}
result.append(((ArgumentSet) context.arguments).getName());
};

void append(ArgumentsContext context, StringBuffer result);
}

private interface NonPlaceholderFormatter extends PartialFormatter {}

private static class MessageFormatPartialFormatter implements PartialFormatter {

@SuppressWarnings("UnnecessaryUnicodeEscape")
Expand Down Expand Up @@ -289,4 +303,34 @@ Set<String> placeholders() {
}
}

private static class DefaultNonPlaceholderFormatter implements NonPlaceholderFormatter {

private final PartialFormatter delegate;

public DefaultNonPlaceholderFormatter(String segment, int argumentMaxLength) {
this.delegate = segment.contains("{") //
? new MessageFormatPartialFormatter(segment, argumentMaxLength) //
: (context, result) -> result.append(segment);
}

@Override
public void append(ArgumentsContext context, StringBuffer result) {
delegate.append(context, result);
}
}

private static class ExpressionLanguageNonPlaceholderFormatter implements NonPlaceholderFormatter {

private final ExpressionLanguageAdapter expressionLanguageAdapter;

public ExpressionLanguageNonPlaceholderFormatter(ExpressionLanguageAdapter expressionLanguageAdapter) {
this.expressionLanguageAdapter = expressionLanguageAdapter;

}

@Override
public void append(ArgumentsContext argumentsContext, StringBuffer result) {
expressionLanguageAdapter.format(argumentsContext.arguments.get()[0], result);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct, but I'm unsure how we should name multiple arguments, so that they can be accessed by the EL?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should provide a Map<String, Object> using the method parameter names as keys. That would require compiling with javac's -parameters option. We already check for parameter.isNamePresent() in ParameterizedTestMethodContext.getParameterName(int), though, so we could populate the Map based on that and fail (?) if no parameter names are present?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the Map<String, Object>, that's also what I suggested in #1154 (comment).

In summary, I believe it would be better to design a new extension API that receives the String displayName and a Map<String,Object> expressionContextMap and then allow people to pick between Mustache, SpEL, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would require compiling with javac's -parameters option. We already check for parameter.isNamePresent() in ParameterizedTestMethodContext.getParameterName(int), though, so we could populate the Map based on that and fail (?) if no parameter names are present?

I don't think I would throw an exception. Rather, I would document the behavior, pointing out that source code parameter names are available when compiled with -parameters and that otherwise the arguments are available via arg0, arg1, etc. (i.e., the default parameter names returned by Parameter.getName()).

}
}
}
1 change: 1 addition & 0 deletions jupiter-tests/jupiter-tests.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ dependencies {
testImplementation(testFixtures(projects.junitJupiterEngine))
testImplementation(testFixtures(projects.junitPlatformLauncher))
testImplementation(testFixtures(projects.junitPlatformReporting))
implementation(libs.mustache)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
implementation(libs.mustache)
testImplementation(libs.mustache)

}

tasks {
Expand Down
Loading