diff --git a/documentation/modules/ROOT/pages/writing-tests/parameterized-classes-and-tests.adoc b/documentation/modules/ROOT/pages/writing-tests/parameterized-classes-and-tests.adoc index 8f3844fcce9c..42e967d4cc8b 100644 --- a/documentation/modules/ROOT/pages/writing-tests/parameterized-classes-and-tests.adoc +++ b/documentation/modules/ROOT/pages/writing-tests/parameterized-classes-and-tests.adoc @@ -913,8 +913,8 @@ integral types: `byte`, `short`, `int`, `long`, and their boxed counterparts. In addition to implicit conversion from strings to the target types listed in the above table, JUnit Jupiter also provides a fallback mechanism for automatic conversion from a -`String` to a given target type if the target type declares exactly one suitable _factory -method_ or a _factory constructor_ as defined below. +`String` to a given target type if the target type declares a suitable _factory method_ +or _factory constructor_ as defined below. - __factory method__: a non-private, `static` method declared in the target type that accepts either a single `String` argument or a single `CharSequence` argument and @@ -924,9 +924,17 @@ method_ or a _factory constructor_ as defined below. either a single `String` argument or a single `CharSequence` argument. Note that the target type must be declared as either a top-level class or as a `static` nested class. -NOTE: If multiple _factory methods_ are discovered, they will be ignored. If a _factory -method_ and a _factory constructor_ are discovered, the factory method will be used -instead of the constructor. +If there are multiple _factory methods_ or _factory constructors_, matching proceeds in the +following order: + +1. A single _factory method_ accepting a `String` argument. +2. A single _factory constructor_ accepting a `String` argument. +3. A single _factory method_ accepting a `CharSequence` argument. +4. A single _factory constructor_ accepting a `CharSequence` argument. +5. A single _factory method_ accepting a `String` argument once all `@Deprecated` factory +methods have been removed from the set of methods being considered. +6. A single _factory method_ accepting a `CharSequence` argument once all `@Deprecated` factory +methods have been removed from the set of methods being considered. For example, in the following `@ParameterizedTest` method, the `Book` argument will be created by invoking the `Book.fromTitle(String)` factory method and passing `"42 Cats"` diff --git a/documentation/modules/ROOT/partials/release-notes/release-notes-6.1.0-M2.adoc b/documentation/modules/ROOT/partials/release-notes/release-notes-6.1.0-M2.adoc index ba9b0fb0fa68..3ab82eda272e 100644 --- a/documentation/modules/ROOT/partials/release-notes/release-notes-6.1.0-M2.adoc +++ b/documentation/modules/ROOT/partials/release-notes/release-notes-6.1.0-M2.adoc @@ -48,6 +48,9 @@ repository on GitHub. * https://www.junit-pioneer.org/[JUnit Pioneer]'s `DefaultLocaleExtension` and `DefaultTimeZoneExtension` are now part of the JUnit Jupiter. Find examples in the xref:writing-tests/built-in-extensions.adoc#DefaultLocaleAndTimeZone[User Guide]. +* Exclude competing `@Deprecated` factory methods in + xref:writing-tests/parameterized-classes-and-tests.adoc#tests-argument-conversion-implicit-fallback[fallback String-to-Object] + converter. * Trim internal stack frames from `AssertionFailedError` stack traces. * Introduce new `trimStacktrace(Class)` and `retainStackTraceElements(int)` methods for `AssertionFailureBuilder`. These allow user defined assertions to diff --git a/junit-platform-commons/src/main/java/org/junit/platform/commons/support/conversion/FallbackStringToObjectConverter.java b/junit-platform-commons/src/main/java/org/junit/platform/commons/support/conversion/FallbackStringToObjectConverter.java index 80be72128211..b5960eaed539 100644 --- a/junit-platform-commons/src/main/java/org/junit/platform/commons/support/conversion/FallbackStringToObjectConverter.java +++ b/junit-platform-commons/src/main/java/org/junit/platform/commons/support/conversion/FallbackStringToObjectConverter.java @@ -15,6 +15,8 @@ import static org.junit.platform.commons.support.ModifierSupport.isNotStatic; import static org.junit.platform.commons.support.ReflectionSupport.findMethods; import static org.junit.platform.commons.support.ReflectionSupport.invokeMethod; +import static org.junit.platform.commons.support.conversion.FallbackStringToObjectConverter.DeprecationStatus.EXCLUDE_DEPRECATED; +import static org.junit.platform.commons.support.conversion.FallbackStringToObjectConverter.DeprecationStatus.INCLUDE_DEPRECATED; import static org.junit.platform.commons.util.ReflectionUtils.findConstructors; import static org.junit.platform.commons.util.ReflectionUtils.newInstance; @@ -32,8 +34,8 @@ /** * {@code FallbackStringToObjectConverter} is a {@link StringToObjectConverter} * that provides a fallback conversion strategy for converting from a - * {@link String} to a given target type by invoking a static factory method - * or factory constructor defined in the target type. + * {@link String} or {@link CharSequence} to a given target type by invoking a + * static factory method or factory constructor defined in the target type. * *

Search Algorithm

* @@ -92,12 +94,29 @@ public boolean canConvertTo(Class targetType) { private static Function findFactoryExecutable(Class targetType) { return factoryExecutableCache.computeIfAbsent(targetType, type -> { // First, search for exact String argument matches. - Function factory = findFactoryExecutable(type, String.class); + var factory = findFactoryMethodExecutable(type, String.class, INCLUDE_DEPRECATED); + if (factory != null) { + return factory; + } + factory = findFactoryConstructorExecutable(type, String.class); if (factory != null) { return factory; } // Second, fall back to CharSequence argument matches. - factory = findFactoryExecutable(type, CharSequence.class); + factory = findFactoryMethodExecutable(type, CharSequence.class, INCLUDE_DEPRECATED); + if (factory != null) { + return factory; + } + factory = findFactoryConstructorExecutable(type, CharSequence.class); + if (factory != null) { + return factory; + } + // Third, try factory methods again, but exclude deprecated methods + factory = findFactoryMethodExecutable(type, String.class, EXCLUDE_DEPRECATED); + if (factory != null) { + return factory; + } + factory = findFactoryMethodExecutable(type, CharSequence.class, EXCLUDE_DEPRECATED); if (factory != null) { return factory; } @@ -106,13 +125,17 @@ public boolean canConvertTo(Class targetType) { }); } - private static @Nullable Function findFactoryExecutable(Class targetType, - Class parameterType) { - - Method factoryMethod = findFactoryMethod(targetType, parameterType); + private static @Nullable Function findFactoryMethodExecutable(Class targetType, + Class parameterType, DeprecationStatus deprecationStatus) { + Method factoryMethod = findFactoryMethod(targetType, parameterType, deprecationStatus); if (factoryMethod != null) { return source -> invokeMethod(factoryMethod, null, source); } + return null; + } + + private static @Nullable Function findFactoryConstructorExecutable(Class targetType, + Class parameterType) { Constructor constructor = findFactoryConstructor(targetType, parameterType); if (constructor != null) { return source -> newInstance(constructor, source); @@ -120,9 +143,10 @@ public boolean canConvertTo(Class targetType) { return null; } - private static @Nullable Method findFactoryMethod(Class targetType, Class parameterType) { - List factoryMethods = findMethods(targetType, new IsFactoryMethod(targetType, parameterType), - BOTTOM_UP); + private static @Nullable Method findFactoryMethod(Class targetType, Class parameterType, + DeprecationStatus deprecationStatus) { + var isFactoryMethod = new IsFactoryMethod(targetType, parameterType, deprecationStatus); + List factoryMethods = findMethods(targetType, isFactoryMethod, BOTTOM_UP); if (factoryMethods.size() == 1) { return factoryMethods.get(0); } @@ -138,12 +162,17 @@ public boolean canConvertTo(Class targetType) { return null; } + enum DeprecationStatus { + INCLUDE_DEPRECATED, EXCLUDE_DEPRECATED + } + /** * {@link Predicate} that determines if the {@link Method} supplied to * {@link #test(Method)} is a non-private static factory method for the * supplied {@link #targetType} and {@link #parameterType}. */ - record IsFactoryMethod(Class targetType, Class parameterType) implements Predicate { + record IsFactoryMethod(Class targetType, Class parameterType, DeprecationStatus deprecationStatus) + implements Predicate { @Override public boolean test(Method method) { @@ -154,6 +183,10 @@ public boolean test(Method method) { if (isNotStatic(method)) { return false; } + if (deprecationStatus == DeprecationStatus.EXCLUDE_DEPRECATED + && method.getAnnotation(Deprecated.class) != null) { + return false; + } return isFactoryCandidate(method, this.parameterType); } } diff --git a/platform-tests/src/test/java/org/junit/platform/commons/support/conversion/FallbackStringToObjectConverterTests.java b/platform-tests/src/test/java/org/junit/platform/commons/support/conversion/FallbackStringToObjectConverterTests.java index 64b028cb6808..041556934a26 100644 --- a/platform-tests/src/test/java/org/junit/platform/commons/support/conversion/FallbackStringToObjectConverterTests.java +++ b/platform-tests/src/test/java/org/junit/platform/commons/support/conversion/FallbackStringToObjectConverterTests.java @@ -12,6 +12,8 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.junit.platform.commons.support.ReflectionSupport.findMethod; +import static org.junit.platform.commons.support.conversion.FallbackStringToObjectConverter.DeprecationStatus.EXCLUDE_DEPRECATED; +import static org.junit.platform.commons.support.conversion.FallbackStringToObjectConverter.DeprecationStatus.INCLUDE_DEPRECATED; import static org.junit.platform.commons.util.ReflectionUtils.getDeclaredConstructor; import java.lang.reflect.Constructor; @@ -32,7 +34,8 @@ */ class FallbackStringToObjectConverterTests { - private static final IsFactoryMethod isBookFactoryMethod = new IsFactoryMethod(Book.class, String.class); + private static final IsFactoryMethod isBookFactoryMethod = new IsFactoryMethod(Book.class, String.class, + INCLUDE_DEPRECATED); private static final FallbackStringToObjectConverter converter = new FallbackStringToObjectConverter(); @@ -41,7 +44,8 @@ void isNotFactoryMethodForWrongParameterType() { assertThat(isBookFactoryMethod).rejects(bookMethod("factory", Object.class)); assertThat(isBookFactoryMethod).rejects(bookMethod("factory", Number.class)); assertThat(isBookFactoryMethod).rejects(bookMethod("factory", StringBuilder.class)); - assertThat(new IsFactoryMethod(Record2.class, String.class)).rejects(record2Method("from")); + assertThat(new IsFactoryMethod(Record2.class, String.class, INCLUDE_DEPRECATED)).rejects(record2Method("from")); + assertThat(new IsFactoryMethod(Record2.class, String.class, EXCLUDE_DEPRECATED)).rejects(record2Method("from")); } @Test @@ -55,19 +59,60 @@ void isNotFactoryMethodForNonStaticMethod() { } @Test - void isFactoryMethodForValidMethods() { - assertThat(new IsFactoryMethod(Book.class, String.class))// + void isFactoryMethodForValidMethodsNoDeprecated() { + assertThat(new IsFactoryMethod(Book.class, String.class, INCLUDE_DEPRECATED))// .accepts(bookMethod("factory", String.class)); - assertThat(new IsFactoryMethod(Book.class, CharSequence.class))// + assertThat(new IsFactoryMethod(Book.class, String.class, EXCLUDE_DEPRECATED))// + .accepts(bookMethod("factory", String.class)); + + assertThat(new IsFactoryMethod(Book.class, CharSequence.class, INCLUDE_DEPRECATED))// + .accepts(bookMethod("factory", CharSequence.class)); + assertThat(new IsFactoryMethod(Book.class, CharSequence.class, EXCLUDE_DEPRECATED))// .accepts(bookMethod("factory", CharSequence.class)); - assertThat(new IsFactoryMethod(Newspaper.class, String.class))// + + assertThat(new IsFactoryMethod(Newspaper.class, String.class, INCLUDE_DEPRECATED))// + .accepts(newspaperMethod("from"), newspaperMethod("of")); + assertThat(new IsFactoryMethod(Newspaper.class, String.class, EXCLUDE_DEPRECATED))// .accepts(newspaperMethod("from"), newspaperMethod("of")); - assertThat(new IsFactoryMethod(Magazine.class, String.class))// + + assertThat(new IsFactoryMethod(Magazine.class, String.class, INCLUDE_DEPRECATED))// .accepts(magazineMethod("from"), magazineMethod("of")); - assertThat(new IsFactoryMethod(Record2.class, CharSequence.class))// + assertThat(new IsFactoryMethod(Magazine.class, String.class, EXCLUDE_DEPRECATED))// + .accepts(magazineMethod("from"), magazineMethod("of")); + + assertThat(new IsFactoryMethod(Record2.class, CharSequence.class, INCLUDE_DEPRECATED))// + .accepts(record2Method("from")); + assertThat(new IsFactoryMethod(Record2.class, CharSequence.class, EXCLUDE_DEPRECATED))// .accepts(record2Method("from")); } + @Test + void isFactoryMethodForValidMethodsWithDeprecated() { + assertThat(new IsFactoryMethod(Book2.class, String.class, INCLUDE_DEPRECATED))// + .accepts(bookWithDeprecatedMethod("factory", String.class)); + assertThat(new IsFactoryMethod(Book2.class, String.class, EXCLUDE_DEPRECATED))// + .accepts(bookWithDeprecatedMethod("factory", String.class)); + + assertThat(new IsFactoryMethod(Book2.class, CharSequence.class, INCLUDE_DEPRECATED))// + .accepts(bookWithDeprecatedMethod("factory", CharSequence.class)); + assertThat(new IsFactoryMethod(Book2.class, CharSequence.class, EXCLUDE_DEPRECATED))// + .accepts(bookWithDeprecatedMethod("factory", CharSequence.class)); + assertThat(new IsFactoryMethod(Book2.class, CharSequence.class, EXCLUDE_DEPRECATED))// + .rejects(bookWithDeprecatedMethod("factory", StringBuilder.class)); + + assertThat(new IsFactoryMethod(Book2.class, String.class, INCLUDE_DEPRECATED))// + .accepts(bookWithDeprecatedMethod("factoryDeprecated", String.class)); + assertThat(new IsFactoryMethod(Book2.class, String.class, EXCLUDE_DEPRECATED))// + .rejects(bookWithDeprecatedMethod("factoryDeprecated", String.class)); + + assertThat(new IsFactoryMethod(Book2.class, CharSequence.class, INCLUDE_DEPRECATED))// + .accepts(bookWithDeprecatedMethod("factoryDeprecated", CharSequence.class)); + assertThat(new IsFactoryMethod(Book2.class, CharSequence.class, EXCLUDE_DEPRECATED))// + .rejects(bookWithDeprecatedMethod("factoryDeprecated", CharSequence.class)); + assertThat(new IsFactoryMethod(Book2.class, CharSequence.class, EXCLUDE_DEPRECATED))// + .rejects(bookWithDeprecatedMethod("factoryDeprecated", CharSequence.class)); + } + @Test void isNotFactoryConstructorForPrivateConstructor() { assertThat(new IsFactoryConstructor(Magazine.class, String.class)).rejects(constructor(Magazine.class)); @@ -79,6 +124,8 @@ void isNotFactoryConstructorForWrongParameterType() { .rejects(getDeclaredConstructor(Record1.class)); assertThat(new IsFactoryConstructor(Record2.class, String.class))// .rejects(getDeclaredConstructor(Record2.class)); + assertThat(new IsFactoryConstructor(Record3.class, String.class))// + .rejects(getDeclaredConstructor(Record3.class)); } @Test @@ -100,6 +147,12 @@ void convertsStringToBookViaStaticFactoryMethod() throws Exception { assertConverts("enigma", Book.class, new Book("factory(String): enigma")); } + @Test + void convertsStringToBookWithDeprecatedViaConstructor() throws Exception { + // constructor takes precedence over factory method when there are two factory methods, and one is deprecated + assertConverts("enigma", Book2.class, new Book2("enigma")); + } + @Test void convertsStringToRecord2ViaStaticFactoryMethodAcceptingCharSequence() throws Exception { assertConvertsRecord2("enigma", Record2.from(new StringBuffer("enigma"))); @@ -120,6 +173,30 @@ void convertsStringToNewspaperViaConstructorIgnoringMultipleFactoryMethods() thr assertConverts("enigma", Newspaper.class, new Newspaper("enigma")); } + @Test + void convertsDeprecatedToNewspaper() throws Exception { + // when only one method @Deprecated is irrelevant, @Deprecated from(String) > @Deprecated from(CharSequence) + assertConverts("enigma", Newspaper1.class, new Newspaper1("from(String): enigma")); + } + + @Test + void convertsToNewspaperPreferNonDeprecatedToDeprecated() throws Exception { + // when two String factories: parse(String) > @Deprecated from(String) + assertConverts("enigma", Newspaper2.class, new Newspaper2("parse(String): enigma")); + } + + @Test + void convertsToNewspaperPreferOnlyCharSequenceToNonDeprecatedString() throws Exception { + // when two String and one CharSequence factories: parse(CharSequence) > parse(String)/@Deprecated from(String) + assertConverts("enigma", Newspaper3.class, new Newspaper3("parse(CharSequence): enigma")); + } + + @Test + void convertsToNewspaperPreferOnlyCharSequenceToDeprecatedString() throws Exception { + // when two String and two CharSequence factories: parse(CharSequence) > @Deprecated parse(String)/@Deprecated from(String)/@Deprecated from(CharSequence) + assertConverts("enigma", Newspaper4.class, new Newspaper4("parse(CharSequence): enigma")); + } + @Test @DisplayName("Cannot convert String to Diary because Diary has neither a static factory method nor a factory constructor") void cannotConvertStringToDiary() { @@ -147,6 +224,10 @@ private static Method bookMethod(String methodName, Class parameterType) { return findMethod(Book.class, methodName, parameterType).orElseThrow(); } + private static Method bookWithDeprecatedMethod(String methodName, Class parameterType) { + return findMethod(Book2.class, methodName, parameterType).orElseThrow(); + } + private static Method newspaperMethod(String methodName) { return findMethod(Newspaper.class, methodName, String.class).orElseThrow(); } @@ -349,7 +430,252 @@ static Record2 from(CharSequence title) { } } + record Record3(StringBuilder title) { + + static Record2 from(StringBuilder title) { + return new Record2("Record2(StringBuilder): " + title); + } + } + static class Diary { } + static class Book2 { + + private final String title; + + Book2(String title) { + this.title = title; + } + + static Book2 factory(String title) { + return new Book2("factory(String): " + title); + } + + @Deprecated + static Book2 factoryDeprecated(String title) { + return new Book2("factoryDeprecated(String): " + title); + } + + /** + * Static and non-private, but intentionally overloads {@link #factory(String)} + * with a {@link CharSequence} argument to ensure that we don't introduce a + * regression in 6.0, since the String-based factory method should take + * precedence over a CharSequence-based factory method. + */ + static Book2 factory(CharSequence title) { + return new Book2("factory(CharSequence): " + title); + } + + @Deprecated + static Book2 factoryDeprecated(CharSequence title) { + return new Book2("factoryDeprecated(CharSequence): " + title); + } + + // wrong parameter type + static Book2 factory(Object obj) { + throw new UnsupportedOperationException(); + } + + // wrong parameter type + static Book2 factory(Number number) { + throw new UnsupportedOperationException(); + } + + /** + * Wrong parameter type, intentionally a subtype of {@link CharSequence} + * other than {@link String}. + */ + static Book2 factory(StringBuilder builder) { + throw new UnsupportedOperationException(); + } + + @SuppressWarnings("unused") + private static Book2 privateFactory(String title) { + return new Book2(title); + } + + Book2 nonStaticFactory(String title) { + return new Book2(title); + } + + @Override + public boolean equals(Object obj) { + return (this == obj) || (obj instanceof Book2 that && Objects.equals(this.title, that.title)); + } + + @Override + public int hashCode() { + return Objects.hash(title); + } + + @Override + public String toString() { + return "Book2 [title=" + this.title + "]"; + } + } + + static class Newspaper1 { + + private final String title; + + private Newspaper1(String title) { + // constructor must be private for factory/deprecated logic to kick in + this.title = title; + } + + // only String factory, thus being deprecated is irrelevant + @Deprecated + static Newspaper1 from(String title) { + return new Newspaper1("from(String): " + title); + } + + // only CharSequence factory, thus being deprecated is irrelevant, but String version takes precedence + @Deprecated + static Newspaper1 from(CharSequence title) { + return new Newspaper1("from(CharSequence): " + title); + } + + @Override + public boolean equals(Object obj) { + return (this == obj) || (obj instanceof Newspaper1 that && Objects.equals(this.title, that.title)); + } + + @Override + public int hashCode() { + return Objects.hash(title); + } + + @Override + public String toString() { + return "Newspaper1 [title=" + this.title + "]"; + } + } + + static class Newspaper2 { + + private final String title; + + private Newspaper2(String title) { + // constructor must be private for factory/deprecated logic to kick in + this.title = title; + } + + @Deprecated + static Newspaper2 from(String title) { + return new Newspaper2("from(String): " + title); + } + + @Deprecated + static Newspaper2 other(String title) { + return new Newspaper2("parse(CharSequence): " + title); + } + + // String factory without deprecated has precedence over String factory with deprecated + static Newspaper2 parse(String title) { + return new Newspaper2("parse(String): " + title); + } + + @Override + public boolean equals(Object obj) { + return (this == obj) || (obj instanceof Newspaper2 that && Objects.equals(this.title, that.title)); + } + + @Override + public int hashCode() { + return Objects.hash(title); + } + + @Override + public String toString() { + return "Newspaper2 [title=" + this.title + "]"; + } + } + + static class Newspaper3 { + + private final String title; + + private Newspaper3(String title) { + // constructor must be private for factory/deprecated logic to kick in + this.title = title; + } + + @Deprecated + static Newspaper3 from(String title) { + return new Newspaper3("from(String): " + title); + } + + static Newspaper3 parse(String title) { + return new Newspaper3("parse(String): " + title); + } + + // CharSequence factory without deprecated alternative has precedence + // over String factory with deprecated alternative + static Newspaper3 parse(CharSequence title) { + return new Newspaper3("parse(CharSequence): " + title); + } + + @Override + public boolean equals(Object obj) { + return (this == obj) || (obj instanceof Newspaper3 that && Objects.equals(this.title, that.title)); + } + + @Override + public int hashCode() { + return Objects.hash(title); + } + + @Override + public String toString() { + return "Newspaper3 [title=" + this.title + "]"; + } + } + + static class Newspaper4 { + + private final String title; + + private Newspaper4(String title) { + // constructor must be private for factory/deprecated logic to kick in + this.title = title; + } + + @Deprecated + static Newspaper4 from(String title) { + return new Newspaper4("from(String): " + title); + } + + @Deprecated + static Newspaper4 parse(String title) { + return new Newspaper4("parse(String): " + title); + } + + @Deprecated + static Newspaper4 from(CharSequence title) { + return new Newspaper4("from(CharSequence): " + title); + } + + // CharSequence factory with deprecated alternative has precedence + // over deprecated String factory + static Newspaper4 parse(CharSequence title) { + return new Newspaper4("parse(CharSequence): " + title); + } + + @Override + public boolean equals(Object obj) { + return (this == obj) || (obj instanceof Newspaper4 that && Objects.equals(this.title, that.title)); + } + + @Override + public int hashCode() { + return Objects.hash(title); + } + + @Override + public String toString() { + return "Newspaper4 [title=" + this.title + "]"; + } + } + }