Added checks for mocking best practices

.github/workflows/push.yml

@@ -14,6 +14,9 @@ on:
     branches:
       - main
 
+env:
+  HATCH_VERSION: 1.9.4
+
 jobs:
   ci:
     strategy:
@@ -32,10 +35,11 @@ jobs:
           cache-dependency-path: '**/pyproject.toml'
           python-version: ${{ matrix.pyVersion }}
 
+      - name: Install hatch
+        run: pip install hatch==$HATCH_VERSION
+
       - name: Run unit tests
-        run: |
-          pip install hatch==1.9.4
-          make test
+        run: make test
 
       - name: Publish test coverage
         uses: codecov/codecov-action@v4
@@ -46,8 +50,11 @@ jobs:
       - name: Checkout
         uses: actions/checkout@v4
 
+      - name: Install hatch
+        run: pip install hatch==$HATCH_VERSION
+
       - name: Format all files
-        run: make dev fmt
+        run: make fmt
 
       - name: Fail on differences
         run: git diff --exit-code

README.md

@@ -247,11 +247,50 @@ Function XXX is missing a 'spark' argument. Function refers to a global spark va
 
 [[back to top](#pylint-plugin-for-databricks)]
 
+## `mocking` checker
+
+[[back to top](#pylint-plugin-for-databricks)]
+
+### `R8918`: `explicit-dependency-required`
+
+Obscure implicit test dependency with mock.patch(XXX). Rewrite to inject dependencies through constructor.. Using `patch` to mock dependencies in unit tests can introduce implicit 
+dependencies within a class, making it unclear to other developers. Constructor arguments, on the other hand, 
+explicitly declare dependencies, enhancing code readability and maintainability. However, reliance on `patch` 
+for testing may lead to issues during refactoring, as updates to underlying implementations would necessitate 
+changes across multiple unrelated unit tests. Moreover, the use of hard-coded strings in `patch` can obscure 
+which unit tests require modification, as they lack strongly typed references. This coupling of the class 
+under test to concrete classes signifies a code smell, and such code is not easily portable to statically typed 
+languages where monkey patching isn't feasible without significant effort. In essence, extensive patching of 
+external clients suggests a need for refactoring, with experienced engineers recognizing the potential for 
+dependency inversion in such scenarios.
+
+To address this issue, refactor the code to inject dependencies through the constructor. This approach
+explicitly declares dependencies, enhancing code readability and maintainability. Moreover, it allows for
+dependency inversion, enabling the use of interfaces to decouple the class under test from concrete classes.
+This decoupling facilitates unit testing, as it allows for the substitution of mock objects for concrete
+implementations, ensuring that the class under test behaves as expected. By following this approach, you can
+create more robust and maintainable unit tests, improving the overall quality of your codebase.
+
+Use `require-explicit-dependency` option to specify the package names that contain code for your project.
+
+[[back to top](#pylint-plugin-for-databricks)]
+
+### `R8919`: `obscure-mock`
+
+Obscure implicit test dependency with MagicMock(). Rewrite with create_autospec(ConcreteType).. Using `MagicMock` to mock dependencies in unit tests can introduce implicit dependencies 
+within a class, making it unclear to other developers. create_autospec(ConcreteType) is a better alternative, as it
+automatically creates a mock object with the same attributes and methods as the concrete class. This
+approach ensures that the mock object behaves like the concrete class, allowing for more robust and
+maintainable unit tests. Moreover, reliance on `MagicMock` for testing leads to issues during refactoring,
+as updates to underlying implementations would necessitate changes across multiple unrelated unit tests.
+
+[[back to top](#pylint-plugin-for-databricks)]
+
 ## Testing in isolation
 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 .
+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 .
 ```
 
 [[back to top](#pylint-plugin-for-databricks)]

scripts/docs.py

@@ -6,11 +6,13 @@
 from databricks.labs.pylint.airflow import AirflowChecker
 from databricks.labs.pylint.dbutils import DbutilsChecker
 from databricks.labs.pylint.legacy import LegacyChecker
+from databricks.labs.pylint.mocking import MockingChecker
 from databricks.labs.pylint.notebooks import NotebookChecker
 from databricks.labs.pylint.spark import SparkChecker
 
 
 def do_something():
+    heading_anchor = "\n[[back to top](#pylint-plugin-for-databricks)]\n"
     out = ["<!-- CHECKS -->\n"]
     symbols = []
     linter = PyLinter()
@@ -20,20 +22,21 @@ def do_something():
         LegacyChecker(linter),
         NotebookChecker(linter),
         SparkChecker(linter),
+        MockingChecker(linter),
     ]:
         out.append(f"## `{checker.name}` checker")
-        out.append("\n[[back to top](#databricks-labs-pylint-plugin)]\n")
+        out.append(heading_anchor)
         for msg_def in checker.messages:
             out.append(f"### `{msg_def.msgid}`: `{msg_def.symbol}`\n")
             out.append(f"{msg_def.msg.replace('%s', 'XXX')}. {msg_def.description}")
-            out.append("\n[[back to top](#databricks-labs-pylint-plugin)]\n")
+            out.append(heading_anchor)
             symbols.append(msg_def.symbol)
     out.append("## Testing in isolation")
     out.append("To test this plugin in isolation, you can use the following command:\n")
     out.append("```bash")
     out.append(f"pylint --load-plugins=databricks.labs.pylint.all --disable=all --enable={','.join(symbols)} .")
     out.append("```")
-    out.append("\n[[back to top](#databricks-labs-pylint-plugin)]\n")
+    out.append(heading_anchor)
     out.append("<!-- END CHECKS -->")
     checker_docs = "\n".join(out)
     readme_file = Path(__file__).parent.parent.joinpath("README.md")

src/databricks/labs/pylint/all.py

@@ -1,6 +1,7 @@
 from databricks.labs.pylint.airflow import AirflowChecker
 from databricks.labs.pylint.dbutils import DbutilsChecker
 from databricks.labs.pylint.legacy import LegacyChecker
+from databricks.labs.pylint.mocking import MockingChecker
 from databricks.labs.pylint.notebooks import NotebookChecker
 from databricks.labs.pylint.spark import SparkChecker
 
@@ -11,3 +12,4 @@ def register(linter):
     linter.register_checker(LegacyChecker(linter))
     linter.register_checker(AirflowChecker(linter))
     linter.register_checker(SparkChecker(linter))
+    linter.register_checker(MockingChecker(linter))

src/databricks/labs/pylint/mocking.py

@@ -0,0 +1,80 @@
+from astroid import nodes  # type: ignore
+from pylint.checkers import BaseChecker
+
+DOC_EXPLICIT_DEPENDENCY_REQUIRED = """Using `patch` to mock dependencies in unit tests can introduce implicit 
+dependencies within a class, making it unclear to other developers. Constructor arguments, on the other hand, 
+explicitly declare dependencies, enhancing code readability and maintainability. However, reliance on `patch` 
+for testing may lead to issues during refactoring, as updates to underlying implementations would necessitate 
+changes across multiple unrelated unit tests. Moreover, the use of hard-coded strings in `patch` can obscure 
+which unit tests require modification, as they lack strongly typed references. This coupling of the class 
+under test to concrete classes signifies a code smell, and such code is not easily portable to statically typed 
+languages where monkey patching isn't feasible without significant effort. In essence, extensive patching of 
+external clients suggests a need for refactoring, with experienced engineers recognizing the potential for 
+dependency inversion in such scenarios.
+
+To address this issue, refactor the code to inject dependencies through the constructor. This approach
+explicitly declares dependencies, enhancing code readability and maintainability. Moreover, it allows for
+dependency inversion, enabling the use of interfaces to decouple the class under test from concrete classes.
+This decoupling facilitates unit testing, as it allows for the substitution of mock objects for concrete
+implementations, ensuring that the class under test behaves as expected. By following this approach, you can
+create more robust and maintainable unit tests, improving the overall quality of your codebase.
+
+Use `require-explicit-dependency` option to specify the package names that contain code for your project."""
+
+DOC_OBSCURE_MOCK = """Using `MagicMock` to mock dependencies in unit tests can introduce implicit dependencies 
+within a class, making it unclear to other developers. create_autospec(ConcreteType) is a better alternative, as it
+automatically creates a mock object with the same attributes and methods as the concrete class. This
+approach ensures that the mock object behaves like the concrete class, allowing for more robust and
+maintainable unit tests. Moreover, reliance on `MagicMock` for testing leads to issues during refactoring,
+as updates to underlying implementations would necessitate changes across multiple unrelated unit tests."""
+
+
+class MockingChecker(BaseChecker):
+    name = "mocking"
+    msgs = {
+        "R8918": (
+            "Obscure implicit test dependency with mock.patch(%s). Rewrite to inject dependencies through constructor.",
+            "explicit-dependency-required",
+            DOC_EXPLICIT_DEPENDENCY_REQUIRED,
+        ),
+        "R8919": (
+            "Obscure implicit test dependency with MagicMock(). Rewrite with create_autospec(ConcreteType).",
+            "obscure-mock",
+            DOC_OBSCURE_MOCK,
+        ),
+    }
+
+    options = (
+        (
+            "require-explicit-dependency",
+            {
+                "default": ("databricks",),
+                "type": "csv",
+                "metavar": "<modules>",
+                "help": "Package names that contain code for your project.",
+            },
+        ),
+    )
+
+    def open(self) -> None:
+        self._require_explicit_dependency = self.linter.config.require_explicit_dependency
+
+    def visit_call(self, node: nodes.Call) -> None:
+        # this also means that rare cases, like MagicMock(side_effect=...) are fine
+        if node.as_string() == "MagicMock()":
+            # 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 not node.args:
+            return
+        if self._require_explicit_dependency and node.func.as_string() in {"mocker.patch", "patch"}:
+            argument_value = node.args[0].as_string()
+            no_quotes = argument_value.strip("'\"")
+            for module in self._require_explicit_dependency:
+                if not no_quotes.startswith(module):
+                    continue
+                self.add_message("explicit-dependency-required", node=node, args=argument_value)
+
+
+def register(linter):
+    linter.register_checker(MockingChecker(linter))

tests/conftest.py

@@ -10,8 +10,10 @@
 
 
 class TestSupport(Generic[T]):
-    def __init__(self, klass: Type[T]):
+    def __init__(self, klass: Type[T], config: dict):
         linter = UnittestLinter()
+        for k, v in config.items():
+            setattr(linter.config, k, v)
         checker = klass(linter)
         checker.open()
         linter.register_checker(checker)
@@ -39,7 +41,7 @@ def __lshift__(self, code: str):
 
 @pytest.fixture
 def lint_with():
-    def factory(klass: Type[T]) -> TestSupport[T]:
-        return TestSupport(klass)
+    def factory(klass: Type[T], **config) -> TestSupport[T]:
+        return TestSupport(klass, config)
 
     yield factory

tests/test_mocking.py

@@ -0,0 +1,17 @@
+from databricks.labs.pylint.mocking import MockingChecker
+
+
+def test_obscure_mock(lint_with):
+    messages = lint_with(MockingChecker) << "MagicMock()"
+    _ = "[obscure-mock] Obscure implicit test dependency with MagicMock(). Rewrite with create_autospec(ConcreteType)."
+    assert _ in messages
+
+
+def test_explicit_dependency_required(lint_with):
+    messages = lint_with(MockingChecker) << "mocker.patch('databricks.sdk.foo')"
+
+    _ = (
+        "[explicit-dependency-required] Obscure implicit test dependency with mock.patch('databricks.sdk.foo'). "
+        "Rewrite to inject dependencies through constructor."
+    )
+    assert _ in messages