Prefer public methods for Vintage MethodSources

marcphilipp · marcphilipp · commit f5fffa1062be · 2019-06-05T20:24:28.000+02:00
Since JUnit requires test methods to be public, prefer such methods over other methods with the same name when creating the `MethodSource` for a JUnit 4 `Description` instance. Resolves #1904.
diff --git a/documentation/src/docs/asciidoc/release-notes/release-notes-5.5.0-RC1.adoc b/documentation/src/docs/asciidoc/release-notes/release-notes-5.5.0-RC1.adoc
@@ -99,3 +99,5 @@ on GitHub.
 
 * `junit:junit` is now a compile-scoped dependency of `junit-vintage-engine` to allow for
   easier dependency management in Maven POMs.
+* Methods that are `public` are now preferred over other methods with the same name in the
+  same test class when creating `MethodSources` for JUnit 4 `Descriptions`.
diff --git a/junit-vintage-engine/src/main/java/org/junit/vintage/engine/descriptor/VintageTestDescriptor.java b/junit-vintage-engine/src/main/java/org/junit/vintage/engine/descriptor/VintageTestDescriptor.java
@@ -12,8 +12,8 @@
 
 import static java.util.Arrays.stream;
 import static java.util.function.Predicate.isEqual;
+import static java.util.stream.Collectors.toList;
 import static org.apiguardian.api.API.Status.INTERNAL;
-import static org.junit.platform.commons.util.CollectionUtils.getOnlyElement;
 import static org.junit.platform.commons.util.FunctionUtils.where;
 import static org.junit.platform.commons.util.ReflectionUtils.findMethods;
 import static org.junit.platform.commons.util.StringUtils.isNotBlank;
@@ -28,6 +28,7 @@
 
 import org.apiguardian.api.API;
 import org.junit.experimental.categories.Category;
+import org.junit.platform.commons.support.ModifierSupport;
 import org.junit.platform.commons.util.ReflectionUtils;
 import org.junit.platform.engine.TestDescriptor;
 import org.junit.platform.engine.TestSource;
@@ -148,25 +149,33 @@ private static TestSource toTestSource(Description description) {
 		if (testClass != null) {
 			String methodName = description.getMethodName();
 			if (methodName != null) {
-				MethodSource methodSource = toMethodSource(testClass, methodName);
-				if (methodSource != null) {
-					return methodSource;
+				Method method = findMethod(testClass, methodName);
+				if (method != null) {
+					return MethodSource.from(testClass, method);
 				}
 			}
 			return ClassSource.from(testClass);
 		}
 		return null;
 	}
 
-	private static MethodSource toMethodSource(Class<?> testClass, String methodName) {
+	private static Method findMethod(Class<?> testClass, String methodName) {
 		if (methodName.contains("[") && methodName.endsWith("]")) {
 			// special case for parameterized tests
-			return toMethodSource(testClass, methodName.substring(0, methodName.indexOf("[")));
+			return findMethod(testClass, methodName.substring(0, methodName.indexOf("[")));
 		}
-		else {
-			List<Method> methods = findMethods(testClass, where(Method::getName, isEqual(methodName)));
-			return (methods.size() == 1) ? MethodSource.from(testClass, getOnlyElement(methods)) : null;
+		List<Method> methods = findMethods(testClass, where(Method::getName, isEqual(methodName)));
+		if (methods.isEmpty()) {
+			return null;
+		}
+		if (methods.size() == 1) {
+			return methods.get(0);
 		}
+		methods = methods.stream().filter(ModifierSupport::isPublic).collect(toList());
+		if (methods.size() == 1) {
+			return methods.get(0);
+		}
+		return null;
 	}
 
 }
diff --git a/junit-vintage-engine/src/test/java/org/junit/vintage/engine/VintageTestEngineDiscoveryTests.java b/junit-vintage-engine/src/test/java/org/junit/vintage/engine/VintageTestEngineDiscoveryTests.java
@@ -60,8 +60,9 @@
 import org.junit.vintage.engine.samples.junit4.IgnoredJUnit4TestCase;
 import org.junit.vintage.engine.samples.junit4.JUnit4SuiteWithPlainJUnit4TestCaseWithSingleTestWhichIsIgnored;
 import org.junit.vintage.engine.samples.junit4.JUnit4SuiteWithTwoTestCases;
+import org.junit.vintage.engine.samples.junit4.JUnit4TestCaseWithDistinguishableOverloadedMethod;
+import org.junit.vintage.engine.samples.junit4.JUnit4TestCaseWithIndistinguishableOverloadedMethod;
 import org.junit.vintage.engine.samples.junit4.JUnit4TestCaseWithNotFilterableRunner;
-import org.junit.vintage.engine.samples.junit4.JUnit4TestCaseWithOverloadedMethod;
 import org.junit.vintage.engine.samples.junit4.JUnit4TestCaseWithRunnerWithCustomUniqueIds;
 import org.junit.vintage.engine.samples.junit4.ParameterizedTestCase;
 import org.junit.vintage.engine.samples.junit4.PlainJUnit4TestCaseWithFiveTestMethods;
@@ -196,8 +197,8 @@ void resolvesJUnit4SuiteWithPlainJUnit4TestCaseWithSingleTestWhichIsIgnored() th
 	}
 
 	@Test
-	void resolvesJUnit4TestCaseWithOverloadedMethod() {
-		Class<?> testClass = JUnit4TestCaseWithOverloadedMethod.class;
+	void resolvesJUnit4TestCaseWithIndistinguishableOverloadedMethod() {
+		Class<?> testClass = JUnit4TestCaseWithIndistinguishableOverloadedMethod.class;
 		LauncherDiscoveryRequest discoveryRequest = discoveryRequestForClass(testClass);
 
 		TestDescriptor engineDescriptor = discoverTests(discoveryRequest);
@@ -221,6 +222,24 @@ void resolvesJUnit4TestCaseWithOverloadedMethod() {
 		assertClassSource(testClass, testMethodDescriptor);
 	}
 
+	@Test
+	void resolvesJUnit4TestCaseWithDistinguishableOverloadedMethod() throws Exception {
+		Class<?> testClass = JUnit4TestCaseWithDistinguishableOverloadedMethod.class;
+		LauncherDiscoveryRequest discoveryRequest = discoveryRequestForClass(testClass);
+
+		TestDescriptor engineDescriptor = discoverTests(discoveryRequest);
+
+		TestDescriptor runnerDescriptor = getOnlyElement(engineDescriptor.getChildren());
+		assertRunnerTestDescriptor(runnerDescriptor, testClass);
+
+		List<TestDescriptor> testMethodDescriptors = new ArrayList<>(runnerDescriptor.getChildren());
+
+		TestDescriptor testMethodDescriptor = getOnlyElement(testMethodDescriptors);
+		assertEquals("test", testMethodDescriptor.getDisplayName());
+		assertEquals(VintageUniqueIdBuilder.uniqueIdForMethod(testClass, "test"), testMethodDescriptor.getUniqueId());
+		assertMethodSource(testClass.getMethod("test"), testMethodDescriptor);
+	}
+
 	@Test
 	void doesNotResolvePlainOldJavaClassesWithoutAnyTest() {
 		assertYieldsNoDescriptors(PlainOldJavaClassWithoutAnyTestsTestCase.class);
@@ -283,7 +302,7 @@ void resolvesApplyingClassNameFilters() throws Exception {
 		assertThat(engineDescriptor.getChildren())
 			.extracting(TestDescriptor::getDisplayName)
 			.contains(PlainJUnit4TestCaseWithSingleTestWhichFails.class.getSimpleName())
-			.doesNotContain(JUnit4TestCaseWithOverloadedMethod.class.getSimpleName())
+			.doesNotContain(JUnit4TestCaseWithIndistinguishableOverloadedMethod.class.getSimpleName())
 			.doesNotContain(PlainJUnit3TestCaseWithSingleTestWhichFails.class.getSimpleName());
 		// @formatter:on
 	}
diff --git a/junit-vintage-engine/src/test/java/org/junit/vintage/engine/VintageTestEngineExecutionTests.java b/junit-vintage-engine/src/test/java/org/junit/vintage/engine/VintageTestEngineExecutionTests.java
@@ -69,7 +69,7 @@
 import org.junit.vintage.engine.samples.junit4.JUnit4TestCaseWithErrorInBeforeClass;
 import org.junit.vintage.engine.samples.junit4.JUnit4TestCaseWithExceptionThrowingRunner;
 import org.junit.vintage.engine.samples.junit4.JUnit4TestCaseWithFailingDescriptionThatIsNotReportedAsFinished;
-import org.junit.vintage.engine.samples.junit4.JUnit4TestCaseWithOverloadedMethod;
+import org.junit.vintage.engine.samples.junit4.JUnit4TestCaseWithIndistinguishableOverloadedMethod;
 import org.junit.vintage.engine.samples.junit4.JUnit4TestCaseWithRunnerWithCustomUniqueIds;
 import org.junit.vintage.engine.samples.junit4.MalformedJUnit4TestCase;
 import org.junit.vintage.engine.samples.junit4.ParameterizedTestCase;
@@ -282,15 +282,19 @@ void executesJUnit4TestCaseWithErrorInAfterClass() {
 
 	@Test
 	void executesJUnit4TestCaseWithOverloadedMethod() {
-		Class<?> testClass = JUnit4TestCaseWithOverloadedMethod.class;
+		Class<?> testClass = JUnit4TestCaseWithIndistinguishableOverloadedMethod.class;
 
 		execute(testClass).assertEventsMatchExactly( //
 			event(engine(), started()), //
 			event(container(testClass), started()), //
-			event(test("theory(" + JUnit4TestCaseWithOverloadedMethod.class.getName() + ")[0]"), started()), //
-			event(test("theory(" + JUnit4TestCaseWithOverloadedMethod.class.getName() + ")[0]"), finishedWithFailure()), //
-			event(test("theory(" + JUnit4TestCaseWithOverloadedMethod.class.getName() + ")[1]"), started()), //
-			event(test("theory(" + JUnit4TestCaseWithOverloadedMethod.class.getName() + ")[1]"), finishedWithFailure()), //
+			event(test("theory(" + JUnit4TestCaseWithIndistinguishableOverloadedMethod.class.getName() + ")[0]"),
+				started()), //
+			event(test("theory(" + JUnit4TestCaseWithIndistinguishableOverloadedMethod.class.getName() + ")[0]"),
+				finishedWithFailure()), //
+			event(test("theory(" + JUnit4TestCaseWithIndistinguishableOverloadedMethod.class.getName() + ")[1]"),
+				started()), //
+			event(test("theory(" + JUnit4TestCaseWithIndistinguishableOverloadedMethod.class.getName() + ")[1]"),
+				finishedWithFailure()), //
 			event(container(testClass), finishedSuccessfully()), //
 			event(engine(), finishedSuccessfully()));
 	}
diff --git a/junit-vintage-engine/src/test/java/org/junit/vintage/engine/samples/junit4/JUnit4TestCaseWithDistinguishableOverloadedMethod.java b/junit-vintage-engine/src/test/java/org/junit/vintage/engine/samples/junit4/JUnit4TestCaseWithDistinguishableOverloadedMethod.java
@@ -0,0 +1,34 @@
+/*
+ * Copyright 2015-2019 the original author or authors.
+ *
+ * All rights reserved. This program and the accompanying materials are
+ * made available under the terms of the Eclipse Public License v2.0 which
+ * accompanies this distribution and is available at
+ *
+ * https://www.eclipse.org/legal/epl-v20.html
+ */
+
+package org.junit.vintage.engine.samples.junit4;
+
+import static org.junit.Assert.fail;
+
+import org.junit.Test;
+import org.junit.experimental.theories.Theories;
+import org.junit.runner.RunWith;
+
+/**
+ * @since 5.5
+ */
+@RunWith(Theories.class)
+public class JUnit4TestCaseWithDistinguishableOverloadedMethod {
+
+	@Test
+	public void test() {
+		test("foo");
+	}
+
+	private void test(String message) {
+		fail(message);
+	}
+
+}
diff --git a/junit-vintage-engine/src/test/java/org/junit/vintage/engine/samples/junit4/JUnit4TestCaseWithIndistinguishableOverloadedMethod.java b/junit-vintage-engine/src/test/java/org/junit/vintage/engine/samples/junit4/JUnit4TestCaseWithIndistinguishableOverloadedMethod.java
@@ -21,7 +21,7 @@
  * @since 4.12
  */
 @RunWith(Theories.class)
-public class JUnit4TestCaseWithOverloadedMethod {
+public class JUnit4TestCaseWithIndistinguishableOverloadedMethod {
 
 	@DataPoint
 	public static int MAGIC_NUMBER = 42;
