Added checks for missing mock usage

README.md

@@ -43,6 +43,8 @@ PyLint with checks for common mistakes and issues in Python code specifically in
   * [`mocking` checker](#mocking-checker)
     * [`R8918`: `explicit-dependency-required`](#r8918-explicit-dependency-required)
     * [`R8919`: `obscure-mock`](#r8919-obscure-mock)
+    * [`R8921`: `mock-no-assign`](#r8921-mock-no-assign)
+    * [`R8922`: `mock-no-usage`](#r8922-mock-no-usage)
   * [`eradicate` checker](#eradicate-checker)
     * [`C8920`: `dead-code`](#c8920-dead-code)
   * [Testing in isolation](#testing-in-isolation)
@@ -337,6 +339,22 @@ To disable this check on a specific line, add `# pylint: disable=obscure-mock` a
 
 [[back to top](#pylint-plugin-for-databricks)]
 
+### `R8921`: `mock-no-assign`
+
+Mock not assigned to a variable: XXX. Every mocked object should be assigned to a variable to allow for assertions.
+
+To disable this check on a specific line, add `# pylint: disable=mock-no-assign` at the end of it.
+
+[[back to top](#pylint-plugin-for-databricks)]
+
+### `R8922`: `mock-no-usage`
+
+Missing usage of mock for XXX. Usually this check means a hidden bug, where object is mocked, but we don't check if it was used correctly. Every mock should have at least one assertion, return value, or side effect specified.
+
+To disable this check on a specific line, add `# pylint: disable=mock-no-usage` at the end of it.
+
+[[back to top](#pylint-plugin-for-databricks)]
+
 ## `eradicate` checker
 To use this checker, add `databricks.labs.pylint.eradicate` to `load-plugins` configuration in your `pylintrc` or `pyproject.toml` file.
 
@@ -354,7 +372,7 @@ To disable this check on a specific line, add `# pylint: disable=dead-code` at t
 To test this plugin in isolation, you can use the following command:
 
 ```bash
-pylint --load-plugins=databricks.labs.pylint.all --disable=all --enable=missing-data-security-mode,unsupported-runtime,dbutils-fs-cp,dbutils-fs-head,dbutils-fs-ls,dbutils-fs-mount,dbutils-credentials,dbutils-notebook-run,pat-token-leaked,internal-api,legacy-cli,incompatible-with-uc,notebooks-too-many-cells,notebooks-percent-run,spark-outside-function,use-display-instead-of-show,no-spark-argument-in-function,explicit-dependency-required,obscure-mock,dead-code .
+pylint --load-plugins=databricks.labs.pylint.all --disable=all --enable=missing-data-security-mode,unsupported-runtime,dbutils-fs-cp,dbutils-fs-head,dbutils-fs-ls,dbutils-fs-mount,dbutils-credentials,dbutils-notebook-run,pat-token-leaked,internal-api,legacy-cli,incompatible-with-uc,notebooks-too-many-cells,notebooks-percent-run,spark-outside-function,use-display-instead-of-show,no-spark-argument-in-function,explicit-dependency-required,obscure-mock,mock-no-assign,mock-no-usage,dead-code .
 ```
 
 [[back to top](#pylint-plugin-for-databricks)]

src/databricks/labs/pylint/mocking.py

@@ -42,6 +42,17 @@
             "obscure-mock",
             DOC_OBSCURE_MOCK,
         ),
+        "R8921": (
+            "Mock not assigned to a variable: %s",
+            "mock-no-assign",
+            "Every mocked object should be assigned to a variable to allow for assertions.",
+        ),
+        "R8922": (
+            "Missing usage of mock for %s",
+            "mock-no-usage",
+            "Usually this check means a hidden bug, where object is mocked, but we don't check if it was used "
+            "correctly. Every mock should have at least one assertion, return value, or side effect specified.",
+        ),
     }
 
     options = (
@@ -65,6 +76,8 @@
             # here we can go and figure out the expected type of the object being mocked based on the arguments
             # where it is being assigned to, but that is a bit too much for this check. Other people can add this later.
             self.add_message("obscure-mock", node=node)
+        if node.func.as_string() == "create_autospec" and self._no_mock_usage(node):
+            return
         if not node.args:
             return
         if self._require_explicit_dependency and node.func.as_string() in {"mocker.patch", "patch"}:
@@ -75,6 +88,37 @@
                     continue
                 self.add_message("explicit-dependency-required", node=node, args=argument_value)
 
+    def _no_mock_usage(self, node: nodes.Call) -> bool:
+        assignment = node.parent
+        if not isinstance(assignment, nodes.Assign):
+            self.add_message("mock-no-assign", node=node, args=node.as_string())
+            return True
+        if not assignment.targets:
+            self.add_message("mock-no-assign", node=node, args=assignment.as_string())
+            return True
+        mocked_type = node.args[0].as_string()
+        variable = assignment.targets[0].as_string()
+        has_assertion = False
+        has_return_value = False
+        for statement in assignment.parent.get_children():
+            statement_str = statement.as_string()
+            if not statement_str.startswith(f"{variable}."):
+                # this is not a method call on the variable we are interested in
+                continue
+            if ".assert" in statement_str:
+                has_assertion = True
+                break
+            if ".return_value" in statement_str:
+                has_return_value = True
+                break
+            if ".side_effect" in statement_str:
+                has_return_value = True
+                break
+        if not has_assertion and not has_return_value:
+            self.add_message("mock-no-usage", node=node, args=mocked_type)
+            return True
+        return False
+
 
 def register(linter):
     linter.register_checker(MockingChecker(linter))

tests/test_mocking.py

@@ -15,3 +15,64 @@ def test_explicit_dependency_required(lint_with):
         "Rewrite to inject dependencies through constructor."
     )
     assert _ in messages
+
+
+def test_mock_in_function_arg(lint_with):
+    messages = (
+        lint_with(MockingChecker)
+        << """some_fn(
+    some_arg,
+    create_autospec(ConcreteType), #@
+    True,
+)"""
+    )
+
+    assert "[mock-no-assign] Mock not assigned to a variable: create_autospec(ConcreteType)" in messages
+
+
+def test_mock_not_assigned(lint_with):
+    messages = (
+        lint_with(MockingChecker)
+        << """_ = 1
+create_autospec(ConcreteType) #@
+some_fn(some_arg, True)
+"""
+    )
+
+    assert "[mock-no-assign] Mock not assigned to a variable: create_autospec(ConcreteType)" in messages
+
+
+def test_mock_return_value_real(lint_with):
+    messages = (
+        lint_with(MockingChecker)
+        << """with _lock:
+    installation = mock_installation()
+    if 'workspace_client' not in replace:
+        ws = (
+            create_autospec(WorkspaceClient) #@
+        )
+        ws.api_client.do.return_value = {}
+        ws.permissions.get.return_value = {}
+        replace['workspace_client'] = ws
+    if 'sql_backend' not in replace:
+        replace['sql_backend'] = MockBackend()
+"""
+    )
+
+    assert not messages
+
+
+def test_mock_is_asserted(lint_with):
+    messages = (
+        lint_with(MockingChecker)
+        << """_ = 1
+mocked_thing = ( # wrapping in parentheses to fetch call node
+    create_autospec(ConcreteType) #@
+)
+mocked_thing.foo.bar.return_value = 42
+some_fn(some_arg, mocked_thing, True)
+mocked_thing.foo.bar.assert_called_once()
+"""
+    )
+
+    assert not messages