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:

junit5/platform-tests/src/test/java/org/junit/platform/launcher/core/LauncherFactoryTests.java

Lines 353 to 366 in 16c6f72

private static void withTestServices(Runnable runnable) {

var current = Thread.currentThread().getContextClassLoader();

var url = LauncherFactoryTests.class.getClassLoader().getResource("testservices/");

try (var classLoader = new URLClassLoader(new URL[] { url }, current)) {

Thread.currentThread().setContextClassLoader(classLoader);

runnable.run();

}

catch (IOException e) {

throw new UncheckedIOException(e);

}

finally {

Thread.currentThread().setContextClassLoader(current);

}

}

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.

scordio · 2025-05-11T13:06:54Z

After renaming to Converter, I realized StringToObjectConverter is pretty much a similar abstraction.

I propose to refactor the StringToObjectConverter hierarchy and rewrite all classes as Converter implementations (still with package-private visibility and used by DefaultConverter only).

WDYT?

sbrannen · 2025-05-11T15:13:33Z

I propose to refactor the StringToObjectConverter hierarchy and rewrite all classes as Converter implementations (still with package-private visibility and used by DefaultConverter only).

WDYT?

That's pretty much the vision I had all along: to have single abstraction/API.

So, yes, that sounds good to me. 👍

Though... the devil is in details. 😉

scordio · 2025-05-11T17:08:52Z

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

+	public Class<?> getType() {
+		return type;
+	}


For now, most of the code relies on getType(), but I plan to check if dedicated APIs could better encapsulate some use cases (similar to getWrapperType or isPrimitive).

scordio · 2025-05-11T17:13:28Z

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

-			}
-			return null;
-		}
+		return convert(source, TypeDescriptor.forType(targetType), getClassLoader(classLoader));


I wonder if the deprecated convert should delegate to the other convert method or should invoke DefaultConverter directly, skipping the new service loader logic. For now, I went with the former.

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

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

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

marcphilipp · 2025-05-12T07:04:33Z

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

+ * @see TypedConverter
+ */
+@API(status = EXPERIMENTAL, since = "1.13")
+public interface Converter {


Didn't we decide to make this generic as in Converter<S, T>?

I guess for that to make sense, we'd have to parameterize TypeDescriptor as well.

@scordio @sbrannen WDYT?

I had the same in mind but wasn't entirely confident about this direction yesterday, especially before refactoring StringToObjectConversion.

But now the refactoring is done so I can sketch it and see what happens 🙂

Started in 8b78923, there is still work to do.

@marcphilipp @sbrannen could you please check if this is the direction you have in mind?

It's worth mentioning that TypedConverter is still available to allow simpler implementations of basic converters.

Thanks! I left some comments in #4219 (review) below

scordio · 2025-05-16T06:21:53Z

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

+	public static TypeDescriptor forClass(Class<?> clazz) {
+		return new TypeDescriptor(clazz);
+	}
+
+	public static TypeDescriptor forInstance(Object instance) {
+		return new TypeDescriptor(instance.getClass());
+	}
+
+	public static TypeDescriptor forField(Field field) {
+		return new TypeDescriptor(field.getType());
+	}
+
+	public static TypeDescriptor forParameter(Parameter parameter) {
+		return new TypeDescriptor(parameter.getType());
+	}


General null handling is currently missing

scordio · 2025-05-16T17:06:41Z

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

+	@Override
+	public boolean equals(Object o) {
+		if (this == o) {
+			return true;
+		}
+		if (o == null || getClass() != o.getClass()) {
+			return false;
+		}
+		TypeDescriptor that = (TypeDescriptor) o;
+		return this.type.equals(that.type);
+	}
+
+	@Override
+	public int hashCode() {
+		return this.type.hashCode();
+	}


I noticed EqualsAndHashCodeAssertions under junit-jupiter-api to test equals/hashCode. It seems I can use the same in junit-platform-commons.

Out of curiosity, was EqualsVerifier ever considered for such tests?

I noticed EqualsAndHashCodeAssertions under junit-jupiter-api to test equals/hashCode. It seems I can use the same in junit-platform-commons.

👍

Out of curiosity, was EqualsVerifier ever considered for such tests?

I don't think it was discussed but it might be a good addition or even replacement but probably in a separate PR.

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

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

marcphilipp · 2025-05-26T06:44:47Z

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

+	}
+
+	public String getTypeName() {
+		return type.getName();


Shouldn't this method also be @Nullable and check for NONE?

marcphilipp · 2025-05-26T06:49:52Z

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

+
+	@Override
+	public final boolean canConvert(TypeDescriptor sourceType, TypeDescriptor targetType) {
+		return canConvert(targetType.getType());


It still doesn't sit well with me that targetType.getType() can be null. Should we add a getRequiredType() method so that Converter implementations don't have to verify it's not null?

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?

...-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 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

scordio commented May 11, 2025

View reviewed changes

marcphilipp reviewed May 12, 2025

View reviewed changes

scordio force-pushed the conversion-service branch from 6001a2c to 5190e37 Compare May 16, 2025 06:19

scordio commented May 16, 2025

View reviewed changes

scordio mentioned this pull request May 16, 2025

Support binary literal format for String to Number conversion #3605

Open

marcphilipp mentioned this pull request May 16, 2025

Introduce generic ConversionService SPI #853

Open

scordio commented May 16, 2025

View reviewed changes

scordio force-pushed the conversion-service branch 2 times, most recently from af01791 to 0034d7d Compare May 16, 2025 17:40

This was referenced May 17, 2025

Add TemporaryClasspathExecutor test utility #4544

Merged

Add nullability annotations and compile-time checks #4557

Merged

scordio force-pushed the conversion-service branch from 0034d7d to 791778e Compare May 24, 2025 07:46

marcphilipp reviewed May 24, 2025

View reviewed changes

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

scordio mentioned this pull request May 24, 2025

Remove null support from getWrapperType input, add test coverage #4571

Merged

6 tasks

scordio commented May 24, 2025

View reviewed changes

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

scordio force-pushed the conversion-service branch 3 times, most recently from 267ee7f to 8b78923 Compare May 25, 2025 15:19

marcphilipp reviewed May 26, 2025

View reviewed changes

nareshrajkumar866 approved these changes May 26, 2025

View reviewed changes

scordio added 2 commits June 1, 2025 11:57

Introduce Converter in junit-platform-commons

0825961

Add ConversionContext

789d5d5

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


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

		// FIXME delete

	private static void withTestServices(Runnable runnable) {
	var current = Thread.currentThread().getContextClassLoader();
	var url = LauncherFactoryTests.class.getClassLoader().getResource("testservices/");
	try (var classLoader = new URLClassLoader(new URL[] { url }, current)) {
	Thread.currentThread().setContextClassLoader(classLoader);
	runnable.run();
	}
	catch (IOException e) {
	throw new UncheckedIOException(e);
	}
	finally {
	Thread.currentThread().setContextClassLoader(current);
	}
	}

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!

scordio commented May 11, 2025

Uh oh!

sbrannen commented May 11, 2025

Uh oh!

scordio May 11, 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!

scordio May 11, 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!

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!

scordio May 25, 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!

scordio May 16, 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!

Uh oh!

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 May 11, 2025 •

edited

Loading

scordio May 11, 2025 •

edited

Loading

scordio May 25, 2025 •

edited

Loading

scordio May 16, 2025 •

edited

Loading