Skip to content

Commit

Permalink
Treat overloaded local & external @MethodSource factory methods equally
Browse files Browse the repository at this point in the history 
Prior to this commit, given overloaded @MethodSource factory methods, a
local qualified method name (LQMN) was not treated the same as a
fully-qualified method name (FQMN).

Specifically, if the user provided a FQMN without specifying the
parameter list, JUnit Jupiter would find the factory method that
accepts zero arguments. Whereas, if the user provided an LQMN without
specifying the parameter list, JUnit Jupiter would fail to find the
factory method.

This commit fixes that bug by reworking the internals of
MethodArgumentsProvider so that overloaded local and external factory
methods are looked up with the same semantics.

The commit also improves diagnostics for failure to find a factory
method specified via LQMN by falling back to the same lenient search
semantics that are used to locate a "default" local factory method.

Furthermore, this commit modifies the internals of
MethodArgumentsProvider so that it consistently throws
PreconditionViolationExceptions for user configuration errors.

This commit also introduces additional tests for error use cases.

See: #3130, #3131
Closes: #3266
  • Loading branch information
@sbrannen
sbrannen committed Apr 23, 2023
1 parent 5b4a000 commit 82f1f4b
Show file tree
Hide file tree
Showing 4 changed files with 300 additions and 125 deletions.
8 changes: 7 additions & 1 deletion documentation/src/docs/asciidoc/release-notes/release-notes-5.9.3.adoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ JUnit repository on GitHub.
`@MethodSource("myFactory([I)"` (which was already supported) and
`@MethodSource("myFactory(java.lang.String[])` instead of
`@MethodSource("myFactory([Ljava.lang.String;)`.
* The search algorithm used to find `@MethodSource` factory methods now applies consistent
semantics for _local_ qualified method names and fully-qualified method names for
overloaded factory methods.
* Exceptions thrown for files that cannot be deleted when cleaning up a temporary
directory created via `@TempDir` now include the root cause.
* Lifecycle methods are allowed to be declared as `private` again for backwards
Expand All @@ -52,7 +55,10 @@ JUnit repository on GitHub.

==== New Features and Improvements

* ❓
* The search algorithm used to find `@MethodSource` factory methods now falls back to
lenient search semantics when a factory method cannot be found by qualified name
(without a parameter list) and also provides better diagnostics when a unique factory
method cannot be found.


[[release-notes-5.9.3-junit-vintage]]
Expand Down
113 changes: 64 additions & 49 deletions ...piter-params/src/main/java/org/junit/jupiter/params/provider/MethodArgumentsProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.junit.jupiter.api.extension.ExtensionContext;
import org.junit.jupiter.params.support.AnnotationConsumer;
import org.junit.platform.commons.JUnitException;
import org.junit.platform.commons.PreconditionViolationException;
import org.junit.platform.commons.util.CollectionUtils;
import org.junit.platform.commons.util.Preconditions;
import org.junit.platform.commons.util.ReflectionUtils;
Expand All @@ -40,6 +41,9 @@ class MethodArgumentsProvider implements ArgumentsProvider, AnnotationConsumer<M

private String[] methodNames;

private static final Predicate<Method> isFactoryMethod = //
method -> isConvertibleToStream(method.getReturnType()) && !isTestMethod(method);

@Override
public void accept(MethodSource annotation) {
this.methodNames = annotation.value();
Expand All @@ -52,28 +56,37 @@ public Stream<Arguments> provideArguments(ExtensionContext context) {
Object testInstance = context.getTestInstance().orElse(null);
// @formatter:off
return stream(this.methodNames)
.map(factoryMethodName -> getFactoryMethod(testClass, testMethod, factoryMethodName))
.map(factoryMethodName -> findFactoryMethod(testClass, testMethod, factoryMethodName))
.map(factoryMethod -> context.getExecutableInvoker().invoke(factoryMethod, testInstance))
.flatMap(CollectionUtils::toStream)
.map(MethodArgumentsProvider::toArguments);
// @formatter:on
}

private Method getFactoryMethod(Class<?> testClass, Method testMethod, String factoryMethodName) {
if (!StringUtils.isBlank(factoryMethodName)) {
if (looksLikeAFullyQualifiedMethodName(factoryMethodName)) {
return getFactoryMethodByFullyQualifiedName(factoryMethodName);
}
else if (looksLikeALocalQualifiedMethodName(factoryMethodName)) {
return getFactoryMethodByFullyQualifiedName(testClass.getName() + "#" + factoryMethodName);
}
}
else {
// User did not provide a factory method name, so we search for a
// factory method with the same name as the parameterized test method.
private static Method findFactoryMethod(Class<?> testClass, Method testMethod, String factoryMethodName) {
String originalFactoryMethodName = factoryMethodName;

// If the user did not provide a factory method name, find a "default" local
// factory method with the same name as the parameterized test method.
if (StringUtils.isBlank(factoryMethodName)) {
factoryMethodName = testMethod.getName();
return findFactoryMethodBySimpleName(testClass, testMethod, factoryMethodName);
}
return findFactoryMethodBySimpleName(testClass, testMethod, factoryMethodName);

// Convert local factory method name to fully-qualified method name.
if (!looksLikeAFullyQualifiedMethodName(factoryMethodName)) {
factoryMethodName = testClass.getName() + "#" + factoryMethodName;
}

// Find factory method using fully-qualified name.
Method factoryMethod = findFactoryMethodByFullyQualifiedName(testMethod, factoryMethodName);

// Ensure factory method has a valid return type and is not a test method.
Preconditions.condition(isFactoryMethod.test(factoryMethod), () -> format(
"Could not find valid factory method [%s] for test class [%s] but found the following invalid candidate: %s",
originalFactoryMethodName, testClass.getName(), factoryMethod));

return factoryMethod;
}

private static boolean looksLikeAFullyQualifiedMethodName(String factoryMethodName) {
Expand All @@ -90,52 +103,54 @@ private static boolean looksLikeAFullyQualifiedMethodName(String factoryMethodNa
return indexOfDot < indexOfOpeningParenthesis;
}
// If we get this far, we conclude the supplied factory method name "looks"
// like it was intended to be a fully qualified method name, even if the
// like it was intended to be a fully-qualified method name, even if the
// syntax is invalid. We do this in order to provide better diagnostics for
// the user when a fully qualified method name is in fact invalid.
// the user when a fully-qualified method name is in fact invalid.
return true;
}

private static boolean looksLikeALocalQualifiedMethodName(String factoryMethodName) {
// This method is intended to be called after looksLikeAFullyQualifiedMethodName()
// and therefore does not check for the absence of '#' and does not reason about
// the presence or absence of a fully qualified class name.
if (factoryMethodName.endsWith("()")) {
return true;
}
int indexOfLastOpeningParenthesis = factoryMethodName.lastIndexOf('(');
return (indexOfLastOpeningParenthesis > 0)
&& (indexOfLastOpeningParenthesis < factoryMethodName.lastIndexOf(')'));
}

private Method getFactoryMethodByFullyQualifiedName(String fullyQualifiedMethodName) {
private static Method findFactoryMethodByFullyQualifiedName(Method testMethod, String fullyQualifiedMethodName) {
String[] methodParts = ReflectionUtils.parseFullyQualifiedMethodName(fullyQualifiedMethodName);
String className = methodParts[0];
String methodName = methodParts[1];
String methodParameters = methodParts[2];
Class<?> clazz = loadRequiredClass(className);

// Attempt to find an exact match first.
Method factoryMethod = ReflectionUtils.findMethod(clazz, methodName, methodParameters).orElse(null);
if (factoryMethod != null) {
return factoryMethod;
}

boolean explicitParameterListSpecified = //
StringUtils.isNotBlank(methodParameters) || fullyQualifiedMethodName.endsWith("()");

// If we didn't find an exact match but an explicit parameter list was specified,
// that's a user configuration error.
Preconditions.condition(!explicitParameterListSpecified,
() -> format("Could not find factory method [%s(%s)] in class [%s]", methodName, methodParameters,
className));

return ReflectionUtils.findMethod(loadRequiredClass(className), methodName, methodParameters).orElseThrow(
() -> new JUnitException(format("Could not find factory method [%s(%s)] in class [%s]", methodName,
methodParameters, className)));
// Otherwise, fall back to the same lenient search semantics that are used
// to locate a "default" local factory method.
return findFactoryMethodBySimpleName(clazz, testMethod, methodName);
}

/**
* Find all methods in the given {@code testClass} with the desired {@code factoryMethodName}
* which have return types that can be converted to a {@link Stream}, ignoring the
* {@code testMethod} itself as well as any {@code @Test}, {@code @TestTemplate},
* or {@code @TestFactory} methods with the same name.
* @return the factory method, if found
* @throws org.junit.platform.commons.PreconditionViolationException if the
* factory method was not found or if multiple competing factory methods with
* the same name were found
* Find the factory method by searching for all methods in the given {@code clazz}
* with the desired {@code factoryMethodName} which have return types that can be
* converted to a {@link Stream}, ignoring the {@code testMethod} itself as well
* as any {@code @Test}, {@code @TestTemplate}, or {@code @TestFactory} methods
* with the same name.
* @return the single factory method matching the search criteria
* @throws PreconditionViolationException if the factory method was not found or
* multiple competing factory methods with the same name were found
*/
private Method findFactoryMethodBySimpleName(Class<?> testClass, Method testMethod, String factoryMethodName) {
private static Method findFactoryMethodBySimpleName(Class<?> clazz, Method testMethod, String factoryMethodName) {
Predicate<Method> isCandidate = candidate -> factoryMethodName.equals(candidate.getName())
&& !testMethod.equals(candidate);
List<Method> candidates = ReflectionUtils.findMethods(testClass, isCandidate);
List<Method> candidates = ReflectionUtils.findMethods(clazz, isCandidate);

Predicate<Method> isFactoryMethod = method -> isConvertibleToStream(method.getReturnType())
&& !isTestMethod(method);
List<Method> factoryMethods = candidates.stream().filter(isFactoryMethod).collect(toList());

Preconditions.condition(factoryMethods.size() > 0, () -> {
Expand All @@ -145,23 +160,23 @@ private Method findFactoryMethodBySimpleName(Class<?> testClass, Method testMeth
if (candidates.size() > 0) {
return format(
"Could not find valid factory method [%s] in class [%s] but found the following invalid candidates: %s",
factoryMethodName, testClass.getName(), candidates);
factoryMethodName, clazz.getName(), candidates);
}
// Otherwise, report that we didn't find anything.
return format("Could not find factory method [%s] in class [%s]", factoryMethodName, testClass.getName());
return format("Could not find factory method [%s] in class [%s]", factoryMethodName, clazz.getName());
});
Preconditions.condition(factoryMethods.size() == 1,
() -> format("%d factory methods named [%s] were found in class [%s]: %s", factoryMethods.size(),
factoryMethodName, testClass.getName(), factoryMethods));
factoryMethodName, clazz.getName(), factoryMethods));
return factoryMethods.get(0);
}

private boolean isTestMethod(Method candidate) {
private static boolean isTestMethod(Method candidate) {
return isAnnotated(candidate, Test.class) || isAnnotated(candidate, TestTemplate.class)
|| isAnnotated(candidate, TestFactory.class);
}

private Class<?> loadRequiredClass(String className) {
private static Class<?> loadRequiredClass(String className) {
return ReflectionUtils.tryToLoadClass(className).getOrThrow(
cause -> new JUnitException(format("Could not load class [%s]", className), cause));
}
Expand Down
16 changes: 12 additions & 4 deletions junit-jupiter-params/src/main/java/org/junit/jupiter/params/provider/MethodSource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,12 +110,20 @@
* The names of factory methods within the test class or in external classes
* to use as sources for arguments.
*
* <p>Factory methods in external classes must be referenced by <em>fully
* qualified method name</em> &mdash; for example,
* {@code com.example.StringsProviders#blankStrings} or
* {@code com.example.TopLevelClass$NestedClass#classMethod} for a factory
* <p>Factory methods in external classes must be referenced by
* <em>fully-qualified method name</em> &mdash; for example,
* {@code "com.example.StringsProviders#blankStrings"} or
* {@code "com.example.TopLevelClass$NestedClass#classMethod"} for a factory
* method in a static nested class.
*
* <p>If a factory method accepts arguments that are provided by a
* {@link org.junit.jupiter.api.extension.ParameterResolver ParameterResolver},
* you can supply the formal parameter list in the qualified method name to
* disambiguate between overloaded variants of the factory method. For example,
* {@code "blankStrings(int)"} for a local qualified method name or
* {@code "com.example.StringsProviders#blankStrings(int)"} for a fully-qualified
* method name.
*
* <p>If no factory method names are declared, a method within the test class
* that has the same name as the test method will be used as the factory
* method by default.
Expand Down
Loading

0 comments on commit 82f1f4b

Please sign in to comment.