Polishing

Issue: junit-team#1178

junit-jupiter-api/src/main/java/org/junit/jupiter/api/DynamicContainer.java

@@ -71,13 +71,13 @@ public static DynamicContainer dynamicContainer(String displayName, Stream<? ext
 
 	/**
 	 * Factory for creating a new {@code DynamicContainer} for the supplied display
-	 * name, the test source uri, and stream of dynamic nodes.
+	 * name, the test source {@link URI}, and stream of dynamic nodes.
 	 *
 	 * <p>The stream of dynamic nodes must not contain {@code null} elements.
 	 *
 	 * @param displayName the display name for the dynamic container; never
 	 * {@code null} or blank
-	 * @param testSourceUri the test source uri for the dynamic test; can be {@code null}
+	 * @param testSourceUri the test source URI for the dynamic test; can be {@code null}
 	 * @param dynamicNodes stream of dynamic nodes to execute;
 	 * never {@code null}
 	 * @since 5.3

junit-jupiter-api/src/main/java/org/junit/jupiter/api/DynamicNode.java

@@ -42,16 +42,21 @@ public abstract class DynamicNode {
 
 	/**
 	 * Get the display name of this {@code DynamicNode}.
+	 *
+	 * @return the display name
 	 */
 	public String getDisplayName() {
 		return this.displayName;
 	}
 
 	/**
-	 * Get the optional test source of this {@code DynamicNode}.
+	 * Get the optional test source {@link URI} of this {@code DynamicNode}.
+	 *
+	 * @return an {@code Optional} containing the test source {@link URI};
+	 * never {@code null} but potentially empty
 	 * @since 5.3
 	 */
-	public Optional<URI> getTestSourceURI() {
+	public Optional<URI> getTestSourceUri() {
 		return Optional.ofNullable(testSourceUri);
 	}
 

junit-jupiter-api/src/main/java/org/junit/jupiter/api/DynamicTest.java

@@ -65,11 +65,11 @@ public static DynamicTest dynamicTest(String displayName, Executable executable)
 
 	/**
 	 * Factory for creating a new {@code DynamicTest} for the supplied display
-	 * name, the test source uri, and executable code block.
+	 * name, the test source {@link URI}, and executable code block.
 	 *
 	 * @param displayName the display name for the dynamic test; never
 	 * {@code null} or blank
-	 * @param testSourceUri the test source uri for the dynamic test; can be {@code null}
+	 * @param testSourceUri the test source URI for the dynamic test; can be {@code null}
 	 * @param executable the executable code block for the dynamic test;
 	 * never {@code null}
 	 * @since 5.3

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/TestFactoryTestDescriptor.java

@@ -81,15 +81,15 @@ protected void invokeTestMethod(JupiterEngineExecutionContext context, DynamicTe
 			Object instance = extensionContext.getRequiredTestInstance();
 			Object testFactoryMethodResult = executableInvoker.invoke(getTestMethod(), instance, extensionContext,
 				context.getExtensionRegistry());
-			TestSource source = getSource().orElseThrow(
+			TestSource defaultTestSource = getSource().orElseThrow(
 				() -> new JUnitException("Illegal state: TestSource must be present"));
 			try (Stream<DynamicNode> dynamicNodeStream = toDynamicNodeStream(testFactoryMethodResult)) {
 				int index = 1;
 				Iterator<DynamicNode> iterator = dynamicNodeStream.iterator();
 				while (iterator.hasNext()) {
 					DynamicNode dynamicNode = iterator.next();
 					Optional<JupiterTestDescriptor> descriptor = createDynamicDescriptor(this, dynamicNode, index++,
-						source, getDynamicDescendantFilter());
+						defaultTestSource, getDynamicDescendantFilter());
 					descriptor.ifPresent(dynamicTestExecutor::execute);
 				}
 			}
@@ -117,10 +117,11 @@ private JUnitException invalidReturnTypeException(Throwable cause) {
 	}
 
 	static Optional<JupiterTestDescriptor> createDynamicDescriptor(JupiterTestDescriptor parent, DynamicNode node,
-			int index, TestSource testSource, DynamicDescendantFilter dynamicDescendantFilter) {
+			int index, TestSource defaultTestSource, DynamicDescendantFilter dynamicDescendantFilter) {
 		UniqueId uniqueId;
 		Supplier<JupiterTestDescriptor> descriptorCreator;
-		TestSource source = computeOptionalTestSource(node).orElse(testSource);
+		Optional<TestSource> optionalTestSource = node.getTestSourceUri().map(UriSource::from);
+		TestSource source = optionalTestSource.orElse(defaultTestSource);
 		if (node instanceof DynamicTest) {
 			DynamicTest test = (DynamicTest) node;
 			uniqueId = parent.getUniqueId().append(DYNAMIC_TEST_SEGMENT_TYPE, "#" + index);
@@ -140,8 +141,4 @@ static Optional<JupiterTestDescriptor> createDynamicDescriptor(JupiterTestDescri
 		return Optional.empty();
 	}
 
-	static Optional<TestSource> computeOptionalTestSource(DynamicNode node) {
-		return node.getTestSourceURI().map(UriSource::from);
-	}
-
 }

junit-jupiter-engine/src/test/java/org/junit/jupiter/api/DynamicTestTests.java

@@ -88,8 +88,11 @@ void reflectiveOperationThrowingInvocationTargetException() {
 	@Test
 	void testSourceUriIsNotPresentByDefault() {
 		DynamicTest test = dynamicTest("foo", nix);
-		assertThat(test.getTestSourceURI()).isNotPresent();
-		assertThat(dynamicContainer("bar", Stream.of(test)).getTestSourceURI()).isNotPresent();
+		assertThat(test.getTestSourceUri()).isNotPresent();
+		assertThat(test.toString()).isEqualTo("DynamicTest [displayName = 'foo', testSourceUri = null]");
+		DynamicContainer container = dynamicContainer("bar", Stream.of(test));
+		assertThat(container.getTestSourceUri()).isNotPresent();
+		assertThat(container.toString()).isEqualTo("DynamicContainer [displayName = 'bar', testSourceUri = null]");
 	}
 
 	@Test
@@ -99,8 +102,11 @@ void testSourceUriIsReturnedWhenSupplied() {
 		URI containerSourceUri = URI.create("other://container");
 		DynamicContainer container = dynamicContainer("bar", containerSourceUri, Stream.of(test));
 
-		assertThat(test.getTestSourceURI().get()).isSameAs(testSourceUri);
-		assertThat(container.getTestSourceURI().get()).isSameAs(containerSourceUri);
+		assertThat(test.getTestSourceUri().get()).isSameAs(testSourceUri);
+		assertThat(test.toString()).isEqualTo("DynamicTest [displayName = 'foo', testSourceUri = any://test]");
+		assertThat(container.getTestSourceUri().get()).isSameAs(containerSourceUri);
+		assertThat(container.toString()).isEqualTo(
+			"DynamicContainer [displayName = 'bar', testSourceUri = other://container]");
 	}
 
 	private void assert1Equals48Directly() {

junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/DefaultUriSource.java

@@ -20,7 +20,7 @@
 import org.junit.platform.commons.util.ToStringBuilder;
 
 /**
- * Default uri-based test source implementation.
+ * Default implementation of {@link UriSource}.
  *
  * @since 1.3
  */
@@ -32,7 +32,7 @@ class DefaultUriSource implements UriSource {
 	private final URI uri;
 
 	DefaultUriSource(URI uri) {
-		this.uri = Preconditions.notNull(uri, "uri must not be null");
+		this.uri = Preconditions.notNull(uri, "URI must not be null");
 	}
 
 	@Override

junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/FilePosition.java

@@ -17,6 +17,7 @@
 import java.util.Optional;
 
 import org.apiguardian.api.API;
+import org.junit.platform.commons.logging.Logger;
 import org.junit.platform.commons.logging.LoggerFactory;
 import org.junit.platform.commons.util.Preconditions;
 import org.junit.platform.commons.util.StringUtils;
@@ -33,11 +34,14 @@ public class FilePosition implements Serializable {
 
 	private static final long serialVersionUID = 1L;
 
+	private static final Logger logger = LoggerFactory.getLogger(FilePosition.class);
+
 	/**
 	 * Create a new {@code FilePosition} using the supplied {@code line} number
 	 * and an undefined column number.
 	 *
 	 * @param line the line number; must be greater than zero
+	 * @return a {@link FilePosition} with the given line number
 	 */
 	public static FilePosition from(int line) {
 		return new FilePosition(line);
@@ -49,13 +53,14 @@ public static FilePosition from(int line) {
 	 *
 	 * @param line the line number; must be greater than zero
 	 * @param column the column number; must be greater than zero
+	 * @return a {@link FilePosition} with the given line and column numbers
 	 */
 	public static FilePosition from(int line, int column) {
 		return new FilePosition(line, column);
 	}
 
 	/**
-	 * Create an optional {@code FilePosition} parsing the supplied
+	 * Create an optional {@code FilePosition} by parsing the supplied
 	 * {@code query} string.
 	 *
 	 * <p>Examples of valid {@code query} strings:
@@ -65,6 +70,8 @@ public static FilePosition from(int line, int column) {
 	 * </ul>
 	 *
 	 * @param query the query string; may be {@code null}
+	 * @return an {@link Optional} containing a {@link FilePosition} with
+	 * the parsed line and column numbers; potentially empty
 	 * @since 1.3
 	 */
 	public static Optional<FilePosition> fromQuery(String query) {
@@ -88,7 +95,7 @@ public static Optional<FilePosition> fromQuery(String query) {
 				}
 			}
 			catch (IllegalArgumentException e) {
-				LoggerFactory.getLogger(FilePosition.class).debug(e, () -> "parsing query failed: " + query);
+				logger.debug(e, () -> "Failed to parse 'line' and/or 'column' from query string: " + query);
 				// fall-through and continue
 			}
 			if (line != null) {

junit-platform-engine/src/main/java/org/junit/platform/engine/support/descriptor/UriSource.java

@@ -40,38 +40,39 @@ public interface UriSource extends TestSource {
 	URI getUri();
 
 	/**
-	 * Create a new {@code UriSource} using the supplied {@code uri}.
+	 * Create a new {@code UriSource} using the supplied {@code URI}.
 	 * <p>
 	 * This implementation tries resolve the {@code uri} to local file
 	 * system path-based source first. If that fails for any reason, an
-	 * instance of a simple default uri source class storing the supplied
-	 * {@code uri} <em>as-is</em> is returned.
+	 * instance of the default {@code UriSource} implementation storing
+	 * the supplied {@code URI} <em>as-is</em> is returned.
 	 *
-	 * @param uri the uri instance; must not be {@code null}
+	 * @param uri the URI instance; must not be {@code null}
 	 * @return a uri source instance
 	 * @since 1.3
 	 * @see org.junit.platform.engine.support.descriptor.FileSource
 	 * @see org.junit.platform.engine.support.descriptor.DirectorySource
 	 */
 	static UriSource from(final URI uri) {
-		Preconditions.notNull(uri, "uri must not be null");
+		Preconditions.notNull(uri, "URI must not be null");
 		try {
-			URI pathBasedUri = uri;
-			String query = pathBasedUri.getQuery();
+			URI pathBasedUriWithoutQuery = uri;
+			String query = uri.getQuery();
 			if (StringUtils.isNotBlank(query)) {
-				String s = pathBasedUri.toString();
-				pathBasedUri = URI.create(s.substring(0, s.indexOf('?')));
+				String uriAsString = uri.toString();
+				pathBasedUriWithoutQuery = URI.create(uriAsString.substring(0, uriAsString.indexOf('?')));
 			}
-			Path path = Paths.get(pathBasedUri);
+			Path path = Paths.get(pathBasedUriWithoutQuery);
 			if (Files.isRegularFile(path)) {
 				return FileSource.from(path.toFile(), FilePosition.fromQuery(query).orElse(null));
 			}
 			if (Files.isDirectory(path)) {
 				return DirectorySource.from(path.toFile());
 			}
 		}
-		catch (IllegalArgumentException e) {
-			LoggerFactory.getLogger(UriSource.class).debug(e, () -> "uri not path-based: " + uri);
+		catch (RuntimeException e) {
+			LoggerFactory.getLogger(UriSource.class).debug(e, () -> String.format(
+				"The supplied URI [%s] is not path-based. Falling back to default UriSource implementation.", uri));
 		}
 		// store uri as-is
 		return new DefaultUriSource(uri);

platform-tests/src/test/java/org/junit/platform/engine/support/descriptor/FilePositionTests.java

@@ -13,6 +13,7 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.params.provider.Arguments.arguments;
 
 import java.util.Optional;
 import java.util.stream.Stream;
@@ -81,13 +82,13 @@ void filePositionFromQuery(String query, int expectedLine, int expectedColumn) {
 	@SuppressWarnings("unused")
 	static Stream<Arguments> filePositionFromQuery() {
 		return Stream.of( //
-			Arguments.of(null, -1, -1), //
-			Arguments.of("?!", -1, -1), //
-			Arguments.of("line=ZZ", -1, -1), //
-			Arguments.of("line=42", 42, -1), //
-			Arguments.of("line=42&line=24", 24, -1), //
-			Arguments.of("line=42&column=99", 42, 99), //
-			Arguments.of("line=42&column=ZZ", 42, -1) //
+			arguments(null, -1, -1), //
+			arguments("?!", -1, -1), //
+			arguments("line=ZZ", -1, -1), //
+			arguments("line=42", 42, -1), //
+			arguments("line=42&line=24", 24, -1), //
+			arguments("line=42&column=99", 42, 99), //
+			arguments("line=42&column=ZZ", 42, -1) //
 		);
 	}