Report unannotated JUnit Test annotation

testing/trino-testing-services/pom.xml

@@ -53,6 +53,13 @@
             <optional>true</optional>
         </dependency>
 
+        <dependency>
+            <groupId>org.junit.jupiter</groupId>
+            <artifactId>junit-jupiter-api</artifactId>
+            <!-- it's either on classpath because anyway, or not needed at all. Let's not force the dependency on downstream when not needed. -->
+            <optional>true</optional>
+        </dependency>
+
         <dependency>
             <groupId>org.openjdk.jmh</groupId>
             <artifactId>jmh-core</artifactId>

testing/trino-testing-services/src/main/java/io/trino/junit/ReportBadJunitTestAnnotations.java

@@ -0,0 +1,127 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.trino.junit;
+
+import com.google.common.annotations.VisibleForTesting;
+import io.trino.testng.services.ReportBadTestAnnotations;
+import org.junit.jupiter.api.extension.ExtensionContext;
+import org.junit.jupiter.api.extension.TestInstanceFactoryContext;
+import org.junit.jupiter.api.extension.TestInstancePreConstructCallback;
+
+import java.lang.annotation.Annotation;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Optional;
+
+import static com.google.common.base.Throwables.getStackTraceAsString;
+import static com.google.common.collect.ImmutableList.toImmutableList;
+import static io.trino.testing.Listeners.reportListenerFailure;
+import static java.util.stream.Collectors.joining;
+
+public class ReportBadJunitTestAnnotations
+        implements TestInstancePreConstructCallback
+{
+    @Override
+    public void preConstructTestInstance(TestInstanceFactoryContext factoryContext, ExtensionContext context)
+    {
+        Class<?> testClass = factoryContext.getTestClass();
+        try {
+            reportBadTestAnnotations(testClass);
+        }
+        catch (RuntimeException | Error e) {
+            reportListenerFailure(
+                    ReportBadTestAnnotations.class,
+                    "Failed to process %s: \n%s",
+                    testClass,
+                    getStackTraceAsString(e));
+        }
+    }
+
+    private void reportBadTestAnnotations(Class<?> testClass)
+    {
+        List<Method> unannotatedTestMethods = findUnannotatedInheritedTestMethods(testClass);
+        if (!unannotatedTestMethods.isEmpty()) {
+            reportListenerFailure(
+                    ReportBadJunitTestAnnotations.class,
+                    "Test class %s has methods which are inherited but not explicitly annotated. Are they missing @Test?%s",
+                    testClass.getName(),
+                    unannotatedTestMethods.stream()
+                            .map(Method::toString)
+                            .collect(joining("\n\t\t", "\n\t\t", "")));
+        }
+    }
+
+    @VisibleForTesting
+    static List<Method> findUnannotatedInheritedTestMethods(Class<?> realClass)
+    {
+        return Arrays.stream(realClass.getMethods())
+                .filter(method -> method.getDeclaringClass() != Object.class)
+                .filter(method -> !Modifier.isStatic(method.getModifiers()))
+                .filter(method -> !method.isBridge())
+                .filter(method -> isUnannotated(method) && overriddenMethodHasTestAnnotation(method))
+                .collect(toImmutableList());
+    }
+
+    private static boolean isUnannotated(Method method)
+    {
+        return Arrays.stream(method.getAnnotations()).map(Annotation::annotationType)
+                .noneMatch(ReportBadJunitTestAnnotations::isJUnitAnnotation);
+    }
+
+    private static boolean isJUnitAnnotation(Class<? extends Annotation> clazz)
+    {
+        return clazz.getPackage().getName().startsWith("org.junit.jupiter.api");
+    }
+
+    private static boolean overriddenMethodHasTestAnnotation(Method method)
+    {
+        if (method.isAnnotationPresent(org.junit.jupiter.api.Test.class)) {
+            return true;
+        }
+
+        // Skip methods in Object class, e.g. toString()
+        if (method.getDeclaringClass() == Object.class) {
+            return false;
+        }
+
+        // The test class may override the default method of the interface
+        for (Class<?> interfaceClass : method.getDeclaringClass().getInterfaces()) {
+            Optional<Method> overridden = getOverridden(method, interfaceClass);
+            if (overridden.isPresent() && overridden.get().isAnnotationPresent(org.junit.jupiter.api.Test.class)) {
+                return true;
+            }
+        }
+
+        Class<?> superClass = method.getDeclaringClass().getSuperclass();
+        if (superClass == null) {
+            return false;
+        }
+        return getOverridden(method, superClass)
+                .map(ReportBadJunitTestAnnotations::overriddenMethodHasTestAnnotation)
+                .orElse(false);
+    }
+
+    private static Optional<Method> getOverridden(Method method, Class<?> base)
+    {
+        try {
+            // Simplistic override detection
+            return Optional.of(base.getMethod(method.getName(), method.getParameterTypes()));
+        }
+        catch (NoSuchMethodException ignored) {
+            return Optional.empty();
+        }
+    }
+}

testing/trino-testing-services/src/main/java/io/trino/testng/services/Listeners.java → testing/trino-testing-services/src/main/java/io/trino/testing/Listeners.java

@@ -11,27 +11,26 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package io.trino.testng.services;
+package io.trino.testing;
 
 import com.google.common.base.Joiner;
 import com.google.errorprone.annotations.FormatMethod;
 import org.testng.ITestClass;
-import org.testng.ITestNGListener;
 import org.testng.ITestResult;
 
 import static java.lang.String.format;
 
-final class Listeners
+public final class Listeners
 {
     private Listeners() {}
 
     /**
      * Print error to standard error and exit JVM.
      *
-     * @apiNote A TestNG listener cannot throw an exception, as this are not currently properly handled by TestNG.
+     * @apiNote A TestNG listener and JUnit extension cannot throw an exception, as this are not currently properly handled by them.
      */
     @FormatMethod
-    public static void reportListenerFailure(Class<? extends ITestNGListener> listenerClass, String format, Object... args)
+    public static void reportListenerFailure(Class<?> listenerClass, String format, Object... args)
     {
         System.err.println(format("FATAL: %s: ", listenerClass.getName()) + format(format, args));
         System.err.println("JVM will be terminated");

testing/trino-testing-services/src/main/java/io/trino/testng/services/FlakyAnnotationVerifier.java

@@ -28,7 +28,7 @@
 
 import static com.google.common.base.Throwables.getStackTraceAsString;
 import static com.google.common.collect.ImmutableList.toImmutableList;
-import static io.trino.testng.services.Listeners.reportListenerFailure;
+import static io.trino.testing.Listeners.reportListenerFailure;
 import static java.lang.String.format;
 import static java.util.stream.Collectors.joining;
 

testing/trino-testing-services/src/main/java/io/trino/testng/services/LogTestDurationListener.java

@@ -39,8 +39,8 @@
 import static com.google.common.base.Throwables.getStackTraceAsString;
 import static io.airlift.concurrent.Threads.daemonThreadsNamed;
 import static io.airlift.units.Duration.nanosSince;
-import static io.trino.testng.services.Listeners.formatTestName;
-import static io.trino.testng.services.Listeners.reportListenerFailure;
+import static io.trino.testing.Listeners.formatTestName;
+import static io.trino.testing.Listeners.reportListenerFailure;
 import static java.lang.String.format;
 import static java.lang.management.ManagementFactory.getThreadMXBean;
 import static java.util.concurrent.TimeUnit.MINUTES;

testing/trino-testing-services/src/main/java/io/trino/testng/services/ManageTestResources.java

@@ -38,7 +38,7 @@
 import static com.google.common.base.Throwables.getStackTraceAsString;
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static com.google.common.collect.MoreCollectors.toOptional;
-import static io.trino.testng.services.Listeners.reportListenerFailure;
+import static io.trino.testing.Listeners.reportListenerFailure;
 import static io.trino.testng.services.ManageTestResources.Stage.AFTER_CLASS;
 import static io.trino.testng.services.ManageTestResources.Stage.BEFORE_CLASS;
 import static java.lang.annotation.ElementType.FIELD;

testing/trino-testing-services/src/main/java/io/trino/testng/services/ProgressLoggingListener.java

@@ -25,7 +25,7 @@
 import java.math.BigDecimal;
 import java.math.RoundingMode;
 
-import static io.trino.testng.services.Listeners.formatTestName;
+import static io.trino.testing.Listeners.formatTestName;
 import static java.lang.String.format;
 
 public class ProgressLoggingListener

testing/trino-testing-services/src/main/java/io/trino/testng/services/ReportAfterMethodNotAlwaysRun.java

@@ -32,7 +32,7 @@
 
 import static com.google.common.base.Throwables.getStackTraceAsString;
 import static com.google.common.collect.ImmutableList.toImmutableList;
-import static io.trino.testng.services.Listeners.reportListenerFailure;
+import static io.trino.testing.Listeners.reportListenerFailure;
 import static java.util.Objects.requireNonNull;
 import static java.util.function.Predicate.not;
 import static java.util.stream.Collectors.joining;

testing/trino-testing-services/src/main/java/io/trino/testng/services/ReportBadTestAnnotations.java

@@ -28,7 +28,7 @@
 
 import static com.google.common.base.Throwables.getStackTraceAsString;
 import static com.google.common.collect.ImmutableList.toImmutableList;
-import static io.trino.testng.services.Listeners.reportListenerFailure;
+import static io.trino.testing.Listeners.reportListenerFailure;
 import static java.lang.annotation.ElementType.METHOD;
 import static java.lang.annotation.ElementType.TYPE;
 import static java.lang.annotation.RetentionPolicy.RUNTIME;

testing/trino-testing-services/src/main/java/io/trino/testng/services/ReportIllNamedTest.java

@@ -17,7 +17,7 @@
 import org.testng.ITestClass;
 
 import static com.google.common.base.Throwables.getStackTraceAsString;
-import static io.trino.testng.services.Listeners.reportListenerFailure;
+import static io.trino.testing.Listeners.reportListenerFailure;
 import static io.trino.testng.services.ReportBadTestAnnotations.isTemptoClass;
 
 public class ReportIllNamedTest

testing/trino-testing-services/src/main/java/io/trino/testng/services/ReportInnerTestClasses.java

@@ -19,7 +19,7 @@
 import java.util.Optional;
 
 import static com.google.common.base.Throwables.getStackTraceAsString;
-import static io.trino.testng.services.Listeners.reportListenerFailure;
+import static io.trino.testing.Listeners.reportListenerFailure;
 
 /**
  * Detects test classes which are defined as inner classes

testing/trino-testing-services/src/main/java/io/trino/testng/services/ReportMultiThreadedBeforeOrAfterMethod.java

@@ -25,7 +25,7 @@
 import java.lang.reflect.Method;
 
 import static com.google.common.base.Throwables.getStackTraceAsString;
-import static io.trino.testng.services.Listeners.reportListenerFailure;
+import static io.trino.testing.Listeners.reportListenerFailure;
 import static java.lang.String.format;
 
 public class ReportMultiThreadedBeforeOrAfterMethod

testing/trino-testing-services/src/main/java/io/trino/testng/services/ReportOrphanedExecutors.java

@@ -26,7 +26,7 @@
 
 import static com.google.common.base.Throwables.getStackTraceAsString;
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
-import static io.trino.testng.services.Listeners.reportListenerFailure;
+import static io.trino.testing.Listeners.reportListenerFailure;
 import static java.lang.String.format;
 import static java.lang.annotation.ElementType.FIELD;
 import static java.lang.annotation.RetentionPolicy.RUNTIME;

testing/trino-testing-services/src/main/java/io/trino/testng/services/ReportPrivateMethods.java

@@ -28,7 +28,7 @@
 
 import static com.google.common.base.Throwables.getStackTraceAsString;
 import static com.google.common.collect.ImmutableList.toImmutableList;
-import static io.trino.testng.services.Listeners.reportListenerFailure;
+import static io.trino.testing.Listeners.reportListenerFailure;
 import static java.lang.annotation.ElementType.METHOD;
 import static java.lang.annotation.RetentionPolicy.RUNTIME;
 import static java.util.stream.Collectors.joining;

testing/trino-testing-services/src/main/resources/META-INF/services/org.junit.jupiter.api.extension.Extension

@@ -0,0 +1 @@
+io.trino.junit.ReportBadJunitTestAnnotations

testing/trino-testing-services/src/main/resources/junit-platform.properties

@@ -0,0 +1 @@
+junit.jupiter.extensions.autodetection.enabled=true

testing/trino-testing-services/src/test/java/io/trino/junit/TestReportBadJunitTestAnnotations.java

@@ -0,0 +1,113 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.trino.junit;
+
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
+
+import java.lang.reflect.Method;
+
+import static io.trino.junit.ReportBadJunitTestAnnotations.findUnannotatedInheritedTestMethods;
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class TestReportBadJunitTestAnnotations
+{
+    @Test
+    public void testTest()
+    {
+        assertThat(findUnannotatedInheritedTestMethods(TestingTest.class))
+                .isEmpty();
+        assertThat(findUnannotatedInheritedTestMethods(TestingTestWithProxy.class))
+                .isEmpty();
+        assertThat(findUnannotatedInheritedTestMethods(TestingBeforeAfterAnnotations.class))
+                .isEmpty();
+        assertThat(findUnannotatedInheritedTestMethods(TestingTestWithoutTestAnnotation.class))
+                .isEmpty();
+    }
+
+    @Test
+    public void testTestWithoutTestAnnotation()
+    {
+        assertThat(findUnannotatedInheritedTestMethods(TestingTestWithoutAnnotation.class))
+                .extracting(Method::getName)
+                .containsExactly("testInInterface");
+    }
+
+    private static class TestingTest
+            implements TestingInterfaceWithTest
+    {
+        @Test
+        public void test() {}
+    }
+
+    private static class TestingTestWithoutAnnotation
+            implements TestingInterfaceWithTest
+    {
+        @Override
+        public void testInInterface()
+        {
+            TestingInterfaceWithTest.super.testInInterface();
+        }
+    }
+
+    private static class TestingTestWithProxy
+            extends TestingInterfaceWithTestProxy
+    {
+        @Test
+        public void test() {}
+    }
+
+    private static class TestingTestWithoutTestAnnotation
+            implements TestingInterface
+    {
+        public void testWithMissingTestAnnotation() {}
+
+        @Override
+        public String toString()
+        {
+            return "test override";
+        }
+    }
+
+    private static class TestingInterfaceWithTestProxy
+            implements TestingInterfaceWithTest {}
+
+    private interface TestingInterfaceWithTest
+    {
+        @Test
+        default void testInInterface() {}
+    }
+
+    private interface TestingInterface
+    {
+        default void methodInInterface() {}
+    }
+
+    private static class TestingBeforeAfterAnnotations
+            extends BaseTest {}
+
+    private static class BaseTest
+    {
+        @BeforeAll
+        @BeforeClass
+        public final void initialize() {}
+
+        @AfterAll
+        @AfterClass(alwaysRun = true)
+        public final void destroy() {}
+    }
+}