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

scordio · 2025-02-09T16:31:37Z

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 · 2025-02-09T16:33:59Z

DefaultArgumentConverterTests still has a lot of duplicated test cases.

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

junit5/junit-jupiter-params/src/main/java/org/junit/jupiter/params/converter/DefaultArgumentConverter.java

Line 81 in bfd5ebb

return ConversionSupport.convert((String) source, targetType, classLoader);

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

marcphilipp · 2025-02-09T18:21:03Z

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

SGTM 👍

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.

marcphilipp · 2025-02-12T14:00:17Z

Thanks, @scordio! 👍

scordio force-pushed the conversion-support-testing branch from a8f5b4a to 528b570 Compare February 9, 2025 16:34

scordio mentioned this pull request Feb 9, 2025

Introduce ConversionService in junit-platform-commons #4219

Draft

6 tasks

scordio force-pushed the conversion-support-testing branch 2 times, most recently from fe36544 to afe8ec1 Compare February 12, 2025 13:08

scordio force-pushed the conversion-support-testing branch from afe8ec1 to 0e5b639 Compare February 12, 2025 13:22

scordio marked this pull request as ready for review February 12, 2025 13:23

marcphilipp approved these changes Feb 12, 2025

View reviewed changes

marcphilipp merged commit 779a46f into junit-team:main Feb 12, 2025
15 checks passed

scordio deleted the conversion-support-testing branch February 12, 2025 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

scordio commented Feb 9, 2025 •

edited by marcphilipp

Loading

scordio commented Feb 9, 2025 •

edited

Loading

marcphilipp commented Feb 9, 2025

marcphilipp commented Feb 12, 2025

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

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

Conversation

scordio commented Feb 9, 2025 • edited by marcphilipp Loading

Overview

Definition of Done

scordio commented Feb 9, 2025 • edited Loading

marcphilipp commented Feb 9, 2025

marcphilipp commented Feb 12, 2025

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

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

scordio commented Feb 9, 2025 •

edited by marcphilipp

Loading

scordio commented Feb 9, 2025 •

edited

Loading