From 4732d12860bad31e55aef9ce5146a79dce35e551 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Mon, 9 Oct 2023 15:49:51 +0200 Subject: [PATCH] Apply method predicate before searching type hierarchy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../release-notes/release-notes-5.10.1.adoc | 10 ++++++- .../commons/util/AnnotationUtilsTests.java | 27 +++++++++++++++++++ .../commons/util/ReflectionUtilsTests.java | 24 +++++++++++++++++ ...sWithStaticPackagePrivateBeforeMethod.java | 25 +++++++++++++++++ ...thNonStaticPackagePrivateBeforeMethod.java | 26 ++++++++++++++++++ 5 files changed, 111 insertions(+), 1 deletion(-) create mode 100644 platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/SuperclassWithStaticPackagePrivateBeforeMethod.java create mode 100644 platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/subpkg/SubclassWithNonStaticPackagePrivateBeforeMethod.java diff --git a/documentation/src/docs/asciidoc/release-notes/release-notes-5.10.1.adoc b/documentation/src/docs/asciidoc/release-notes/release-notes-5.10.1.adoc index cb575e6f4dc3..abd61752cc23 100644 --- a/documentation/src/docs/asciidoc/release-notes/release-notes-5.10.1.adoc +++ b/documentation/src/docs/asciidoc/release-notes/release-notes-5.10.1.adoc @@ -15,7 +15,10 @@ 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]] @@ -23,6 +26,11 @@ JUnit repository on GitHub. ==== 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. diff --git a/platform-tests/src/test/java/org/junit/platform/commons/util/AnnotationUtilsTests.java b/platform-tests/src/test/java/org/junit/platform/commons/util/AnnotationUtilsTests.java index 478491014be1..f26f5ab755d3 100644 --- a/platform-tests/src/test/java/org/junit/platform/commons/util/AnnotationUtilsTests.java +++ b/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"); diff --git a/platform-tests/src/test/java/org/junit/platform/commons/util/ReflectionUtilsTests.java b/platform-tests/src/test/java/org/junit/platform/commons/util/ReflectionUtilsTests.java index 56ed88a8ddcc..30a403fe00c6 100644 --- a/platform-tests/src/test/java/org/junit/platform/commons/util/ReflectionUtilsTests.java +++ b/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()) { diff --git a/platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/SuperclassWithStaticPackagePrivateBeforeMethod.java b/platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/SuperclassWithStaticPackagePrivateBeforeMethod.java new file mode 100644 index 000000000000..2895f2b4980d --- /dev/null +++ b/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 + } + +} diff --git a/platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/subpkg/SubclassWithNonStaticPackagePrivateBeforeMethod.java b/platform-tests/src/test/java/org/junit/platform/commons/util/pkg1/subpkg/SubclassWithNonStaticPackagePrivateBeforeMethod.java new file mode 100644 index 000000000000..c7c6d1e0ac22 --- /dev/null +++ b/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 + } + +}