Report unannotated JUnit Test annotation #18691
Conversation
| @@ -28,10 +27,10 @@ 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. | |||
There was a problem hiding this comment.
This class is in testng.services package. let's move to io.trino.testing
|
|
||
| private static boolean overriddenMethodHasTestAnnotation(Method method) | ||
| { | ||
| if (method.isAnnotationPresent(Test.class)) { |
There was a problem hiding this comment.
fully qualified name would be better here. there are more than 1 @Test candidates
| if (method.getDeclaringClass().isInterface()) { | ||
| return false; |
| return false; | ||
| } | ||
|
|
||
| for (Class<?> interfaceClass : method.getDeclaringClass().getInterfaces()) { |
There was a problem hiding this comment.
Why look at interfaces? please explain
| @@ -0,0 +1 @@ | |||
| io.trino.testng.services.ReportBadJunitTestAnnotations | |||
There was a problem hiding this comment.
not under testng -- io.trino.testing.junit or just io.trino.junit
| private static boolean isUnannotated(Method method) | ||
| { | ||
| return Arrays.stream(method.getAnnotations()).map(Annotation::annotationType) | ||
| .anyMatch(ReportBadJunitTestAnnotations::isJUnitAnnotation); |
There was a problem hiding this comment.
isUnannotated = ... anyMatch(isJUnitAnnotation)
was noneMatch meant? or was the method be named isAnnotated?
| { | ||
| return Arrays.stream(realClass.getMethods()) | ||
| .filter(method -> method.getDeclaringClass() != Object.class) | ||
| .filter(method -> !Modifier.isStatic(method.getModifiers())) |
There was a problem hiding this comment.
Static methods are not overridden, so this should be handled by the overriddenMethodHasTestAnnotation logic below.
There was a problem hiding this comment.
Disagreed. There's no benefit to delay the filter.
| @@ -0,0 +1 @@ | |||
| junit.jupiter.extensions.autodetection.enabled=true | |||
There was a problem hiding this comment.
This should go in the surefire configuration: https://junit.org/junit5/docs/current/user-guide/#running-tests-build-maven-config-params
| @@ -0,0 +1 @@ | |||
| io.trino.junit.ReportBadJunitTestAnnotations | |||
There was a problem hiding this comment.
This is very brittle. It only works if a module depends on trino-testing-services. To ensure it applies to everything, the extension should be registered via the surefire plugin configuration. It may require moving this check out of the project, and building an artifact that the Trino build can depend on.
There was a problem hiding this comment.
I know, but it's much better than no reporter.
There was a problem hiding this comment.
@martint would you like to provide this safety? Without this we are risking that some tests are not executed and IMHO this is a blocker for further migration to JUnit.
|
Closing as I have no bandwidth for converting to maven plugin. |
|
It doesn't need to be a maven plugin -- just a library that can be included in the plugin configuration for the whole project. |
Description
Fixes #14027
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.