Issues/2914 private lifecycle methods should cause exception

junit-jupiter-api/src/main/java/org/junit/jupiter/api/AfterAll.java

@@ -43,8 +43,10 @@
  * <h2>Inheritance and Execution Order</h2>
  *
  * <p>{@code @AfterAll} methods are inherited from superclasses as long as
- * they are not <em>hidden</em> or <em>overridden</em>. Furthermore,
- * {@code @AfterAll} methods from superclasses will be executed after
+ * they are not <em>hidden</em> (default mode with {@code static} modifier), or
+ * <em>overridden</em>, or <em>superseded</em> (i.e., replaced based on
+ * signature only, irrespective of Java's visibility rules). Furthermore,
+ * {@code @AfterAll} methods from superclasses will be executed before
  * {@code @AfterAll} methods in subclasses.
  *
  * <p>Similarly, {@code @AfterAll} methods declared in an interface are

junit-jupiter-api/src/main/java/org/junit/jupiter/api/AfterEach.java

@@ -36,8 +36,10 @@
  * <h2>Inheritance and Execution Order</h2>
  *
  * <p>{@code @AfterEach} methods are inherited from superclasses as long as
- * they are not overridden. Furthermore, {@code @AfterEach} methods from
- * superclasses will be executed after {@code @AfterEach} methods in subclasses.
+ * they are not overridden or <em>superseded</em> (i.e., replaced based on
+ * signature only, irrespective of Java's visibility rules). Furthermore,
+ * {@code @AfterEach} methods from superclasses will be executed after
+ * {@code @AfterEach} methods in subclasses.
  *
  * <p>Similarly, {@code @AfterEach} methods declared as <em>interface default
  * methods</em> are inherited as long as they are not overridden, and

junit-jupiter-api/src/main/java/org/junit/jupiter/api/BeforeAll.java

@@ -43,7 +43,9 @@
  * <h2>Inheritance and Execution Order</h2>
  *
  * <p>{@code @BeforeAll} methods are inherited from superclasses as long as
- * they are not <em>hidden</em> or <em>overridden</em>. Furthermore,
+ * they are not <em>hidden</em> (default mode with {@code static} modifier), or
+ * <em>overridden</em>, or <em>superseded</em> (i.e., replaced based on
+ * signature only, irrespective of Java's visibility rules). Furthermore,
  * {@code @BeforeAll} methods from superclasses will be executed before
  * {@code @BeforeAll} methods in subclasses.
  *

junit-jupiter-api/src/main/java/org/junit/jupiter/api/BeforeEach.java

@@ -36,8 +36,10 @@
  * <h2>Inheritance and Execution Order</h2>
  *
  * <p>{@code @BeforeEach} methods are inherited from superclasses as long as
- * they are not overridden. Furthermore, {@code @BeforeEach} methods from
- * superclasses will be executed before {@code @BeforeEach} methods in subclasses.
+ * they are not overridden or <em>superseded</em> (i.e., replaced based on
+ * signature only, irrespective of Java's visibility rules). Furthermore,
+ * {@code @BeforeEach} methods from superclasses will be executed before
+ * {@code @BeforeEach} methods in subclasses.
  *
  * <p>Similarly, {@code @BeforeEach} methods declared as <em>interface default
  * methods</em> are inherited as long as they are not overridden, and

junit-jupiter-engine/src/main/java/org/junit/jupiter/engine/descriptor/LifecycleMethodUtils.java

@@ -37,19 +37,21 @@ private LifecycleMethodUtils() {
 	}
 
 	static List<Method> findBeforeAllMethods(Class<?> testClass, boolean requireStatic) {
-		return findMethodsAndAssertStatic(testClass, requireStatic, BeforeAll.class, HierarchyTraversalMode.TOP_DOWN);
+		return findMethodsAndAssertStaticAndNonPrivate(testClass, requireStatic, BeforeAll.class,
+			HierarchyTraversalMode.TOP_DOWN);
 	}
 
 	static List<Method> findAfterAllMethods(Class<?> testClass, boolean requireStatic) {
-		return findMethodsAndAssertStatic(testClass, requireStatic, AfterAll.class, HierarchyTraversalMode.BOTTOM_UP);
+		return findMethodsAndAssertStaticAndNonPrivate(testClass, requireStatic, AfterAll.class,
+			HierarchyTraversalMode.BOTTOM_UP);
 	}
 
 	static List<Method> findBeforeEachMethods(Class<?> testClass) {
-		return findMethodsAndAssertNonStatic(testClass, BeforeEach.class, HierarchyTraversalMode.TOP_DOWN);
+		return findMethodsAndAssertNonStaticAndNonPrivate(testClass, BeforeEach.class, HierarchyTraversalMode.TOP_DOWN);
 	}
 
 	static List<Method> findAfterEachMethods(Class<?> testClass) {
-		return findMethodsAndAssertNonStatic(testClass, AfterEach.class, HierarchyTraversalMode.BOTTOM_UP);
+		return findMethodsAndAssertNonStaticAndNonPrivate(testClass, AfterEach.class, HierarchyTraversalMode.BOTTOM_UP);
 	}
 
 	private static void assertStatic(Class<? extends Annotation> annotationType, Method method) {
@@ -67,26 +69,39 @@ private static void assertNonStatic(Class<? extends Annotation> annotationType,
 		}
 	}
 
+	private static void assertNonPrivate(Class<? extends Annotation> annotationType, Method method) {
+		if (ReflectionUtils.isPrivate(method)) {
+			throw new JUnitException(String.format("@%s method '%s' must not be private.",
+				annotationType.getSimpleName(), method.toGenericString()));
+		}
+	}
+
 	private static void assertVoid(Class<? extends Annotation> annotationType, Method method) {
 		if (!returnsVoid(method)) {
 			throw new JUnitException(String.format("@%s method '%s' must not return a value.",
 				annotationType.getSimpleName(), method.toGenericString()));
 		}
 	}
 
-	private static List<Method> findMethodsAndAssertStatic(Class<?> testClass, boolean requireStatic,
+	private static List<Method> findMethodsAndAssertStaticAndNonPrivate(Class<?> testClass, boolean requireStatic,
 			Class<? extends Annotation> annotationType, HierarchyTraversalMode traversalMode) {
 		List<Method> methods = findMethodsAndCheckVoidReturnType(testClass, annotationType, traversalMode);
 		if (requireStatic) {
 			methods.forEach(method -> assertStatic(annotationType, method));
 		}
+		methods.forEach(method -> assertNonPrivate(annotationType, method));
+
 		return methods;
 	}
 
-	private static List<Method> findMethodsAndAssertNonStatic(Class<?> testClass,
+	private static List<Method> findMethodsAndAssertNonStaticAndNonPrivate(Class<?> testClass,
 			Class<? extends Annotation> annotationType, HierarchyTraversalMode traversalMode) {
 		List<Method> methods = findMethodsAndCheckVoidReturnType(testClass, annotationType, traversalMode);
-		methods.forEach(method -> assertNonStatic(annotationType, method));
+		methods.forEach(method -> {
+			assertNonStatic(annotationType, method);
+			assertNonPrivate(annotationType, method);
+		});
+
 		return methods;
 	}
 

junit-jupiter-engine/src/test/java/org/junit/jupiter/engine/InvalidLifecycleMethodConfigurationTests.java

@@ -23,7 +23,7 @@
 import org.junit.platform.testkit.engine.Events;
 
 /**
- * Integration tests that very proper handling of invalid configuration for
+ * Integration tests that verify proper handling of invalid configuration for
  * lifecycle methods in conjunction with the {@link JupiterTestEngine}.
  *
  * <p>In general, configuration errors should not be thrown until the
@@ -34,23 +34,43 @@
 class InvalidLifecycleMethodConfigurationTests extends AbstractJupiterTestEngineTests {
 
 	@Test
-	void executeValidTestCaseAlongsideTestCaseWithInvalidBeforeAllDeclaration() {
-		assertExecutionResults(TestCaseWithInvalidBeforeAllMethod.class);
+	void executeValidTestCaseAlongsideTestCaseWithInvalidNonStaticBeforeAllDeclaration() {
+		assertExecutionResults(TestCaseWithInvalidNonStaticBeforeAllMethod.class);
 	}
 
 	@Test
-	void executeValidTestCaseAlongsideTestCaseWithInvalidAfterAllDeclaration() {
-		assertExecutionResults(TestCaseWithInvalidAfterAllMethod.class);
+	void executeValidTestCaseAlongsideTestCaseWithInvalidPrivateBeforeAllDeclaration() {
+		assertExecutionResults(TestCaseWithInvalidPrivateBeforeAllMethod.class);
 	}
 
 	@Test
-	void executeValidTestCaseAlongsideTestCaseWithInvalidBeforeEachDeclaration() {
-		assertExecutionResults(TestCaseWithInvalidBeforeEachMethod.class);
+	void executeValidTestCaseAlongsideTestCaseWithInvalidNonStaticAfterAllDeclaration() {
+		assertExecutionResults(TestCaseWithInvalidNonStaticAfterAllMethod.class);
 	}
 
 	@Test
-	void executeValidTestCaseAlongsideTestCaseWithInvalidAfterEachDeclaration() {
-		assertExecutionResults(TestCaseWithInvalidAfterEachMethod.class);
+	void executeValidTestCaseAlongsideTestCaseWithInvalidPrivateAfterAllDeclaration() {
+		assertExecutionResults(TestCaseWithInvalidPrivateAfterAllMethod.class);
+	}
+
+	@Test
+	void executeValidTestCaseAlongsideTestCaseWithInvalidStaticBeforeEachDeclaration() {
+		assertExecutionResults(TestCaseWithInvalidStaticBeforeEachMethod.class);
+	}
+
+	@Test
+	void executeValidTestCaseAlongsideTestCaseWithInvalidPrivateBeforeEachDeclaration() {
+		assertExecutionResults(TestCaseWithInvalidPrivateBeforeEachMethod.class);
+	}
+
+	@Test
+	void executeValidTestCaseAlongsideTestCaseWithInvalidStaticAfterEachDeclaration() {
+		assertExecutionResults(TestCaseWithInvalidStaticAfterEachMethod.class);
+	}
+
+	@Test
+	void executeValidTestCaseAlongsideTestCaseWithInvalidPrivateAfterEachDeclaration() {
+		assertExecutionResults(TestCaseWithInvalidPrivateAfterEachMethod.class);
 	}
 
 	private void assertExecutionResults(Class<?> invalidTestClass) {
@@ -80,7 +100,7 @@ void test() {
 		}
 	}
 
-	static class TestCaseWithInvalidBeforeAllMethod {
+	static class TestCaseWithInvalidNonStaticBeforeAllMethod {
 
 		// must be static
 		@BeforeAll
@@ -92,7 +112,19 @@ void test() {
 		}
 	}
 
-	static class TestCaseWithInvalidAfterAllMethod {
+	static class TestCaseWithInvalidPrivateBeforeAllMethod {
+
+		// must not be private
+		@BeforeAll
+		private static void beforeAll() {
+		}
+
+		@Test
+		void test() {
+		}
+	}
+
+	static class TestCaseWithInvalidNonStaticAfterAllMethod {
 
 		// must be static
 		@AfterAll
@@ -104,7 +136,19 @@ void test() {
 		}
 	}
 
-	static class TestCaseWithInvalidBeforeEachMethod {
+	static class TestCaseWithInvalidPrivateAfterAllMethod {
+
+		// must not be private
+		@AfterAll
+		private static void afterAll() {
+		}
+
+		@Test
+		void test() {
+		}
+	}
+
+	static class TestCaseWithInvalidStaticBeforeEachMethod {
 
 		// must NOT be static
 		@BeforeEach
@@ -116,7 +160,19 @@ void test() {
 		}
 	}
 
-	static class TestCaseWithInvalidAfterEachMethod {
+	static class TestCaseWithInvalidPrivateBeforeEachMethod {
+
+		// must NOT be private
+		@BeforeEach
+		private void beforeEach() {
+		}
+
+		@Test
+		void test() {
+		}
+	}
+
+	static class TestCaseWithInvalidStaticAfterEachMethod {
 
 		// must NOT be static
 		@AfterEach
@@ -128,4 +184,16 @@ void test() {
 		}
 	}
 
+	static class TestCaseWithInvalidPrivateAfterEachMethod {
+
+		// must NOT be private
+		@AfterEach
+		private void afterEach() {
+		}
+
+		@Test
+		void test() {
+		}
+	}
+
 }