Skip to content
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

Parameters are not validated in local @MethodSource factory method #3130

Closed
2 tasks done
sbrannen opened this issue Jan 21, 2023 · 3 comments
Closed
2 tasks done

Parameters are not validated in local @MethodSource factory method #3130

sbrannen opened this issue Jan 21, 2023 · 3 comments

Comments

@sbrannen
Copy link
Member

sbrannen commented Jan 21, 2023

Overview

The current implementation of support for local factory method names that accept arguments in @MethodSource does not validate the specified parameters. Rather, the lookup is performed based solely on the names of the parameters. Furthermore, the lookup ignores the parameters if there is only one factory method with the specified name.

This can lead to incorrect configuration being silently ignored or confusing error messages when the specified factory method is overloaded.

Examples

Given the following extensions:

class IntegerResolver implements ParameterResolver {
	@Override
	public boolean supportsParameter(ParameterContext pc, ExtensionContext ec) {
		return pc.getParameter().getType() == int.class;
	}
	@Override
	public Object resolveParameter(ParameterContext pc, ExtensionContext ec) {
		return 2;
	}
}

class IntegerArrayResolver implements ParameterResolver {
	@Override
	public boolean supportsParameter(ParameterContext pc, ExtensionContext ec) {
		return pc.getParameter().getType() == int[].class;
	}
	@Override
	public Object resolveParameter(ParameterContext pc, ExtensionContext ec) {
		return new int[] { 2, 3 };
	}
}

And given the following test class:

@ExtendWith({IntegerResolver.class, IntegerArrayResolver.class})
class MethodSourceTests {

	@ParameterizedTest
	// @MethodSource("example.MethodSourceTests#factory(dog)")
	@MethodSource("factory(dog)")
	void test(String argument) {
		assertTrue(argument.startsWith("2"));
	}

	static Stream<Arguments> factory(int quantities) {
		return Stream.of(arguments(quantities + " apples"), arguments(quantities + " lemons"));
	}

//	static Stream<Arguments> factory(int[] quantities) {
//		return Stream.of(arguments(quantities[0] + " apples"), arguments(quantities[0] + " lemons"));
//	}

}

If we run the test "as is", it will pass because the dog parameter is not validated. It is in fact completely ignored since there are no "competing" overloaded factory methods with the same name.

Whereas, if we attempt to do the same using the fully qualified method name (FQMN) syntax (@MethodSource("example.MethodSourceTests#factory(dog)")), we get the following exception.

org.junit.platform.commons.JUnitException: Failed to load parameter type [dog] for method [factory] in class [example.MethodSourceTests].

Similarly, if we supply a valid parameter type for a factory method that does not exist using the FQMN syntax (@MethodSource("example.MethodSourceTests#factory(java.lang.Integer)")), we get the following exception.

org.junit.platform.commons.JUnitException: Could not find factory method [factory(java.lang.Integer)] in class [example.MethodSourceTests]

In contrast, using the local factory method syntax (@MethodSource("factory(java.lang.Integer)")), the test will pass since the parameters are ignored (as with the dog example).

If we uncomment the factory(int[]) method and run the test, we get the following somewhat confusing error message.

org.junit.platform.commons.PreconditionViolationException: 2 factory methods named [factory(dog)] were found in class [example.MethodSourceTests]: [static java.util.stream.Stream example.MethodSourceTests.factory(int), static java.util.stream.Stream example.MethodSourceTests.factory(int[])]

Related Issues

Deliverables

  • Validate factory method parameter types supplied to @MethodSource using the local factory method name syntax.
  • Ensure that error messages for configuration errors encountered when using the local factory method name syntax align with the error messages produced for similar configuration errors when using the fully qualified method name syntax.
@sbrannen
Copy link
Member Author

Reopening to document in release notes.

@sbrannen sbrannen reopened this Jan 23, 2023
@sbrannen sbrannen modified the milestones: 5.10.0-M1, 5.9.3 Apr 22, 2023
@sbrannen
Copy link
Member Author

Reopening to backport to 5.9.3.

@sbrannen sbrannen reopened this Apr 22, 2023
marcphilipp pushed a commit that referenced this issue Apr 22, 2023
Prior to this commit, parameters were not validated in local
@MethodSource factory methods which lead to incorrect configuration
being silently ignored or confusing error messages when the specified
factory method was overloaded. In addition, the syntax for local
@MethodSource factory methods did not support canonical array names.

This commit addresses these issues by treating local factory method
resolution the same as the existing support for resolving a fully
qualified method name for a factory method.

These changes make the recently introduced parseQualifiedMethodName()
method in ReflectionUtils obsolete. This commit therefore removes that
method as well.

Closes #3130
Closes #3131

(cherry picked from commit a0eb8e2)
@marcphilipp
Copy link
Member

Cherry-picked to the releases/5.9.x branch.

sbrannen added a commit that referenced this issue Apr 23, 2023
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
sbrannen added a commit that referenced this issue Apr 23, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants