From 9dde7fd853f06adbc02cc27ef3790c6434e2121a Mon Sep 17 00:00:00 2001
From: Serge Smertin <259697+nfx@users.noreply.github.com>
Date: Fri, 26 Apr 2024 13:59:28 +0200
Subject: [PATCH] Added checks for missing mock usage (#44)

### `R8921`: `mock-no-assign`

```
some_fn(some_arg, create_autospec(ConcreteType), True)
```

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`

```
mocked_thing = create_autospec(ConcreteType)
some_fn(some_arg, mocked_thing, True)
```

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.
---
 README.md                             | 20 ++++++++-
 src/databricks/labs/pylint/mocking.py | 44 +++++++++++++++++++
 tests/test_mocking.py                 | 61 +++++++++++++++++++++++++++
 3 files changed, 124 insertions(+), 1 deletion(-)

diff --git a/README.md b/README.md
index 09a428f..0e5200d 100644
--- a/README.md
+++ b/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)]
diff --git a/src/databricks/labs/pylint/mocking.py b/src/databricks/labs/pylint/mocking.py
index 510d974..d63f228 100644
--- a/src/databricks/labs/pylint/mocking.py
+++ b/src/databricks/labs/pylint/mocking.py
@@ -42,6 +42,17 @@ class MockingChecker(BaseChecker):
             "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 @@ def visit_call(self, node: nodes.Call) -> None:
             # 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 @@ def visit_call(self, node: nodes.Call) -> None:
                     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))
diff --git a/tests/test_mocking.py b/tests/test_mocking.py
index 45310ff..90e0c1e 100644
--- a/tests/test_mocking.py
+++ b/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