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

Overloaded method accepting arguments clashes with intended @MethodSource factory method #3080

Closed
1 task done
sbrannen opened this issue Nov 1, 2022 · 4 comments
Closed
1 task done

Comments

@sbrannen
Copy link
Member

sbrannen commented Nov 1, 2022

Overview

As mentioned in #2191 (comment), prior to Jupiter 5.9 if an overloaded method that accepts arguments also met the criteria for a factory method (based on the return type), that method would not have been considered a factory method.

However, as of Jupiter 5.9 (including 5.9.1) such an overloaded method will be considered a candidate factory method. Consequently, if that method is an overload for the intended @MethodSource factory method an exception will be thrown stating that multiple competing factory methods were discovered.

Example

WorkingTests succeeds. FailingTests fails because it extends Base.

package example;

import java.util.stream.Stream;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.MethodSource;

import static org.junit.jupiter.api.Assertions.assertEquals;

class MethodSourceFactoryTests {

	@Nested
	class WorkingTests {

		@ParameterizedTest
		@MethodSource("data")
		void test(String value) {
			assertEquals(1, value.length());
		}

		// legitimate factory method.
		static Stream<String> data() {
			return Stream.of("a", "b");
		}
	}

	@Nested
	class FailingTests extends Base {

		@ParameterizedTest
		@MethodSource("data")
		void test(String value) {
			assertEquals(1, value.length());
		}

		// legitimate factory method.
		static Stream<String> data() {
			return Stream.of("a", "b");
		}
	}

	static class Base {

		// Jupiter 5.8: not considered a factory method.
		// Jupiter 5.9: considered a factory method.
		static Stream<String> data(String first) {
			return Stream.of(first);
		}
	}

}

Related Issues

Deliverables

  • Determine if there are any improvements that can be made and otherwise document the status quo (including workarounds).
@sbrannen sbrannen changed the title Superclass method accepting arguments clashes with local @MethodSource factory method Overloaded method accepting arguments clashes with intended @MethodSource factory method Nov 1, 2022
@sbrannen
Copy link
Member Author

sbrannen commented Nov 1, 2022

I believe I mentioned this elsewhere, but I'll restate it here...

The current workaround is to use the fully qualified method name to disambiguate between overloaded methods.

However, that is extremely verbose for a locally declared factory method.

For example, the failing test can be fixed with one of the following.

@MethodSource("example.MethodSourceFactoryTests$FailingTests#data")

or

@MethodSource("example.MethodSourceFactoryTests$FailingTests#data()")

To improve the developer experience, we could introduce a simplified syntax for selecting a local factory method (local to the current type hierarchy -- in other words not in an external class).

The following are examples of how this could look (omitting the fully qualified class name).

@MethodSource("data()")

or

@MethodSource("#data()")

In summary, the user originally had this:

@MethodSource("data")

... which leads to ambiguous factory method selection.

And I'm proposing that the user can switch to this for explicit local factory method selection.

@MethodSource("data()") // or @MethodSource("#data()")

@junit-team/junit-lambda, thoughts?

@sbrannen sbrannen added this to the 5.9.2 milestone Nov 1, 2022
@marcphilipp
Copy link
Member

SGTM 👍

@sbrannen
Copy link
Member Author

sbrannen commented Nov 8, 2022

I've intentionally labeled this issue as both a bug and an enhancement, since it's about two things:

  1. a regression (bug) that we won't be able to fix
  2. an enhancement that makes it easier to work around the regression and also makes it easier to disambiguate local factory method candidates in general

@marcphilipp
Copy link
Member

Team decision: Let's go with @MethodSource("data()")

@juliette-derancourt juliette-derancourt self-assigned this Nov 25, 2022
sbrannen added a commit that referenced this issue Jan 8, 2023
This commit fixes a bug regarding whitespace in parameter lists for
local factory methods specified via `@MethodSource`. Specifically,
whitespace is now supported.

For example, @MethodSource("data(int, java.lang.String)") is now
supported (note the space after the comma).

As a side note, whitespace in parameter lists was already supported for
a fully qualified method name.

See #3080, #3101
sbrannen pushed a commit that referenced this issue Jan 8, 2023
sbrannen added a commit that referenced this issue Jan 8, 2023
This commit fixes a bug regarding whitespace in parameter lists for
local factory methods specified via `@MethodSource`. Specifically,
whitespace is now supported.

For example, @MethodSource("data(int, java.lang.String)") is now
supported (note the space after the comma).

As a side note, whitespace in parameter lists was already supported for
a fully qualified method name.

See #3080, #3101
robobario added a commit to kroxylicious/kroxylicious that referenced this issue Mar 13, 2023
There was a change of behaviour in 5.9.0 where methods with arguments became
considered candidate factory methods causing it to throw exceptions when it was
ambiguous which method was targeted.

junit-team/junit5#3080
junit-team/junit5@825ea38
https://junit.org/junit5/docs/current/release-notes/index.html#bug-fixes-2
robobario added a commit to kroxylicious/kroxylicious that referenced this issue Mar 13, 2023
There was a change of behaviour in 5.9.0 where methods with arguments became
considered candidate factory methods causing it to throw exceptions when it was
ambiguous which method was targeted.

junit-team/junit5#3080
junit-team/junit5@825ea38
https://junit.org/junit5/docs/current/release-notes/index.html#bug-fixes-2
@sbrannen sbrannen self-assigned this Apr 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment