Apply method predicate before searching type hierarchy

Prior to this commit, findMethods() and streamMethods() in
ReflectionSupport as well as findAnnotatedMethods() in
AnnotationSupport first searched for all methods in the type hierarchy
and then applied the user-supplied predicate (or "is annotated?"
predicate) afterwards.

This resulted in methods in subclasses incorrectly "shadowing"
package-private methods in superclasses (in a different package) even
if the predicate would otherwise exclude the method in such a subclass.

For example, given a superclass that declares a package-private static
@⁠BeforeAll "before()" method and a subclass (in a different package)
that declares a @⁠BeforeEach "before()" method, when JUnit Jupiter
looked up @⁠BeforeAll methods for the subclass, the @⁠BeforeAll
"before()" method in the superclass was not found because the
@⁠BeforeEach "before()" method shadowed it based solely on the method
signature, ignoring the type of annotation sought.

To address that, this commit modifies the internal search algorithms in
ReflectionUtils so that method predicates are applied while searching
the hierarchy for methods.

Closes #3498
Closes #3500

Author: sbrannen

documentation/src/docs/asciidoc/release-notes/release-notes-5.10.1.adoc

@@ -15,14 +15,22 @@ JUnit repository on GitHub.
 
 ==== Bug Fixes
 
-* ❓
+* Method predicates are now applied while searching the type hierarchy. This fixes bugs
+  in `findMethods(...)` and `streamMethods(...)` in `ReflectionSupport` as well as
+  `findAnnotatedMethods(...)` in `AnnotationSupport`.
+  - See link:https://github.com/junit-team/junit5/issues/3498[issue 3498] for details.
 
 
 [[release-notes-5.10.1-junit-jupiter]]
 === JUnit Jupiter
 
 ==== Bug Fixes
 
+* A package-private class-level lifecycle method annotated with `@BeforeAll` or
+  `@AfterAll` is no longer _shadowed_ by a method-level lifecycle method annotated with
+  `@BeforeEach` or `@AfterEach` when the method-level lifecycle method resides in a
+  different package and has the same name as the class-level lifecycle method.
+  - See link:https://github.com/junit-team/junit5/issues/3498[issue 3498] for details.
 * The `RandomNumberExtension` example in the
   <<../user-guide/index.adoc#extensions-RandomNumberExtension, User Guide>> has been
   updated to properly support `Integer` types as well as non-static field injection.

junit-platform-commons/src/main/java/org/junit/platform/commons/util/ReflectionUtils.java

@@ -1489,29 +1489,27 @@ public static Stream<Method> streamMethods(Class<?> clazz, Predicate<Method> pre
 		Preconditions.notNull(predicate, "Predicate must not be null");
 		Preconditions.notNull(traversalMode, "HierarchyTraversalMode must not be null");
 
-		// @formatter:off
-		return findAllMethodsInHierarchy(clazz, traversalMode).stream()
-				.filter(predicate)
-				.distinct();
-		// @formatter:on
+		return findAllMethodsInHierarchy(clazz, predicate, traversalMode).stream().distinct();
 	}
 
 	/**
 	 * Find all non-synthetic methods in the superclass and interface hierarchy,
-	 * excluding Object.
+	 * excluding Object, that match the specified {@code predicate}.
 	 */
-	private static List<Method> findAllMethodsInHierarchy(Class<?> clazz, HierarchyTraversalMode traversalMode) {
+	private static List<Method> findAllMethodsInHierarchy(Class<?> clazz, Predicate<Method> predicate,
+			HierarchyTraversalMode traversalMode) {
+
 		Preconditions.notNull(clazz, "Class must not be null");
 		Preconditions.notNull(traversalMode, "HierarchyTraversalMode must not be null");
 
 		// @formatter:off
 		List<Method> localMethods = getDeclaredMethods(clazz, traversalMode).stream()
-				.filter(method -> !method.isSynthetic())
+				.filter(predicate.and(method -> !method.isSynthetic()))
 				.collect(toList());
-		List<Method> superclassMethods = getSuperclassMethods(clazz, traversalMode).stream()
+		List<Method> superclassMethods = getSuperclassMethods(clazz, predicate, traversalMode).stream()
 				.filter(method -> !isMethodShadowedByLocalMethods(method, localMethods))
 				.collect(toList());
-		List<Method> interfaceMethods = getInterfaceMethods(clazz, traversalMode).stream()
+		List<Method> interfaceMethods = getInterfaceMethods(clazz, predicate, traversalMode).stream()
 				.filter(method -> !isMethodShadowedByLocalMethods(method, localMethods))
 				.collect(toList());
 		// @formatter:on
@@ -1647,16 +1645,18 @@ private static int defaultMethodSorter(Method method1, Method method2) {
 		return comparison;
 	}
 
-	private static List<Method> getInterfaceMethods(Class<?> clazz, HierarchyTraversalMode traversalMode) {
+	private static List<Method> getInterfaceMethods(Class<?> clazz, Predicate<Method> predicate,
+			HierarchyTraversalMode traversalMode) {
+
 		List<Method> allInterfaceMethods = new ArrayList<>();
 		for (Class<?> ifc : clazz.getInterfaces()) {
 
 			// @formatter:off
 			List<Method> localInterfaceMethods = getMethods(ifc).stream()
-					.filter(m -> !isAbstract(m))
+					.filter(predicate.and(method -> !isAbstract(method)))
 					.collect(toList());
 
-			List<Method> superinterfaceMethods = getInterfaceMethods(ifc, traversalMode).stream()
+			List<Method> superinterfaceMethods = getInterfaceMethods(ifc, predicate, traversalMode).stream()
 					.filter(method -> !isMethodShadowedByLocalMethods(method, localInterfaceMethods))
 					.collect(toList());
 			// @formatter:on
@@ -1706,12 +1706,14 @@ private static boolean isFieldShadowedByLocalFields(Field field, List<Field> loc
 		return localFields.stream().anyMatch(local -> local.getName().equals(field.getName()));
 	}
 
-	private static List<Method> getSuperclassMethods(Class<?> clazz, HierarchyTraversalMode traversalMode) {
+	private static List<Method> getSuperclassMethods(Class<?> clazz, Predicate<Method> predicate,
+			HierarchyTraversalMode traversalMode) {
+
 		Class<?> superclass = clazz.getSuperclass();
 		if (!isSearchable(superclass)) {
 			return Collections.emptyList();
 		}
-		return findAllMethodsInHierarchy(superclass, traversalMode);
+		return findAllMethodsInHierarchy(superclass, predicate, traversalMode);
 	}
 
 	private static boolean isMethodShadowedByLocalMethods(Method method, List<Method> localMethods) {

platform-tests/src/test/java/org/junit/platform/commons/util/AnnotationUtilsTests.java

@@ -36,13 +36,18 @@
 import java.lang.annotation.Target;
 import java.lang.reflect.AnnotatedElement;
 import java.lang.reflect.Field;
+import java.lang.reflect.Method;
 import java.math.BigDecimal;
 import java.util.List;
 import java.util.Optional;
 import java.util.function.Predicate;
 
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.platform.commons.PreconditionViolationException;
+import org.junit.platform.commons.util.pkg1.SuperclassWithStaticPackagePrivateBeforeMethod;
+import org.junit.platform.commons.util.pkg1.subpkg.SubclassWithNonStaticPackagePrivateBeforeMethod;
 
 /**
  * Unit tests for {@link AnnotationUtils}.
@@ -380,6 +385,28 @@ void findAnnotatedMethodsForAnnotationUsedInClassAndSuperclassHierarchyDown() th
 		assertThat(methods.subList(1, 3)).containsOnly(method1, method3);
 	}
 
+	/**
+	 * @see https://github.com/junit-team/junit5/issues/3498
+	 */
+	@Test
+	void findAnnotatedMethodsAppliesPredicateBeforeSearchingTypeHierarchy() throws Exception {
+		final String BEFORE = "before";
+		Class<?> superclass = SuperclassWithStaticPackagePrivateBeforeMethod.class;
+		Method beforeAllMethod = superclass.getDeclaredMethod(BEFORE);
+		Class<?> subclass = SubclassWithNonStaticPackagePrivateBeforeMethod.class;
+		Method beforeEachMethod = subclass.getDeclaredMethod(BEFORE);
+
+		// Prerequisite
+		var methods = findAnnotatedMethods(superclass, BeforeAll.class, TOP_DOWN);
+		assertThat(methods).containsExactly(beforeAllMethod);
+
+		// Actual use cases for this test
+		methods = findAnnotatedMethods(subclass, BeforeAll.class, TOP_DOWN);
+		assertThat(methods).containsExactly(beforeAllMethod);
+		methods = findAnnotatedMethods(subclass, BeforeEach.class, TOP_DOWN);
+		assertThat(methods).containsExactly(beforeEachMethod);
+	}
+
 	@Test
 	void findAnnotatedMethodsForAnnotationUsedInInterface() throws Exception {
 		var interfaceMethod = InterfaceWithAnnotatedDefaultMethod.class.getDeclaredMethod("interfaceMethod");

platform-tests/src/test/java/org/junit/platform/commons/util/ReflectionUtilsTests.java

@@ -72,6 +72,8 @@
 import org.junit.platform.commons.util.ReflectionUtilsTests.OuterClass.StaticNestedSiblingClass;
 import org.junit.platform.commons.util.ReflectionUtilsTests.OuterClassImplementingInterface.InnerClassImplementingInterface;
 import org.junit.platform.commons.util.classes.CustomType;
+import org.junit.platform.commons.util.pkg1.SuperclassWithStaticPackagePrivateBeforeMethod;
+import org.junit.platform.commons.util.pkg1.subpkg.SubclassWithNonStaticPackagePrivateBeforeMethod;
 
 /**
  * Unit tests for {@link ReflectionUtils}.
@@ -1344,6 +1346,28 @@ void findMethodsIgnoresBridgeMethods() throws Exception {
 		assertEquals(0, methods.stream().filter(Method::isBridge).count());
 	}
 
+	/**
+	 * @see https://github.com/junit-team/junit5/issues/3498
+	 */
+	@Test
+	void findMethodsAppliesPredicateBeforeSearchingTypeHierarchy() throws Exception {
+		final String BEFORE = "before";
+		Class<?> superclass = SuperclassWithStaticPackagePrivateBeforeMethod.class;
+		Method staticMethod = superclass.getDeclaredMethod(BEFORE);
+		Class<?> subclass = SubclassWithNonStaticPackagePrivateBeforeMethod.class;
+		Method nonStaticMethod = subclass.getDeclaredMethod(BEFORE);
+
+		// Prerequisite
+		var methods = findMethods(superclass, ReflectionUtils::isStatic);
+		assertThat(methods).containsExactly(staticMethod);
+
+		// Actual use cases for this test
+		methods = findMethods(subclass, ReflectionUtils::isStatic);
+		assertThat(methods).containsExactly(staticMethod);
+		methods = findMethods(subclass, ReflectionUtils::isNotStatic);
+		assertThat(methods).containsExactly(nonStaticMethod);
+	}
+
 	@Test
 	void isGeneric() {
 		for (var method : Generic.class.getMethods()) {

platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/SuperclassWithStaticPackagePrivateBeforeMethod.java

@@ -0,0 +1,25 @@
+/*
+ * Copyright 2015-2023 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.platform.commons.util.pkg1;
+
+import org.junit.jupiter.api.BeforeAll;
+
+/**
+ * @see https://github.com/junit-team/junit5/issues/3498
+ */
+public class SuperclassWithStaticPackagePrivateBeforeMethod {
+
+	@BeforeAll
+	static void before() {
+		// no-op
+	}
+
+}

platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/subpkg/SubclassWithNonStaticPackagePrivateBeforeMethod.java

@@ -0,0 +1,26 @@
+/*
+ * Copyright 2015-2023 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.platform.commons.util.pkg1.subpkg;
+
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.platform.commons.util.pkg1.SuperclassWithStaticPackagePrivateBeforeMethod;
+
+/**
+ * @see https://github.com/junit-team/junit5/issues/3498
+ */
+public class SubclassWithNonStaticPackagePrivateBeforeMethod extends SuperclassWithStaticPackagePrivateBeforeMethod {
+
+	@BeforeEach
+	void before() {
+		// no-op
+	}
+
+}