Move string-based argument conversion tests to platform-test

[scordio]

Overview

Prior to this commit, platform-tests had no dedicated tests for ConversionSupport. Instead, DefaultArgumentConverterTests from jupiter-tests provided the corresponding coverage.

Now, string-based tests are moved to ConversionSupportTests, while DefaultArgumentConverterTests verify that the delegation to ConversionSupport happens correctly.

See #4219 (comment).

For an easier review, here's the diff between DefaultArgumentConverterTests from main and ConversionSupportTests from this PR:

Diff

--- jupiter-tests/src/test/java/org/junit/jupiter/params/converter/DefaultArgumentConverterTests.java	2025-02-09 16:22:11
+++ platform-tests/src/test/java/org/junit/platform/commons/support/conversion/ConversionSupportTests.java	2025-02-09 17:34:21
@@ -8,15 +8,12 @@
  * https://www.eclipse.org/legal/epl-v20.html
  */
 
-package org.junit.jupiter.params.converter;
+package org.junit.platform.commons.support.conversion;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.when;
 
 import java.io.File;
-import java.lang.Thread.State;
 import java.lang.reflect.Method;
 import java.math.BigDecimal;
 import java.math.BigInteger;
@@ -46,18 +43,18 @@
 import java.util.concurrent.TimeUnit;
 
 import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.extension.ParameterContext;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.ValueSource;
 import org.junit.platform.commons.support.ReflectionSupport;
 import org.junit.platform.commons.test.TestClassLoader;
+import org.junit.platform.commons.util.ClassLoaderUtils;
 
 /**
- * Unit tests for {@link DefaultArgumentConverter}.
+ * Unit tests for {@link ConversionSupport}.
  *
- * @since 5.0
+ * @since 5.12
  */
-class DefaultArgumentConverterTests {
+class ConversionSupportTests {
 
 	@Test
 	void isAwareOfNull() {
@@ -67,32 +64,6 @@
 	}
 
 	@Test
-	void isAwareOfWrapperTypesForPrimitiveTypes() {
-		assertConverts(true, boolean.class, true);
-		assertConverts(false, boolean.class, false);
-		assertConverts((byte) 1, byte.class, (byte) 1);
-		assertConverts('o', char.class, 'o');
-		assertConverts((short) 1, short.class, (short) 1);
-		assertConverts(1, int.class, 1);
-		assertConverts(1L, long.class, 1L);
-		assertConverts(1.0f, float.class, 1.0f);
-		assertConverts(1.0d, double.class, 1.0d);
-	}
-
-	@Test
-	void isAwareOfWideningConversions() {
-		assertConverts((byte) 1, short.class, (byte) 1);
-		assertConverts((short) 1, int.class, (short) 1);
-		assertConverts((char) 1, int.class, (char) 1);
-		assertConverts((byte) 1, long.class, (byte) 1);
-		assertConverts(1, long.class, 1);
-		assertConverts((char) 1, float.class, (char) 1);
-		assertConverts(1, float.class, 1);
-		assertConverts(1L, double.class, 1L);
-		assertConverts(1.0f, double.class, 1.0f);
-	}
-
-	@Test
 	void convertsStringsToPrimitiveTypes() {
 		assertConverts("true", boolean.class, true);
 		assertConverts("false", boolean.class, false);
@@ -134,7 +105,7 @@
 	@ValueSource(classes = { char.class, boolean.class, short.class, byte.class, int.class, long.class, float.class,
 			double.class, void.class })
 	void throwsExceptionForNullToPrimitiveTypeConversion(Class<?> type) {
-		assertThatExceptionOfType(ArgumentConversionException.class) //
+		assertThatExceptionOfType(ConversionException.class) //
 				.isThrownBy(() -> convert(null, type)) //
 				.withMessage("Cannot convert null to primitive value of type " + type.getCanonicalName());
 	}
@@ -143,67 +114,47 @@
 	@ValueSource(classes = { Boolean.class, Character.class, Short.class, Byte.class, Integer.class, Long.class,
 			Float.class, Double.class })
 	void throwsExceptionWhenConvertingTheWordNullToPrimitiveWrapperType(Class<?> type) {
-		assertThatExceptionOfType(ArgumentConversionException.class) //
+		assertThatExceptionOfType(ConversionException.class) //
 				.isThrownBy(() -> convert("null", type)) //
 				.withMessage("Failed to convert String \"null\" to type " + type.getCanonicalName());
-		assertThatExceptionOfType(ArgumentConversionException.class) //
+		assertThatExceptionOfType(ConversionException.class) //
 				.isThrownBy(() -> convert("NULL", type)) //
 				.withMessage("Failed to convert String \"NULL\" to type " + type.getCanonicalName());
 	}
 
 	@Test
 	void throwsExceptionOnInvalidStringForPrimitiveTypes() {
-		assertThatExceptionOfType(ArgumentConversionException.class) //
+		assertThatExceptionOfType(ConversionException.class) //
 				.isThrownBy(() -> convert("ab", char.class)) //
 				.withMessage("Failed to convert String \"ab\" to type char") //
 				.havingCause() //
-				.havingCause() //
 				.withMessage("String must have length of 1: ab");
 
-		assertThatExceptionOfType(ArgumentConversionException.class) //
+		assertThatExceptionOfType(ConversionException.class) //
 				.isThrownBy(() -> convert("tru", boolean.class)) //
 				.withMessage("Failed to convert String \"tru\" to type boolean") //
 				.havingCause() //
-				.havingCause() //
 				.withMessage("String must be 'true' or 'false' (ignoring case): tru");
 
-		assertThatExceptionOfType(ArgumentConversionException.class) //
+		assertThatExceptionOfType(ConversionException.class) //
 				.isThrownBy(() -> convert("null", boolean.class)) //
 				.withMessage("Failed to convert String \"null\" to type boolean") //
 				.havingCause() //
-				.havingCause() //
 				.withMessage("String must be 'true' or 'false' (ignoring case): null");
 
-		assertThatExceptionOfType(ArgumentConversionException.class) //
+		assertThatExceptionOfType(ConversionException.class) //
 				.isThrownBy(() -> convert("NULL", boolean.class)) //
 				.withMessage("Failed to convert String \"NULL\" to type boolean") //
 				.havingCause() //
-				.havingCause() //
 				.withMessage("String must be 'true' or 'false' (ignoring case): NULL");
 	}
 
 	@Test
 	void throwsExceptionWhenImplicitConversionIsUnsupported() {
-		assertThatExceptionOfType(ArgumentConversionException.class) //
+		assertThatExceptionOfType(ConversionException.class) //
 				.isThrownBy(() -> convert("foo", Enigma.class)) //
 				.withMessage("No built-in converter for source type java.lang.String and target type %s",
 					Enigma.class.getName());
-
-		assertThatExceptionOfType(ArgumentConversionException.class) //
-				.isThrownBy(() -> convert(new Enigma(), int[].class)) //
-				.withMessage("No built-in converter for source type %s and target type int[]", Enigma.class.getName());
-
-		assertThatExceptionOfType(ArgumentConversionException.class) //
-				.isThrownBy(() -> convert(new long[] {}, int[].class)) //
-				.withMessage("No built-in converter for source type long[] and target type int[]");
-
-		assertThatExceptionOfType(ArgumentConversionException.class) //
-				.isThrownBy(() -> convert(new String[] {}, boolean.class)) //
-				.withMessage("No built-in converter for source type java.lang.String[] and target type boolean");
-
-		assertThatExceptionOfType(ArgumentConversionException.class) //
-				.isThrownBy(() -> convert(Class.class, int[].class)) //
-				.withMessage("No built-in converter for source type java.lang.Class and target type int[]");
 	}
 
 	/**
@@ -262,7 +213,7 @@
 	void convertsStringToClass() {
 		assertConverts("java.lang.Integer", Class.class, Integer.class);
 		assertConverts("java.lang.Void", Class.class, Void.class);
-		assertConverts("java.lang.Thread$State", Class.class, State.class);
+		assertConverts("java.lang.Thread$State", Class.class, Thread.State.class);
 		assertConverts("byte", Class.class, byte.class);
 		assertConverts("void", Class.class, void.class);
 		assertConverts("char[]", Class.class, char[].class);
@@ -281,7 +232,7 @@
 			var declaringExecutable = ReflectionSupport.findMethod(customType, "foo").get();
 			assertThat(declaringExecutable.getDeclaringClass().getClassLoader()).isSameAs(testClassLoader);
 
-			var clazz = (Class<?>) convert(customTypeName, Class.class, parameterContext(declaringExecutable));
+			var clazz = (Class<?>) convert(customTypeName, Class.class, classLoader(declaringExecutable));
 			assertThat(clazz).isNotEqualTo(Enigma.class);
 			assertThat(clazz).isEqualTo(customType);
 			assertThat(clazz.getClassLoader()).isSameAs(testClassLoader);
@@ -358,7 +309,7 @@
 
 	// -------------------------------------------------------------------------
 
-	private void assertConverts(Object input, Class<?> targetClass, Object expectedOutput) {
+	private void assertConverts(String input, Class<?> targetClass, Object expectedOutput) {
 		var result = convert(input, targetClass);
 
 		assertThat(result) //
@@ -366,23 +317,21 @@
 				.isEqualTo(expectedOutput);
 	}
 
-	private Object convert(Object input, Class<?> targetClass) {
-		return convert(input, targetClass, parameterContext());
+	private Object convert(String input, Class<?> targetClass) {
+		return convert(input, targetClass, classLoader());
 	}
 
-	private Object convert(Object input, Class<?> targetClass, ParameterContext parameterContext) {
-		return DefaultArgumentConverter.INSTANCE.convert(input, targetClass, parameterContext);
+	private Object convert(String input, Class<?> targetClass, ClassLoader classLoader) {
+		return ConversionSupport.convert(input, targetClass, classLoader);
 	}
 
-	private static ParameterContext parameterContext() {
-		Method declaringExecutable = ReflectionSupport.findMethod(DefaultArgumentConverterTests.class, "foo").get();
-		return parameterContext(declaringExecutable);
+	private static ClassLoader classLoader() {
+		Method declaringExecutable = ReflectionSupport.findMethod(ConversionSupportTests.class, "foo").get();
+		return classLoader(declaringExecutable);
 	}
 
-	private static ParameterContext parameterContext(Method declaringExecutable) {
-		ParameterContext parameterContext = mock();
-		when(parameterContext.getDeclaringExecutable()).thenReturn(declaringExecutable);
-		return parameterContext;
+	private static ClassLoader classLoader(Method declaringExecutable) {
+		return ClassLoaderUtils.getClassLoader(declaringExecutable.getDeclaringClass());
 	}
 
 	@SuppressWarnings("unused")

---

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]

DefaultArgumentConverterTests still has a lot of duplicated test cases.

What if I wrap this invocation in a package-private method:

https://github.com/junit-team/junit5/blob/bfd5ebb0939efcb4f9a2e553a44332c3710b69c1/junit-jupiter-params/src/main/java/org/junit/jupiter/params/converter/DefaultArgumentConverter.java#L81

and I use Mockito to verify the delegation, cleaning up all the equivalent test cases from DefaultArgumentConverterTests?

[marcphilipp]

and I use Mockito to verify the delegation, cleaning up all the equivalent test cases from DefaultArgumentConverterTests?

SGTM 👍

[marcphilipp]

Thanks, @scordio! 👍