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

Author: Napalys

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

@@ -0,0 +1,56 @@
+# J-T-001: Mocking all non-private methods of a class may indicate the unit test is testing too much
+
+Mocking too many non-private methods of a class may indicate that the test is too complicated, possibly because it is trying to test multiple things at once.
+
+## Overview
+
+Mocking methods of a class is necessary for a unit test to run without overhead caused by expensive I/O operations necessary to compute their values. However, if a unit test ends up mocking all of them, it 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 depends 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(Employee.class)).thenReturn(0);                   // Mocked EmployeeRecord.add
+    when(employeeRecordMock.get(String.class)).thenReturn(sampleEmployee);        // Mocked EmployeeRecord.get
+    when(employeeRecordMock.update(Employee.class, String.class)).thenReturn(0);  // Mocked EmployeeRecord.update
+    when(employeeRecordMock.delete(Employee.class)).thenReturn(0);                // Mocked EmployeeRecord.delete
+  }
+
+  @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
+  }
+
+}
+```
+
+## 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/Likely Bugs/Frameworks/JUnit/MockingAllNonPrivateMethodsMeansUnitTestIsTooBig.ql

@@ -0,0 +1,73 @@
+/**
+ * @id java/mocking-all-non-private-methods-means-unit-test-is-too-big
+ * @name J-T-001: Mocking all non-private methods of a class may indicate the unit test is testing too much
+ * @description Mocking all non-private methods provided by a class might indicate the unit test
+ *              aims to test too many things.
+ * @kind problem
+ * @precision high
+ * @problem.severity recommendation
+ * @tags maintainability
+ *       readability
+ */
+
+import java
+
+class MockitoMockCall extends MethodCall {
+  MockitoMockCall() { this.getMethod().hasQualifiedName("org.mockito", "Mockito", "mock") }
+
+  /**
+   * Gets the type that this call intends to mock. e.g.
+   * ``` 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. e.g.
+ * ``` 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-which-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.getParent+().(Stmt) = testMethod.getBody().getAStmt() and
+    mockedClassOrInterface = mockCall.getMockedType() 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/MockingAllNonPrivateMethodsMeansUnitTestIsTooBig/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/MockingAllNonPrivateMethodsMeansUnitTestIsTooBig/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/MockingAllNonPrivateMethodsMeansUnitTestIsTooBig/MockingAllNonPrivateMethodsMeansUnitTestIsTooBig.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/MockingAllNonPrivateMethodsMeansUnitTestIsTooBig/MockingAllNonPrivateMethodsMeansUnitTestIsTooBig.qlref

@@ -0,0 +1 @@
+Likely Bugs/Frameworks/JUnit/MockingAllNonPrivateMethodsMeansUnitTestIsTooBig.ql

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

@@ -0,0 +1,55 @@
+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() {
+    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() {
+    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
+  }
+}

java/ql/test/query-tests/MockingAllNonPrivateMethodsMeansUnitTestIsTooBig/options

@@ -0,0 +1 @@
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../stubs/junit-4.13:${testdir}/../../stubs/mockito-5.14