Skip to content

Commit

Permalink
Align method and field search algorithms with Java semantics
Browse files Browse the repository at this point in the history 
This commit modifies the method and field search algorithms in
ReflectionUtils in order to align with standard Java semantics
regarding whether a given field or method is visible or overridden
according to the rules of the Java language.

Specifically, this commit:

- Renames isMethodShadowedBy() to isMethodOverriddenBy().

- Revises the logic in isMethodOverriddenBy() to determine if one
  method overrides another method according to the rules of the Java
  language.

- Revises isFieldShadowedByLocalFields() to always return false for the
  time being.

- Revises existing tests accordingly; introduces new tests for the new
  behavior; and disables existing tests for the legacy behavior.

See #3553
  • Loading branch information
@sbrannen
sbrannen committed Apr 7, 2024
1 parent e160aec commit 65501fc
Show file tree
Hide file tree
Showing 8 changed files with 337 additions and 54 deletions.
12 changes: 8 additions & 4 deletions ...eMethodOverridingAndSupersedingTests.java → ...ngine/LifecycleMethodOverridingTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

package org.junit.jupiter.engine;

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.fail;

import org.junit.jupiter.api.BeforeEach;
Expand All @@ -19,12 +20,12 @@
import org.junit.jupiter.engine.subpackage.SuperClassWithPackagePrivateLifecycleMethodInDifferentPackageTestCase;

/**
* Integration tests that explicitly demonstrate the overriding and superseding
* rules for lifecycle methods in the {@link JupiterTestEngine}.
* Integration tests that explicitly demonstrate the overriding rules for
* lifecycle methods in the {@link JupiterTestEngine}.
*
* @since 5.9
*/
class LifecycleMethodOverridingAndSupersedingTests {
class LifecycleMethodOverridingTests {

@Nested
@DisplayName("A package-private lifecycle method can be overridden by")
Expand Down Expand Up @@ -67,7 +68,7 @@ public void beforeEach() {
}

@Nested
@DisplayName("A package-private lifecycle method from a different package can be superseded by")
@DisplayName("A package-private lifecycle method from a different package cannot be overridden by")
class PackagePrivateSuperClassInDifferentPackageTests {

@Nested
Expand All @@ -78,6 +79,7 @@ class ProtectedExtendsPackagePrivateLifecycleMethod
// @Override
@BeforeEach
protected void beforeEach() {
assertThat(super.beforeEachInvoked).isTrue();
}

}
Expand All @@ -90,6 +92,7 @@ class PackagePrivateExtendsPackagePrivateLifecycleMethod
// @Override
@BeforeEach
void beforeEach() {
assertThat(super.beforeEachInvoked).isTrue();
}

}
Expand All @@ -102,6 +105,7 @@ class PublicExtendsPackagePrivateLifecycleMethod
// @Override
@BeforeEach
public void beforeEach() {
assertThat(super.beforeEachInvoked).isTrue();
}

}
Expand Down
40 changes: 24 additions & 16 deletions ...piter-engine/src/test/java/org/junit/jupiter/engine/extension/BeforeAndAfterAllTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,23 +103,31 @@ void beforeAllAndAfterAllCallbacksInSubSubclass() {
assertThat(actualExceptionInAfterAllCallback).isEmpty();
}

/**
* Since static methods cannot be overridden, "static hiding" no longer occurs since 5.11.
*/
@Test
void beforeAllAndAfterAllCallbacksInSubSubclassWithStaticMethodHiding() {
void beforeAllAndAfterAllCallbacksInSubSubclassWithoutStaticMethodHiding() {
// NOTE: both the @BeforeAll AND the @AfterAll methods in level 3 are
// executed as 1/2/3 due to the "stable" method sort order on the Platform.

// @formatter:off
assertBeforeAllAndAfterAllCallbacks(ThirdLevelStaticHidingTestCase.class,
"fooBeforeAllCallback",
"barBeforeAllCallback",
"bazBeforeAllCallback",
"quuxBeforeAllCallback",
"beforeAllMethod-1-hidden",
"beforeAllMethod-2-hidden",
"beforeAllMethod-3",
"test-3",
// The @AfterAll methods are executed as 1/2/3 due to
// the "stable" method sort order on the Platform.
"afterAllMethod-1-hidden",
"afterAllMethod-2-hidden",
"afterAllMethod-3",
"beforeAllMethod-1",
"beforeAllMethod-2",
"beforeAllMethod-1-level3",
"beforeAllMethod-2-level3",
"beforeAllMethod-3-level3",
"test-3",
"afterAllMethod-1-level3",
"afterAllMethod-2-level3",
"afterAllMethod-3-level3",
"afterAllMethod-2",
"afterAllMethod-1",
"quuxAfterAllCallback",
"bazAfterAllCallback",
"barAfterAllCallback",
Expand Down Expand Up @@ -244,32 +252,32 @@ static class ThirdLevelStaticHidingTestCase extends SecondLevelTestCase {

@BeforeAll
static void beforeAll1() {
callSequence.add("beforeAllMethod-1-hidden");
callSequence.add("beforeAllMethod-1-level3");
}

@BeforeAll
static void beforeAll2() {
callSequence.add("beforeAllMethod-2-hidden");
callSequence.add("beforeAllMethod-2-level3");
}

@BeforeAll
static void beforeAll3() {
callSequence.add("beforeAllMethod-3");
callSequence.add("beforeAllMethod-3-level3");
}

@AfterAll
static void afterAll1() {
callSequence.add("afterAllMethod-1-hidden");
callSequence.add("afterAllMethod-1-level3");
}

@AfterAll
static void afterAll2() {
callSequence.add("afterAllMethod-2-hidden");
callSequence.add("afterAllMethod-2-level3");
}

@AfterAll
static void afterAll3() {
callSequence.add("afterAllMethod-3");
callSequence.add("afterAllMethod-3-level3");
}

@Test
Expand Down
35 changes: 33 additions & 2 deletions ...ava/org/junit/jupiter/engine/extension/OrderedProgrammaticExtensionRegistrationTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Order;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;
Expand Down Expand Up @@ -154,7 +155,22 @@ void classLevelWithDefaultOrderAndExplicitOrderInheritedFromSuperclass() {
}

@Test
void classLevelWithDefaultOrderShadowingOrderFromSuperclass() {
void classLevelWithDefaultOrderDoesNotShadowExtensionFromSuperclass() {
Class<?> testClass = DefaultOrderShadowingDefaultOrderAndExplicitOrderClassLevelExtensionRegistrationTestCase.class;
String testClassName = testClass.getSimpleName();
Class<?> parent = DefaultOrderAndExplicitOrderClassLevelExtensionRegistrationTestCase.class;
String parentName = parent.getSimpleName();
assertOutcome(testClass, //
parentName + " :: extension3 :: before test", //
parentName + " :: extension1 :: before test", //
parentName + " :: extension2 :: before test", //
testClassName + " :: extension3 :: before test" //
);
}

@Disabled("Disabled until legacy search mode is supported")
@Test
void classLevelWithDefaultOrderShadowingOrderFromSuperclassInLegacyMode() {
Class<?> testClass = DefaultOrderShadowingDefaultOrderAndExplicitOrderClassLevelExtensionRegistrationTestCase.class;
String testClassName = testClass.getSimpleName();
Class<?> parent = DefaultOrderAndExplicitOrderClassLevelExtensionRegistrationTestCase.class;
Expand All @@ -167,7 +183,22 @@ void classLevelWithDefaultOrderShadowingOrderFromSuperclass() {
}

@Test
void classLevelWithExplicitOrderShadowingOrderFromSuperclass() {
void classLevelWithExplicitOrderDoesNotShadowExtensionFromSuperclass() {
Class<?> testClass = ExplicitOrderShadowingDefaultOrderAndExplicitOrderClassLevelExtensionRegistrationTestCase.class;
String testClassName = testClass.getSimpleName();
Class<?> parent = DefaultOrderAndExplicitOrderClassLevelExtensionRegistrationTestCase.class;
String parentName = parent.getSimpleName();
assertOutcome(testClass, //
parentName + " :: extension3 :: before test", //
testClassName + " :: extension2 :: before test", //
parentName + " :: extension1 :: before test", //
parentName + " :: extension2 :: before test" //
);
}

@Disabled("Disabled until legacy search mode is supported")
@Test
void classLevelWithExplicitOrderShadowingOrderFromSuperclassInLegacyMode() {
Class<?> testClass = ExplicitOrderShadowingDefaultOrderAndExplicitOrderClassLevelExtensionRegistrationTestCase.class;
String testClassName = testClass.getSimpleName();
Class<?> parent = DefaultOrderAndExplicitOrderClassLevelExtensionRegistrationTestCase.class;
Expand Down
55 changes: 51 additions & 4 deletions .../test/java/org/junit/jupiter/engine/extension/ProgrammaticExtensionRegistrationTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;
Expand Down Expand Up @@ -98,7 +99,7 @@ void classLevelFromInterface() {
}

@Test
void instanceLevelWithInheritedAndHiddenExtensions() {
void instanceLevelWithInheritedExtensions() {
Class<?> testClass = InstanceLevelExtensionRegistrationParentTestCase.class;
String parent = testClass.getSimpleName();
assertOneTestSucceeded(testClass);
Expand All @@ -113,13 +114,59 @@ void instanceLevelWithInheritedAndHiddenExtensions() {
assertOneTestSucceeded(testClass);
assertThat(callSequence).containsExactly( //
parent + " :: extension1 :: before test", //
parent + " :: extension2 :: before test", //
child + " :: extension2 :: before test", //
child + " :: extension3 :: before test" //
);
}

@Disabled("Disabled until legacy search mode is supported")
@Test
void classLevelWithInheritedAndHiddenExtensions() {
void instanceLevelWithInheritedAndHiddenExtensionsInLegacyMode() {
Class<?> testClass = InstanceLevelExtensionRegistrationParentTestCase.class;
String parent = testClass.getSimpleName();
assertOneTestSucceeded(testClass);
assertThat(callSequence).containsExactly( //
parent + " :: extension1 :: before test", //
parent + " :: extension2 :: before test" //
);

callSequence.clear();
testClass = InstanceLevelExtensionRegistrationChildTestCase.class;
String child = testClass.getSimpleName();
assertOneTestSucceeded(testClass);
assertThat(callSequence).containsExactly( //
parent + " :: extension1 :: before test", //
child + " :: extension2 :: before test", //
child + " :: extension3 :: before test" //
);
}

@Test
void classLevelWithInheritedExtensions() {
Class<?> testClass = ClassLevelExtensionRegistrationParentTestCase.class;
String parent = testClass.getSimpleName();
assertOneTestSucceeded(testClass);
assertThat(callSequence).containsExactly( //
parent + " :: extension1 :: before test", //
parent + " :: extension2 :: before test" //
);

callSequence.clear();
testClass = ClassLevelExtensionRegistrationChildTestCase.class;
String child = testClass.getSimpleName();
assertOneTestSucceeded(testClass);
assertThat(callSequence).containsExactly( //
parent + " :: extension1 :: before test", //
parent + " :: extension2 :: before test", //
child + " :: extension2 :: before test", //
child + " :: extension3 :: before test" //
);
}

@Disabled("Disabled until legacy search mode is supported")
@Test
void classLevelWithInheritedAndHiddenExtensionsInLegacyMode() {
Class<?> testClass = ClassLevelExtensionRegistrationParentTestCase.class;
String parent = testClass.getSimpleName();
assertOneTestSucceeded(testClass);
Expand Down Expand Up @@ -489,7 +536,7 @@ void test() {

static class ClassLevelExtensionRegistrationChildTestCase extends ClassLevelExtensionRegistrationParentTestCase {

// "Hides" ClassLevelExtensionRegistrationParentTestCase.extension2
// "Hides" ClassLevelExtensionRegistrationParentTestCase.extension2 in legacy mode
@RegisterExtension
static Extension extension2 = new BeforeEachExtension(2);

Expand All @@ -515,7 +562,7 @@ void test() {
static class InstanceLevelExtensionRegistrationChildTestCase
extends InstanceLevelExtensionRegistrationParentTestCase {

// "Hides" InstanceLevelExtensionRegistrationParentTestCase.extension2
// "Hides" InstanceLevelExtensionRegistrationParentTestCase.extension2 in legacy mode
@RegisterExtension
Extension extension2 = new BeforeEachExtension(2);

Expand Down
7 changes: 5 additions & 2 deletions ...ine/subpackage/SuperClassWithPackagePrivateLifecycleMethodInDifferentPackageTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

package org.junit.jupiter.engine.subpackage;

import static org.junit.jupiter.api.Assertions.fail;
import static org.assertj.core.api.Assertions.assertThat;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand All @@ -20,13 +20,16 @@
*/
public class SuperClassWithPackagePrivateLifecycleMethodInDifferentPackageTestCase {

protected boolean beforeEachInvoked = false;

@BeforeEach
void beforeEach() {
fail();
this.beforeEachInvoked = true;
}

@Test
void test() {
assertThat(this.beforeEachInvoked).isTrue();
}

}
43 changes: 36 additions & 7 deletions junit-platform-commons/src/main/java/org/junit/platform/commons/util/ReflectionUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1524,10 +1524,10 @@ private static List<Method> findAllMethodsInHierarchy(Class<?> clazz, HierarchyT
.filter(method -> !method.isSynthetic())
.collect(toList());
List<Method> superclassMethods = getSuperclassMethods(clazz, traversalMode).stream()
.filter(method -> !isMethodShadowedByLocalMethods(method, localMethods))
.filter(method -> !isMethodOverriddenByLocalMethods(method, localMethods))
.collect(toList());
List<Method> interfaceMethods = getInterfaceMethods(clazz, traversalMode).stream()
.filter(method -> !isMethodShadowedByLocalMethods(method, localMethods))
.filter(method -> !isMethodOverriddenByLocalMethods(method, localMethods))
.collect(toList());
// @formatter:on

Expand Down Expand Up @@ -1672,7 +1672,7 @@ private static List<Method> getInterfaceMethods(Class<?> clazz, HierarchyTravers
.collect(toList());

List<Method> superinterfaceMethods = getInterfaceMethods(ifc, traversalMode).stream()
.filter(method -> !isMethodShadowedByLocalMethods(method, localInterfaceMethods))
.filter(method -> !isMethodOverriddenByLocalMethods(method, localInterfaceMethods))
.collect(toList());
// @formatter:on

Expand Down Expand Up @@ -1718,7 +1718,9 @@ private static List<Field> getSuperclassFields(Class<?> clazz, HierarchyTraversa
}

private static boolean isFieldShadowedByLocalFields(Field field, List<Field> localFields) {
return localFields.stream().anyMatch(local -> local.getName().equals(field.getName()));
// TODO Enable if legacy field search semantics are enabled.
// return localFields.stream().anyMatch(local -> local.getName().equals(field.getName()));
return false;
}

private static List<Method> getSuperclassMethods(Class<?> clazz, HierarchyTraversalMode traversalMode) {
Expand All @@ -1729,14 +1731,41 @@ private static List<Method> getSuperclassMethods(Class<?> clazz, HierarchyTraver
return findAllMethodsInHierarchy(superclass, traversalMode);
}

private static boolean isMethodShadowedByLocalMethods(Method method, List<Method> localMethods) {
return localMethods.stream().anyMatch(local -> isMethodShadowedBy(method, local));
private static boolean isMethodOverriddenByLocalMethods(Method method, List<Method> localMethods) {
return localMethods.stream().anyMatch(local -> isMethodOverriddenBy(method, local));
}

private static boolean isMethodShadowedBy(Method upper, Method lower) {
private static boolean isMethodOverriddenBy(Method upper, Method lower) {
// TODO Skip to hasCompatibleSignature() if legacy method search semantics are enabled.

// A static method cannot override anything.
if (Modifier.isStatic(lower.getModifiers())) {
return false;
}

// Cannot override a private, static, or final method.
int modifiers = upper.getModifiers();
if (Modifier.isPrivate(modifiers) || Modifier.isStatic(modifiers) || Modifier.isFinal(modifiers)) {
return false;
}

// Cannot override a package-private method in another package.
if (isPackagePrivate(upper) && !declaredInSamePackage(upper, lower)) {
return false;
}

return hasCompatibleSignature(upper, lower.getName(), lower.getParameterTypes());
}

private static boolean isPackagePrivate(Member member) {
int modifiers = member.getModifiers();
return !(Modifier.isPublic(modifiers) || Modifier.isProtected(modifiers) || Modifier.isPrivate(modifiers));
}

private static boolean declaredInSamePackage(Method m1, Method m2) {
return m1.getDeclaringClass().getPackage().getName().equals(m2.getDeclaringClass().getPackage().getName());
}

/**
* Determine if the supplied candidate method (typically a method higher in
* the type hierarchy) has a signature that is compatible with a method that
Expand Down
Loading

0 comments on commit 65501fc

Please sign in to comment.