Introduce ConversionService in junit-platform-commons

[scordio]

Overview

• Resolves Introduce generic ConversionService SPI #853
• Resolves Locale argument conversion not setting language and country properly #3141
• Relates to Implicit conversion from integral numbers to byte array #2702

---

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]

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!

[marcphilipp]

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! 👍

[marcphilipp]

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.

[marcphilipp]

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]

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

[scordio]

I realized now that there are no tests for ConversionSupport in junit-platform-commons, but DefaultArgumentConverterTests from jupiter-tests provide the corresponding coverage.

Is that intended or should I add something to platform-tests?

[marcphilipp]

I think that's because the functionality was originally in DefaultArgumentConverter and was later extracted into ConversionSupport. I think it would be good to move the String conversion tests to a separate test for ConversionSupport directly.

@scordio Do you have time to do that as part of this PR or shall I do that separately as a preparatory step?

[scordio]

No worries, I can take care of that!

[scordio]

Raised #4305.

[sbrannen]

Isn't this superfluous if you're not adding additional Javadoc yourself?