Introduce `Converter` in `junit-platform-commons` #4219

scordio · 2024-12-23T01:33:01Z

Overview

Resolves Introduce generic ConversionService SPI #853
Relates to Implicit conversion from integral numbers to byte array #2702
Relates to Support binary literal format for String to Number conversion #3605
Relates to Make ConvertWith applicable to a larger scope #4514

I hereby agree to the terms of the JUnit Contributor License Agreement.

Definition of Done

There are no TODOs left in the code
Method preconditions are checked and documented in the method's Javadoc
Coding conventions (e.g. for logging) have been followed
Change is covered by automated tests including corner cases, errors, and exception handling
Public API has Javadoc and @API annotations
Change is documented in the User Guide and Release Notes

scordio · 2024-12-23T01:44:39Z

There is plenty of work to do 🙃

The current highlights:

New ConversionService interface, with ClassLoader param instead of ConversionContext (I will come back to it)
New TypedConversionService base class
The implementation of the existing ConversionSupport.convert(String, Class, ClassLoader) is now part of the new DefaultConversionService
A test setup to demonstrate how to solve Locale argument conversion not setting language and country properly #3141 (I'll shape it better as an integration test)

Any feedback would be highly appreciated!

junit-jupiter-params/src/main/java/org/junit/jupiter/params/converter/ArgumentConverter.java

marcphilipp · 2025-01-02T10:11:46Z

Thanks for the draft! 👍

The tests are failing due to:

org.junit.platform.commons.support.conversion.ConversionService: module org.junit.platform.commons does not declare uses

That's because junit-platform-commons/src/module/org.junit.platform.commons/module-info.java is missing

uses org.junit.platform.commons.support.conversion.ConversionService;

marcphilipp

Looks very promising! 👍

...m-commons/src/main/java/org/junit/platform/commons/support/conversion/ConversionSupport.java

...ns/src/main/java/org/junit/platform/commons/support/conversion/DefaultConversionService.java

marcphilipp · 2025-01-02T10:23:55Z

jupiter-tests/src/test/java/org/junit/jupiter/params/converter/LocaleConversionService.java

+
+import org.junit.platform.commons.support.conversion.TypedConversionService;
+
+// FIXME delete


This would make a good test case, though. We have existing tests that register services for tests using an extra class loader:

https://github.com/junit-team/junit5/blob/16c6f72c1c728c015e35cb739ea75884f19f990c/platform-tests/src/test/java/org/junit/platform/launcher/core/LauncherFactoryTests.java#L353-L366

We could generalize and move that method to a test utility class (e.g. in junit-jupiter-api/src/testFixtures) so it can be reused here.

Another possible integration test could be inspired by #3605.

We could generalize and move that method to a test utility class (e.g. in junit-jupiter-api/src/testFixtures) so it can be reused here.

@marcphilipp fine if I do it in a separate PR? Mostly to keep the size of this one under control 🙃

Raised #4544.

marcphilipp · 2025-01-02T10:25:22Z

That's because junit-platform-commons/src/module/org.junit.platform.commons/module-info.java is missing
uses org.junit.platform.commons.support.conversion.ConversionService;

When you add that, you'll also have to add it to platform-tooling-support-tests/projects/jar-describe-module/junit-platform-commons.expected.txt to adjust the integration test.

scordio · 2025-01-15T08:53:35Z

I've been lagging behind with this one but I should be able to spend time on it in the upcoming weekend.

...m-commons/src/main/java/org/junit/platform/commons/support/conversion/ConversionSupport.java

...-jupiter-params/src/main/java/org/junit/jupiter/params/converter/TypedArgumentConverter.java

scordio · 2025-04-21T10:29:40Z

Would you like to include this in 5.13 too? I should have enough time in the upcoming days to finalize it.

...form-commons/src/main/java/org/junit/platform/commons/support/conversion/TypeDescriptor.java

...ons/src/main/java/org/junit/platform/commons/support/conversion/StringToObjectConverter.java

marcphilipp · 2025-05-26T07:00:32Z

jupiter-tests/src/test/java/org/junit/jupiter/params/converter/LocaleConverter.java

+
+	@Override
+	protected @Nullable Locale convert(@Nullable String source) {
+		return source != null ? Locale.forLanguageTag(source) : null;


This class should return false from canConvert when source is null, shouldn't it?

It's possible that you commented before I refactored the Converter hierarchy, so what I'm about to say might not align with what you had in mind 🙂

Currently, LocaleConverter extends TypedConverter, and I assume the latter should maintain support for null values, similar to TypedArgumentConverter.

However, for the sake of this test, LocaleConverter could also extend Converter directly and have a more fine-grained canConvert implementation, given that the underlying Locale.forLanguageTag API is not supposed to accept null values.

Anyway, I think this test lost its value as DefaultConverter uses Locale.forLanguageTag natively (#4751), so I'll probably rewrite it differently or replace it with a more meaningful one.

...-platform-commons/src/main/java/org/junit/platform/commons/support/conversion/Converter.java

scordio · 2025-05-26T08:13:42Z

@marcphilipp @sbrannen I'm just leaving this here 🙃

https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/managing-repository-settings/managing-pull-request-reviews-in-your-repository

marcphilipp · 2025-05-26T09:33:39Z

@scordio Thanks! I've changed the setting.

scordio · 2025-06-01T16:00:36Z

Now that Converter has type parameters, would it make sense to implement that in TypedArgumentConverter so that the existing TypedArgumentConverter implementations could also be used with the new SPI?

Before exploring this in detail, I wanted to check your opinion first.

marcphilipp · 2025-06-02T07:22:08Z

I think that could be useful and worth giving a try. 👍

scordio · 2025-07-22T06:22:03Z

...form-commons/src/main/java/org/junit/platform/commons/support/conversion/TypeDescriptor.java

+	// FIXME [NullAway] parameter type of referenced method is @NonNull, but parameter in functional interface method java.util.function.Function.apply(T) is @Nullable
+	@SuppressWarnings("NullAway")
+	public Optional<Class<?>> getWrapperType() {
+		return Optional.ofNullable(type).map(ReflectionUtils::getWrapperType);
+	}


I think this is a NullAway bug. I'll compose a small reproducer and raise an issue with the project.

Hmm, that is weird. How does NullAway know about the nullability of the Function type parameters of Optional.map? Maybe it doesn't?

I'm still playing with it to understand it better. For example, the error is slightly different with a lambda:

public Optional<Class<?>> getWrapperType() { return Optional.ofNullable(type).map(value -> ReflectionUtils.getWrapperType(value)); }

warning: [NullAway] passing @Nullable parameter 'value' where @NonNull is required return Optional.ofNullable(type).map(value -> ReflectionUtils.getWrapperType(value)); ^ (see http://t.uber.com/nullaway )

I would have expected NullAway to infer that value is non-nullable, but maybe it has no way to do that.

It looks to me like it thinks Optional.map is typed as

<U> Optional<U> map(Function<? super @Nullable T, ? extends @Nullable U> mapper)

when it should be

<U> Optional<U> map(Function<? super @NonNull T, ? extends @Nullable U> mapper)

...mons/src/main/java/org/junit/platform/commons/support/conversion/StringToClassConverter.java

...-platform-commons/src/main/java/org/junit/platform/commons/support/conversion/Converter.java

scordio · 2025-07-22T07:27:52Z

@marcphilipp @sbrannen I think this is now ready for another round of reviews!

A few points that still require work:

Better test coverage (unit and integration tests)
Documentation (Javadoc and user guide)
The unresolved comments above

...form-commons/src/main/java/org/junit/platform/commons/support/conversion/TypedConverter.java

marcphilipp · 2025-07-22T12:42:40Z

...form-commons/src/main/java/org/junit/platform/commons/support/conversion/TypedConverter.java

+
+	@Override
+	public final T convert(@Nullable S source, ConversionContext context) {
+		return convert(source);


Is this extra indirection worth it?

The advantage I see is that classes extending this one will have a simpler surface, without ConversionContext appearing in the code.

Also, after the updates mentioned at #4219 (comment), this will prevent null source.

marcphilipp · 2025-07-22T12:46:11Z

...m-commons/src/main/java/org/junit/platform/commons/support/conversion/ConversionContext.java

+ * @see Converter
+ */
+@API(status = EXPERIMENTAL, since = "6.0")
+public record ConversionContext(TypeDescriptor sourceType, TypeDescriptor targetType, ClassLoader classLoader) {


I guess the constructors need to be public so they can be used in unit tests for Converter implementations, right?

I kept them public for any code invoking ConversionSupport.convert(), which is not only DefaultArgumentConverter but potentially user code as ConversionSupport is a public API.

marcphilipp · 2025-07-22T12:49:45Z

...-platform-commons/src/main/java/org/junit/platform/commons/support/conversion/Converter.java

+	 * @param context the context for the conversion; never {@code null}
+	 * @return {@code true} if the conversion is supported
+	 */
+	boolean canConvert(ConversionContext context);


Should we pass source here as well? In other words, could a Converter only be able to convert S only partially (for example, excluding null)?

I initially tried this direction but ended up with class cast exceptions due to Converter having type parameters, so I eventually removed it.

I also checked how Spring does it in ConditionalConverter, and the strategy seemed to be the same, so I thought this was a common pitfall 🙂

Thanks! I still think it would be preferable if canConvert could return false for null instead of convert throwing an exception (if the source type is not TypeDescriptor.NONE. I can see the problem with the ClassCastExceptions and don't have a great idea how to achieve this. One option could be to pass @Nullable Object source (instead of S) to canConvert. Another would be to introduce an extra canConvertNull method. WDYT?

marcphilipp · 2025-07-22T12:59:53Z

...-platform-commons/src/main/java/org/junit/platform/commons/support/conversion/Converter.java

+	 * type is a reference type
+	 * @throws ConversionException if an error occurs during the conversion
+	 */
+	T convert(@Nullable S source, ConversionContext context) throws ConversionException;


Should we really allow source to be null here? I think it would simplify things a lot if we didn't, wouldn't it? Maybe we'd restrict ourselves too much if we forbade null here, but maybe TypedConverter or a new NonNullConverter base class could forbid null and make it easier to implement a Converter with that returns @NonNull T?

Maybe we'd restrict ourselves too much if we forbade null here

I think so, especially considering this and this logic in DefaultConverter. Part of the null handling could be pulled to ConversionSupport, but I have the feeling it would spread the responsibility of rejecting null in case of primitive types.

As an extreme case, I considered the scenario where users would like to completely replace the built-in logic of DefaultConverter with a custom Converter that always returns true on canConvert. Such a converter might decide that a null source is always converted to a placeholder object. If Converter were to enforce a non-null source, such a use case wouldn't be possible. As I mentioned, this is likely to be unrealistic, but I considered it anyway 🙂

maybe TypedConverter or a new NonNullConverter base class could forbid null and make it easier to implement a Converter with that returns @NonNull T?

All considered, I think having TypedConverter to enforce a non-null source makes sense, and implementors can always fall back to Converter if they want to have control of null values.

Initially, I wanted to mimic the behavior of TypedArgumentConverter in TypedConverter. However, after digesting #4219 (comment) a bit more, Converter is the actual counterpart of TypedArgumentConverter, and renaming TypedConverter to SimpleConverter would also help to remove the potential ambiguity.

I'll adjust TypedConverter accordingly.

Done in 7903c2b.

I'm unresolving this comment as I still have second thoughts about it.

Should we really allow source to be null here? I think it would simplify things a lot if we didn't, wouldn't it?

After digesting this point for some time, I reevaluated where to position the responsibility for null conversion. That responsibility currently lies with DefaultConverter, which I moved from the original ConversionSupport implementation, but maybe that was a mistake:

ConversionSupport should keep the null conversion responsibility, outside the Converter SPI

Converter#convert should always expect non-null values

The limitations I see are that custom converters don't have control over the null conversion process, and the fact that the Converter contract deviates from the ArgumentConverter contract, the latter expecting null values.

Still, I believe restricting to non-null values would simplify things a lot (and Spring used the same strategy).

Thoughts?

@sbrannen Assuming you were involved on the Spring side, any experience you can share?

...m-commons/src/main/java/org/junit/platform/commons/support/conversion/ConversionSupport.java

marcphilipp · 2025-07-24T05:45:37Z

...-platform-commons/src/main/java/org/junit/platform/commons/support/conversion/Converter.java

+	 * @param context the context for the conversion; never {@code null}
+	 * @return {@code true} if the conversion is supported
+	 */
+	boolean canConvert(ConversionContext context);


Thanks! I still think it would be preferable if canConvert could return false for null instead of convert throwing an exception (if the source type is not TypeDescriptor.NONE. I can see the problem with the ClassCastExceptions and don't have a great idea how to achieve this. One option could be to pass @Nullable Object source (instead of S) to canConvert. Another would be to introduce an extra canConvertNull method. WDYT?

marcphilipp · 2025-07-24T05:47:33Z

...m-commons/src/main/java/org/junit/platform/commons/support/conversion/ConversionSupport.java

+			StreamSupport.stream(serviceLoader.spliterator(), false), //
+			Stream.of(DefaultConverter.INSTANCE)) //
+				.filter(candidate -> candidate.canConvert(context)) //
+				.findFirst() //


We don't have to solve this now, but I'm fully expecting us to get requests for some kind of ordering/precedence support in the future.

Maybe an int order() default method in Converter could be done already now?

Let's hold off doing that for now to keep the PR size down.

marcphilipp · 2025-07-24T05:52:54Z

...orm-commons/src/main/java/org/junit/platform/commons/support/conversion/SimpleConverter.java

+
+	@Override
+	public final boolean canConvert(ConversionContext context) {
+		// FIXME adjust for subtypes


What did you have in mind here? Allow subtypes of S and supertypes of T?

Yes, exactly. I just haven't had the time to provide proper testing coverage here yet 🙂 that's next on the list.

marcphilipp · 2025-07-24T06:05:21Z

...m-commons/src/main/java/org/junit/platform/commons/support/conversion/ConversionSupport.java

+		Converter converter = Stream.concat( //
+			StreamSupport.stream(serviceLoader.spliterator(), false), //
+			Stream.of(DefaultConverter.INSTANCE)) //


I'm not sure whether this is better. From its Javadoc and a quick look at the code the Iterator backing ServiceLoader.spliterator is also lazy. I'll leave it up to you.

Suggested change

Converter converter = Stream.concat( //

StreamSupport.stream(serviceLoader.spliterator(), false), //

Stream.of(DefaultConverter.INSTANCE)) //

Converter converter = Stream.<Supplier<Converter>> concat( //

serviceLoader.stream(), //

Stream.of(() -> DefaultConverter.INSTANCE)) //

.map(Supplier::get) //

If I recall correctly, I originally drew inspiration from other parts of JUnit here.

I'll double-check both options.

marcphilipp · 2025-09-29T17:13:36Z

...form-commons/src/main/java/org/junit/platform/commons/support/conversion/TypeDescriptor.java

+		this.type = type;
+	}
+
+	public Class<?> getType() {


The discussion in #5004 made me think that we should consider supporting Type instead of just Class<?>, i.e. making this class more like Guava's TypeToken or Spring's ParameterizedTypeReference.

Thoughts?

we should consider supporting Type instead of just Class<?>

Yes, I also think it would be more future-proof. I mentioned something similar at #4219 (comment), but I can see now I didn't make it prominent enough.

Re TypeToken, there's a related PR introducing a similar class.

The current shape of TypeDescriptor probably makes it closer to Spring's ResolvableType... or, let's say, I was heavily inspired by it!

Do I understand it correctly that you're considering introducing a type token? Is there a use case related to this PR that you imagine would benefit from it, or is this mostly related to the discussion in #5004?

If we were to follow the Spring design, there could be a dedicated TypeToken class and a new TypeDescriptor.forType(TypeToken) factory method: I think such an enhancement could come at any point in time and not necessarily when TypeDescriptor is introduced.

(here is a slightly related discussion about type tokens in AssertJ, which we eventually decided to postpone, waiting for user demand)

Or maybe I misunderstood what you have in mind 🙃

Is there a use case related to this PR that you imagine would benefit from it, or is this mostly related to the discussion in #5004?

For example, a String to List<T> converter, could inspect the type arguments of List and decide how to convert the individual elements.

Example: "1,2,3" as source and List<Integer> vs. List<Long> as targetType

If we were to follow the Spring design, there could be a dedicated TypeToken class and a new TypeDescriptor.forType(TypeToken) factory method: I think such an enhancement could come at any point in time and not necessarily when TypeDescriptor is introduced.

Agreed, let's not do it right away but keep it in mind when naming things, for example, here:

Suggested change

public Class<?> getType() {

public Class<?> getRawClass() {

Example: "1,2,3" as source and List<Integer> vs. List<Long> as targetType

I got a similar use case covered in scordio/junit-converters#4 (see the corresponding test case here).

However, that's at the level of ArgumentConverter where Field or Parameter is directly available and can be fed into the Spring classes to perform the conversion.

I'll keep this use case in mind and see how the Spring integration can work with the changes from this PR, especially to determine whether the current TypeDescriptor APIs are sufficient for this purpose.

scordio force-pushed the conversion-service branch from c31ca83 to b19cba8 Compare December 23, 2024 01:49

scordio mentioned this pull request Dec 23, 2024

Add Locale conversion format configuration property #3410

Closed

6 tasks

scordio force-pushed the conversion-service branch from b19cba8 to a7a8aa3 Compare December 23, 2024 02:23

scordio force-pushed the conversion-service branch 3 times, most recently from 3304ad5 to c886b7a Compare December 31, 2024 11:26

scordio commented Dec 31, 2024

View reviewed changes

junit-jupiter-params/src/main/java/org/junit/jupiter/params/converter/ArgumentConverter.java Outdated Show resolved Hide resolved

scordio force-pushed the conversion-service branch from c886b7a to 7ea91b8 Compare December 31, 2024 12:17

marcphilipp reviewed Jan 2, 2025

View reviewed changes

scordio force-pushed the conversion-service branch 2 times, most recently from 2e17c2b to 0c2faa7 Compare January 2, 2025 16:35

scordio force-pushed the conversion-service branch from 0c2faa7 to 098c972 Compare January 26, 2025 11:41

scordio force-pushed the conversion-service branch from 098c972 to 8ed6eb6 Compare February 9, 2025 08:46

scordio commented Feb 9, 2025

View reviewed changes

...m-commons/src/main/java/org/junit/platform/commons/support/conversion/ConversionSupport.java Show resolved Hide resolved

scordio mentioned this pull request Feb 9, 2025

Move string-based argument conversion tests to platform-test #4305

Merged

6 tasks

sbrannen reviewed Feb 26, 2025

View reviewed changes

...-jupiter-params/src/main/java/org/junit/jupiter/params/converter/TypedArgumentConverter.java Outdated Show resolved Hide resolved

sbrannen added theme: extensions component: Jupiter component: Platform theme: parameterized tests type: new feature labels Feb 26, 2025

scordio force-pushed the conversion-service branch from 8ed6eb6 to 940d018 Compare April 14, 2025 23:22

scordio force-pushed the conversion-service branch 3 times, most recently from 0a36152 to e380b8f Compare April 27, 2025 11:33

marcphilipp reviewed May 26, 2025

View reviewed changes

nareshrajkumar866 approved these changes May 26, 2025

View reviewed changes

scordio force-pushed the conversion-service branch from 8b78923 to 789d5d5 Compare June 1, 2025 16:15

scordio force-pushed the conversion-service branch from 789d5d5 to 4ea8824 Compare June 19, 2025 16:55

scordio force-pushed the conversion-service branch from 4ea8824 to 17591cc Compare July 5, 2025 08:06

scordio force-pushed the conversion-service branch 5 times, most recently from f2cdba7 to 42cc962 Compare July 22, 2025 06:19

scordio commented Jul 22, 2025

View reviewed changes

...mons/src/main/java/org/junit/platform/commons/support/conversion/StringToClassConverter.java Outdated Show resolved Hide resolved

scordio commented Jul 22, 2025

View reviewed changes

...-platform-commons/src/main/java/org/junit/platform/commons/support/conversion/Converter.java Outdated Show resolved Hide resolved

scordio force-pushed the conversion-service branch from fe8883d to a57f5c9 Compare July 22, 2025 10:50

marcphilipp reviewed Jul 22, 2025

View reviewed changes

marcphilipp reviewed Jul 24, 2025

View reviewed changes

scordio mentioned this pull request Jul 29, 2025

@EmptySource not working with ArgumentConverter accepting its supported types #4801

Open

1 task

scordio force-pushed the conversion-service branch 2 times, most recently from ebe4b0a to b8bedaf Compare August 3, 2025 21:09

scordio force-pushed the conversion-service branch from b8bedaf to e362540 Compare August 17, 2025 08:19

Introduce Converter in junit-platform-commons

fbb8448

scordio force-pushed the conversion-service branch from e362540 to fbb8448 Compare August 17, 2025 08:39

marcphilipp reviewed Sep 29, 2025

View reviewed changes


		import org.junit.platform.commons.support.conversion.TypedConversionService;

		// FIXME delete

-		Converter converter = Stream.concat( //
-			StreamSupport.stream(serviceLoader.spliterator(), false), //
-			Stream.of(DefaultConverter.INSTANCE)) //
+		Converter converter = Stream.<Supplier<Converter>> concat( //
+			serviceLoader.stream(), //
+			Stream.of(() -> DefaultConverter.INSTANCE)) //
+				.map(Supplier::get) //

Uh oh!

Uh oh!

Introduce Converter in junit-platform-commons #4219

Are you sure you want to change the base?

Introduce Converter in junit-platform-commons #4219

Uh oh!

Conversation

scordio commented Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Definition of Done

Uh oh!

scordio commented Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

marcphilipp commented Jan 2, 2025

Uh oh!

marcphilipp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcphilipp commented Jan 2, 2025

Uh oh!

scordio commented Jan 15, 2025

Uh oh!

Uh oh!

Uh oh!

scordio commented Apr 21, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scordio commented May 26, 2025

Uh oh!

marcphilipp commented May 26, 2025

Uh oh!

scordio commented Jun 1, 2025

Uh oh!

marcphilipp commented Jun 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

scordio commented Jul 22, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scordio Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Introduce `Converter` in `junit-platform-commons` #4219

Introduce `Converter` in `junit-platform-commons` #4219

scordio commented Dec 23, 2024 •

edited

Loading

scordio commented Dec 23, 2024 •

edited

Loading

scordio Jul 22, 2025 •

edited

Loading

scordio Jul 22, 2025 •

edited

Loading

marcphilipp Jul 24, 2025 •

edited

Loading