Merge pull request #20205 from Napalys/java/mocking-all-non-private-methods-means-unit-test-is-too-big

Java: port quality query `java/mocking-all-non-private-methods-means-unit-test-is-too-big`

Author: Napalys

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

@@ -80,6 +80,7 @@ ql/java/ql/src/Violations of Best Practice/Naming Conventions/LocalShadowsFieldC
 ql/java/ql/src/Violations of Best Practice/Naming Conventions/SameNameAsSuper.ql
 ql/java/ql/src/Violations of Best Practice/Records/IgnoredSerializationMembersOfRecordClass.ql
 ql/java/ql/src/Violations of Best Practice/SpecialCharactersInLiterals/NonExplicitControlAndWhitespaceCharsInLiterals.ql
+ql/java/ql/src/Violations of Best Practice/Testing/ExcessivePublicMethodMocking.ql
 ql/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToRunFinalizersOnExit.ql
 ql/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToStringToString.ql
 ql/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToSystemExit.ql

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

@@ -78,6 +78,7 @@ ql/java/ql/src/Violations of Best Practice/Naming Conventions/LocalShadowsFieldC
 ql/java/ql/src/Violations of Best Practice/Naming Conventions/SameNameAsSuper.ql
 ql/java/ql/src/Violations of Best Practice/Records/IgnoredSerializationMembersOfRecordClass.ql
 ql/java/ql/src/Violations of Best Practice/SpecialCharactersInLiterals/NonExplicitControlAndWhitespaceCharsInLiterals.ql
+ql/java/ql/src/Violations of Best Practice/Testing/ExcessivePublicMethodMocking.ql
 ql/java/ql/src/Violations of Best Practice/Undesirable Calls/CallsToStringToString.ql
 ql/java/ql/src/Violations of Best Practice/Undesirable Calls/DefaultToString.ql
 ql/java/ql/src/Violations of Best Practice/Undesirable Calls/DoNotCallFinalize.ql

java/ql/src/Violations of Best Practice/Testing/ExcessivePublicMethodMocking.md

@@ -0,0 +1,52 @@
+## Overview
+
+Mocking methods of a class is necessary for unit tests to run without overhead caused by expensive I/O operations. However, when a unit test ends up mocking all public methods of a class, it may indicate that the test is too complicated, possibly because it is trying to test multiple things at once. Such extensive mocking is likely a signal that the scope of the unit test is reaching beyond a single unit of functionality.
+
+## Recommendation
+
+It is best to contain the scope of a single unit test to a single unit of functionality. For example, a unit test may aim to test a series of data-transforming functions that depend on an ORM class. Even though the functions may be semantically related with one another, it is better to create a unit test for each function.
+
+## Example
+
+The following example mocks all methods of an ORM class named `EmployeeRecord`, and tests four functions against them. Since the scope of the unit test harbors all four of them, all of the methods provided by the class are mocked.
+
+```java
+public class EmployeeRecord {
+  public int add(Employee employee) { ... }
+
+  public Employee get(String name) { ... }
+
+  public int update(Employee employee, String newName) { ... }
+
+  public int delete(Employee employee) { ... }
+}
+
+public class TestORM {
+  @Test
+  public void nonCompliant() {
+    Employee sampleEmployee = new Employee("John Doe");
+    EmployeeRecord employeeRecordMock = mock(EmployeeRecord.class);  // NON_COMPLIANT: Mocked class has all of its public methods used in the test
+    when(employeeRecordMock.add(sampleEmployee)).thenReturn(0);                   // Mocked EmployeeRecord.add
+    when(employeeRecordMock.get("John Doe")).thenReturn(sampleEmployee);          // Mocked EmployeeRecord.get
+    when(employeeRecordMock.update(sampleEmployee, "Jane Doe")).thenReturn(0);    // Mocked EmployeeRecord.update
+    when(employeeRecordMock.delete(sampleEmployee)).thenReturn(0);                // Mocked EmployeeRecord.delete
+  }
+
+  @Test
+  public void compliant() {
+    Employee sampleEmployee = new Employee("John Doe");
+    EmployeeRecord employeeRecordMock = mock(EmployeeRecord.class); // COMPLIANT: Only some of the public methods belonging to the mocked object are used
+    when(employeeRecordMock.add(sampleEmployee)).thenReturn(0);                // Mocked EmployeeRecord.add
+    when(employeeRecordMock.update(sampleEmployee, "Jane Doe")).thenReturn(0); // Mocked EmployeeRecord.update
+  }
+
+}
+```
+
+## Implementation Notes
+
+JUnit provides two different ways of mocking a method call: `when(mockedObject.methodToMock(...)).thenReturn(...)` and `doReturn(...).when(mockedObject).methodToMock(...)`. Both forms are taken into account by the query.
+
+## References
+
+- Baeldung: [Best Practices for Unit Testing in Java](https://www.baeldung.com/java-unit-testing-best-practices).

java/ql/src/Violations of Best Practice/Testing/ExcessivePublicMethodMocking.ql

@@ -0,0 +1,80 @@
+/**
+ * @id java/excessive-public-method-mocking
+ * @previous-id java/mocking-all-non-private-methods-means-unit-test-is-too-big
+ * @name Mocking all public methods of a class may indicate the unit test is testing too much
+ * @description Mocking all public methods provided by a class might indicate the unit test
+ *              aims to test too many things.
+ * @kind problem
+ * @precision high
+ * @problem.severity recommendation
+ * @tags quality
+ *       maintainability
+ *       readability
+ */
+
+import java
+
+/**
+ * A call to Mockito's `mock` method.
+ */
+class MockitoMockCall extends MethodCall {
+  MockitoMockCall() { this.getMethod().hasQualifiedName("org.mockito", "Mockito", "mock") }
+
+  /**
+   * Gets the type that this call intends to mock. For example:
+   * ```java
+   * EmployeeRecord employeeRecordMock = mock(EmployeeRecord.class);
+   * ```
+   * This predicate gets the class `EmployeeRecord` in the above example.
+   */
+  Type getMockedType() { result = this.getAnArgument().(TypeLiteral).getReferencedType() }
+}
+
+/**
+ * A method call that mocks a target method in a JUnit test. For example:
+ * ```java
+ * EmployeeRecord employeeRecordMock = mock(EmployeeRecord.class);
+ * when(employeeRecordMock.add(sampleEmployee)).thenReturn(0); // Mocked EmployeeRecord.add
+ * doReturn(0).when(employeeRecordMock).add(sampleEmployee);   // Mocked EmployeeRecord.add
+ * ```
+ * This class captures the call to `add` which mocks the equivalent method of the class `EmployeeRecord`.
+ */
+class MockitoMockingMethodCall extends MethodCall {
+  MockitoMockCall mockCall;
+
+  MockitoMockingMethodCall() {
+    /* 1. The qualifier originates from the mock call. */
+    this.getQualifier().getControlFlowNode().getAPredecessor+() = mockCall.getControlFlowNode() and
+    /* 2. The mocked method can be found in the class being mocked with the mock call. */
+    mockCall.getMockedType().(ClassOrInterface).getAMethod() = this.getMethod()
+  }
+
+  /**
+   * Gets the call to Mockito's `mock` from which the qualifier, the mocked object, originates.
+   */
+  MockitoMockCall getMockitoMockCall() { result = mockCall }
+}
+
+/*
+ * The following from-where-select embodies this pseudocode:
+ * - Find a JUnit4TestMethod which:
+ *   - for a class that it mocks with a call to `mock`,
+ *     - for all methods that the class has, there is a method that this test method mocks.
+ */
+
+from JUnit4TestMethod testMethod, ClassOrInterface mockedClassOrInterface
+where
+  exists(MockitoMockCall mockCall |
+    mockCall.getEnclosingCallable() = testMethod and
+    mockedClassOrInterface = mockCall.getMockedType() and
+    // Only flag classes with multiple public methods (2 or more)
+    strictcount(Method m | m = mockedClassOrInterface.getAMethod() and m.isPublic()) > 1 and
+    forex(Method method | method = mockedClassOrInterface.getAMethod() and method.isPublic() |
+      exists(MockitoMockingMethodCall mockedMethod |
+        mockedMethod.getMockitoMockCall() = mockCall and
+        mockedMethod.getMethod() = method
+      )
+    )
+  )
+select testMethod, "This test method mocks all public methods of a $@.", mockedClassOrInterface,
+  "class or an interface"

java/ql/test/query-tests/ExcessivePublicMethodMocking/Employee.java

@@ -0,0 +1,10 @@
+/**
+ * Underlying data type of the ORM class and functions.
+ */
+public class Employee {
+  Employee(String name) {
+    this.name = name;
+  }
+
+  String name;
+}

java/ql/test/query-tests/ExcessivePublicMethodMocking/EmployeeRecord.java

@@ -0,0 +1,26 @@
+/**
+ * Sample ORM class for the type `Employee`.
+ */
+public class EmployeeRecord {
+  public int add(Employee employee) {
+    return 1;
+  }
+
+  public Employee get(String name) {
+    return new Employee("Sample");
+  }
+
+  public int update(Employee employee, String newName) {
+    return 1;
+  }
+
+  public int delete(Employee employee) {
+    return 1;
+  }
+
+  private void f() { }
+
+  private void g() { }
+
+  private void h() { }
+}

java/ql/test/query-tests/ExcessivePublicMethodMocking/EmployeeStatus.java

@@ -0,0 +1,9 @@
+/**
+ * Simple class with a single public method to test the edge case.
+ * When this single method is mocked, it means ALL public methods are mocked.
+ */
+public class EmployeeStatus {
+    public String getStatus() {
+        return "active";
+    }
+}

java/ql/test/query-tests/ExcessivePublicMethodMocking/ExcessivePublicMethodMocking.expected

@@ -0,0 +1,2 @@
+| TestORM.java:34:15:34:27 | nonCompliant1 | This test method mocks all public methods of a $@. | EmployeeRecord.java:4:14:4:27 | EmployeeRecord | class or an interface |
+| TestORM.java:47:15:47:27 | nonCompliant2 | This test method mocks all public methods of a $@. | EmployeeRecord.java:4:14:4:27 | EmployeeRecord | class or an interface |

java/ql/test/query-tests/ExcessivePublicMethodMocking/ExcessivePublicMethodMocking.qlref

@@ -0,0 +1,2 @@
+query: Violations of Best Practice/Testing/ExcessivePublicMethodMocking.ql
+postprocess: utils/test/InlineExpectationsTestQuery.ql

java/ql/test/query-tests/ExcessivePublicMethodMocking/TestORM.java

@@ -0,0 +1,65 @@
+import org.junit.Test;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.doReturn;
+
+public class TestORM {
+  /**
+   * Test of form `when(mockedObject.methodToBeMocked()).thenReturn(someVal)`.
+   */
+  @Test
+  public void compliant1() {
+    Employee sampleEmployee = new Employee("John Doe");
+    EmployeeRecord employeeRecordMock = mock(EmployeeRecord.class); // COMPLIANT: Only some of the public methods belonging to the mocked object are used
+    when(employeeRecordMock.add(sampleEmployee)).thenReturn(0); // Mocked EmployeeRecord.add
+    when(employeeRecordMock.update(sampleEmployee, "Jane Doe")).thenReturn(0); // Mocked EmployeeRecord.update
+  }
+
+  /**
+   * Test of form `doReturn(someVal).when(mockedObject).methodToBeMocked()`.
+   */
+  @Test
+  public void compliant2() {
+    Employee sampleEmployee = new Employee("John Doe");
+    EmployeeRecord employeeRecordMock = mock(EmployeeRecord.class); // COMPLIANT: Only some of the public methods belonging to the mocked object are used
+    doReturn(0).when(employeeRecordMock).add(sampleEmployee); // Mocked EmployeeRecord.add
+    doReturn(0).when(employeeRecordMock).get("John Doe"); // Mocked EmployeeRecord.get
+    doReturn(0).when(employeeRecordMock).delete(sampleEmployee); // Mocked EmployeeRecord.delete
+  }
+
+  /**
+   * Test of form `when(mockedObject.methodToBeMocked()).thenReturn(someVal)`.
+   */
+  @Test
+  public void nonCompliant1() { // $ Alert
+    Employee sampleEmployee = new Employee("John Doe");
+    EmployeeRecord employeeRecordMock = mock(EmployeeRecord.class); // NON_COMPLIANT: All public methods of the mocked object are used
+    when(employeeRecordMock.add(sampleEmployee)).thenReturn(0); // Mocked EmployeeRecord.add
+    when(employeeRecordMock.get("John Doe")).thenReturn(sampleEmployee); // Mocked EmployeeRecord.get
+    when(employeeRecordMock.update(sampleEmployee, "Jane Doe")).thenReturn(0); // Mocked EmployeeRecord.update
+    when(employeeRecordMock.delete(sampleEmployee)).thenReturn(0); // Mocked EmployeeRecord.delete
+  }
+
+  /**
+   * Test of form `doReturn(someVal).when(mockedObject).methodToBeMocked()`.
+   */
+  @Test
+  public void nonCompliant2() { // $ Alert
+    Employee sampleEmployee = new Employee("John Doe");
+    EmployeeRecord employeeRecordMock = mock(EmployeeRecord.class); // NON_COMPLIANT: All public methods of the mocked object are used
+    doReturn(0).when(employeeRecordMock).add(sampleEmployee); // Mocked EmployeeRecord.add
+    doReturn(0).when(employeeRecordMock).get("John Doe"); // Mocked EmployeeRecord.get
+    doReturn(0).when(employeeRecordMock).update(sampleEmployee, "Jane Doe"); // Mocked EmployeeRecord.update
+    doReturn(0).when(employeeRecordMock).delete(sampleEmployee); // Mocked EmployeeRecord.delete
+  }
+
+  /**
+   * Edge case: Class with single public method - should NOT be flagged.
+   * When there's only one public method, mocking it doesn't indicate a "too big" test.
+   */
+  @Test
+  public void compliantSingleMethod() {
+    EmployeeStatus statusMock = mock(EmployeeStatus.class); // COMPLIANT: Single public method, no choice but to mock it if needed
+    when(statusMock.getStatus()).thenReturn("inactive"); // Mocked EmployeeStatus.getStatus (the only public method, but that's OK)
+  }
+}