Merge pull request #19094 from jcogs33/jcogs33/java/junit5-missing-nested-annotation

Java: Add new quality query to detect missing `@Nested` annotation in JUnit5 tests

Author: jcogs33

java/ql/integration-tests/java/query-suite/java-code-quality.qls.expected

@@ -5,6 +5,7 @@ ql/java/ql/src/Likely Bugs/Comparison/IncomparableEquals.ql
 ql/java/ql/src/Likely Bugs/Comparison/InconsistentEqualsHashCode.ql
 ql/java/ql/src/Likely Bugs/Comparison/MissingInstanceofInEquals.ql
 ql/java/ql/src/Likely Bugs/Comparison/RefEqBoxed.ql
+ql/java/ql/src/Likely Bugs/Frameworks/JUnit/JUnit5MissingNestedAnnotation.ql
 ql/java/ql/src/Likely Bugs/Likely Typos/ContradictoryTypeChecks.ql
 ql/java/ql/src/Likely Bugs/Likely Typos/SuspiciousDateFormat.ql
 ql/java/ql/src/Likely Bugs/Resource Leaks/CloseReader.ql

java/ql/lib/semmle/code/java/UnitTests.qll

@@ -125,6 +125,57 @@ class JUnitJupiterTestMethod extends Method {
   }
 }
 
+/**
+ * A JUnit 5 test method.
+ *
+ * A test method is defined by JUnit as "any instance method
+ * that is directly annotated or meta-annotated with `@Test`,
+ * `@RepeatedTest`, `@ParameterizedTest`, `@TestFactory`, or
+ * `@TestTemplate`."
+ *
+ * See https://junit.org/junit5/docs/current/user-guide/#writing-tests-definitions
+ */
+class JUnit5TestMethod extends Method {
+  JUnit5TestMethod() {
+    this instanceof JUnitJupiterTestMethod or
+    this.getAnAnnotation()
+        .getType()
+        .hasQualifiedName("org.junit.jupiter.api", ["RepeatedTest", "TestFactory", "TestTemplate"]) or
+    this.getAnAnnotation()
+        .getType()
+        .hasQualifiedName("org.junit.jupiter.params", "ParameterizedTest")
+  }
+}
+
+/**
+ * A JUnit 5 test class.
+ *
+ * A test class must contain at least one test method, and
+ * cannot be abstract.
+ *
+ * See https://junit.org/junit5/docs/current/user-guide/#writing-tests-definitions
+ */
+class JUnit5TestClass extends Class {
+  JUnit5TestClass() {
+    this.getAMethod() instanceof JUnit5TestMethod and
+    not this.isAbstract()
+  }
+}
+
+/**
+ * A JUnit inner test class that is non-anonymous, non-local,
+ * and non-private.
+ */
+class JUnit5InnerTestClass extends JUnit5TestClass {
+  JUnit5InnerTestClass() {
+    // `InnerClass` is a non-static nested class.
+    this instanceof InnerClass and
+    not this.isAnonymous() and
+    not this.isLocal() and
+    not this.isPrivate()
+  }
+}
+
 /**
  * A JUnit `@Ignore` annotation.
  */

java/ql/src/Likely Bugs/Frameworks/JUnit/JUnit5MissingNestedAnnotation.md

@@ -0,0 +1,45 @@
+## Overview
+
+JUnit tests are grouped in a class, and starting from JUnit 5, users can group the test classes in a larger class so they can share the local environment of the enclosing class. While this helps organize the unit tests and foster code reuse, if an inner test class is not annotated with `@Nested`, the unit tests in it will fail to execute during builds.
+
+## Recommendation
+
+If you want the tests defined in an inner class to be recognized by the build plugin and be executed, annotate the class with `@Nested`, imported from `org.junit.jupiter.api`.
+
+## Example
+
+```java
+import org.junit.jupiter.api.Nested;
+import static org.junit.Assert.assertEquals;
+
+public class IntegerOperationTest {
+  private int i;  // Shared variable among the inner classes.
+
+  @BeforeEach
+  public void initTest() { i = 0; }
+
+  @Nested
+  public class AdditionTest {  // COMPLIANT: Inner test class annotated with `@Nested`.
+    @Test
+    public void addTest1() {
+      assertEquals(1, i + 1);
+    }
+  }
+
+  public class SubtractionTest {  // NON_COMPLIANT: Inner test class missing `@Nested`.
+    @Test
+    public void addTest1() {
+      assertEquals(-1, i - 1);
+    }
+  }
+}
+```
+
+## Implementation Notes
+
+This rule is focused on missing `@Nested` annotations on non-static nested (inner) test classes. Static nested test classes should not be annotated with `@Nested`. As a result, the absence of a `@Nested` annotation on such classes is compliant. Identifying incorrect application of a `@Nested` annotation to static nested classes is out of scope for this rule.
+
+## References
+
+- JUnit 5 API Documentation: [Annotation Interface Nested](https://junit.org/junit5/docs/current/api/org.junit.jupiter.api/org/junit/jupiter/api/Nested.html).
+- JUnit 5 User Guide: [Nested Tests](https://junit.org/junit5/docs/current/user-guide/#writing-tests-nested).

java/ql/src/Likely Bugs/Frameworks/JUnit/JUnit5MissingNestedAnnotation.ql

@@ -0,0 +1,22 @@
+/**
+ * @id java/junit5-missing-nested-annotation
+ * @previous-id java/junit5-non-static-inner-class-missing-nested-annotation
+ * @name Missing `@Nested` annotation on JUnit 5 inner test class
+ * @description A JUnit 5 inner test class that is missing a `@Nested` annotation will be
+ *              excluded from execution and may indicate a mistake from the
+ *              programmer.
+ * @kind problem
+ * @precision very-high
+ * @problem.severity warning
+ * @tags quality
+ *       reliability
+ *       correctness
+ *       testability
+ *       frameworks/junit
+ */
+
+import java
+
+from JUnit5InnerTestClass testClass
+where not testClass.hasAnnotation("org.junit.jupiter.api", "Nested")
+select testClass, "This JUnit 5 inner test class lacks a '@Nested' annotation."

java/ql/src/codeql-suites/java-code-quality.qls

@@ -6,10 +6,11 @@
       - java/inconsistent-equals-and-hashcode
       - java/input-resource-leak
       - java/integer-multiplication-cast-to-long
+      - java/junit5-missing-nested-annotation
       - java/output-resource-leak
       - java/reference-equality-of-boxed-types
       - java/string-replace-all-with-non-regex
       - java/suspicious-date-format
       - java/type-variable-hides-type
       - java/unchecked-cast-in-equals
-      - java/unused-container
+      - java/unused-container

java/ql/test/query-tests/Likely Bugs/Frameworks/JUnit/AnnotationTest.java

@@ -0,0 +1,117 @@
+import java.util.Collection;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.RepeatedTest;
+import org.junit.jupiter.api.TestFactory;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.Nested;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+public class AnnotationTest {
+  @Nested
+  public class Test1 { // COMPLIANT: Inner test class has `@Nested`
+    @Test
+    public void test() {
+    }
+  }
+
+  // NON_COMPLIANT: Inner test class is missing `@Nested`
+  public class Test2_Test { // $ Alert
+    @Test
+    public void test() {
+    }
+  }
+
+  // NON_COMPLIANT: Inner test class is missing `@Nested`
+  public class Test2_RepeatedTest { // $ Alert
+    @RepeatedTest(2)
+    public void test() {
+    }
+  }
+
+  // NON_COMPLIANT: Inner test class is missing `@Nested`
+  public class Test2_ParameterizedTest { // $ Alert
+    @ParameterizedTest
+    @ValueSource(strings = { "" })
+    public void test(String s) {
+    }
+  }
+
+  // NON_COMPLIANT: Inner test class is missing `@Nested`
+  public class Test2_TestFactory { // $ Alert
+    @TestFactory
+    Collection<Object> test() {
+        return null;
+    }
+  }
+
+  // NON_COMPLIANT: Inner test class is missing `@Nested`
+  public class Test2_TestTemplate { // $ Alert
+    @TestTemplate
+    public void test() {
+    }
+  }
+
+  public class Test3 { // COMPLIANT: Since it is empty, it is not a test class
+  }
+
+  public class Test4 { // COMPLIANT: Since no methods have `@Test`, it is not a test class
+    public void f() {
+    }
+
+    public void g() {
+    }
+
+    public void h() {
+    }
+  }
+
+  public static class Test5 { // COMPLIANT: Static nested test classes don't need `@Nested`
+    @Test
+    public void test() {
+    }
+  }
+
+  // COMPLIANT: Invalid to use `@Nested` on a static class, but
+  // this matter is out of scope (see QHelp Implementation Notes)
+  @Nested
+  public static class Test6 {
+    @Test
+    public void test() {
+    }
+  }
+
+  public abstract class Test7 { // COMPLIANT: Abstract nested test classes don't need `@Nested`
+    @Test
+    public void test() {
+    }
+  }
+
+  interface Test8 {
+  }
+
+  public void f() {
+    // COMPLIANT: anonymous classes are not considered as inner test
+    // classes by JUnit and therefore don't need `@Nested`
+    new Test8() {
+      @Test
+      public void test() {
+      }
+    };
+    // COMPLIANT: local classes are not considered as inner test
+    // classes by JUnit and therefore don't need `@Nested`
+    class Test9 {
+      @Test
+      void test() {
+      }
+    }
+  }
+
+  // COMPLIANT: private classes are not considered as inner test
+  // classes by JUnit and therefore don't need `@Nested`
+  private class Test10 {
+    @Test
+    public void test() {
+    }
+  }
+}

java/ql/test/query-tests/Likely Bugs/Frameworks/JUnit/JUnit5MissingNestedAnnotation.expected

@@ -0,0 +1,5 @@
+| AnnotationTest.java:19:16:19:25 | Test2_Test | This JUnit 5 inner test class lacks a '@Nested' annotation. |
+| AnnotationTest.java:26:16:26:33 | Test2_RepeatedTest | This JUnit 5 inner test class lacks a '@Nested' annotation. |
+| AnnotationTest.java:33:16:33:38 | Test2_ParameterizedTest | This JUnit 5 inner test class lacks a '@Nested' annotation. |
+| AnnotationTest.java:41:16:41:32 | Test2_TestFactory | This JUnit 5 inner test class lacks a '@Nested' annotation. |
+| AnnotationTest.java:49:16:49:33 | Test2_TestTemplate | This JUnit 5 inner test class lacks a '@Nested' annotation. |

java/ql/test/query-tests/Likely Bugs/Frameworks/JUnit/JUnit5MissingNestedAnnotation.qlref

@@ -0,0 +1,2 @@
+query: Likely Bugs/Frameworks/JUnit/JUnit5MissingNestedAnnotation.ql
+postprocess: utils/test/InlineExpectationsTestQuery.ql

java/ql/test/query-tests/Likely Bugs/Frameworks/JUnit/options

@@ -0,0 +1 @@
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/junit-jupiter-api-5.2.0