Skip to content

Commit

Permalink
Added checks for missing mock usage
Browse files Browse the repository at this point in the history
### `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.
  • Loading branch information
nfx committed Apr 26, 2024
1 parent 0b8b025 commit f03ca75
Show file tree
Hide file tree
Showing 3 changed files with 124 additions and 1 deletion.
20 changes: 19 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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.

Expand All @@ -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)]
Expand Down
44 changes: 44 additions & 0 deletions src/databricks/labs/pylint/mocking.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = (
Expand All @@ -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"}:
Expand All @@ -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

Check warning on line 98 in src/databricks/labs/pylint/mocking.py

View check run for this annotation

Codecov / codecov/patch

src/databricks/labs/pylint/mocking.py#L97-L98

Added lines #L97 - L98 were not covered by tests
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

Check warning on line 110 in src/databricks/labs/pylint/mocking.py

View check run for this annotation

Codecov / codecov/patch

src/databricks/labs/pylint/mocking.py#L109-L110

Added lines #L109 - L110 were not covered by tests
if ".return_value" in statement_str:
has_return_value = True
break
if ".side_effect" in statement_str:
has_return_value = True
break

Check warning on line 116 in src/databricks/labs/pylint/mocking.py

View check run for this annotation

Codecov / codecov/patch

src/databricks/labs/pylint/mocking.py#L115-L116

Added lines #L115 - L116 were not covered by tests
if not has_assertion and not has_return_value:
self.add_message("mock-no-usage", node=node, args=mocked_type)
return True

Check warning on line 119 in src/databricks/labs/pylint/mocking.py

View check run for this annotation

Codecov / codecov/patch

src/databricks/labs/pylint/mocking.py#L118-L119

Added lines #L118 - L119 were not covered by tests
return False


def register(linter):
linter.register_checker(MockingChecker(linter))
61 changes: 61 additions & 0 deletions tests/test_mocking.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit f03ca75

Please sign in to comment.