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 
See junit-team#3553
  • Loading branch information
@sbrannen
sbrannen committed Apr 6, 2024
1 parent e160aec commit db04c94
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

0 comments on commit db04c94

Please sign in to comment.