diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index b1994b3..b9b78f7 100644 --- a/.github/workflows/push.yml +++ b/.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 diff --git a/README.md b/README.md index 8e6219e..3b5f666 100644 --- a/README.md +++ b/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)] diff --git a/scripts/docs.py b/scripts/docs.py index 4ca788b..7fad0f5 100644 --- a/scripts/docs.py +++ b/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 = ["\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("") checker_docs = "\n".join(out) readme_file = Path(__file__).parent.parent.joinpath("README.md") diff --git a/src/databricks/labs/pylint/all.py b/src/databricks/labs/pylint/all.py index b5f25ae..5942527 100644 --- a/src/databricks/labs/pylint/all.py +++ b/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)) diff --git a/src/databricks/labs/pylint/mocking.py b/src/databricks/labs/pylint/mocking.py new file mode 100644 index 0000000..510d974 --- /dev/null +++ b/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": "", + "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)) diff --git a/tests/conftest.py b/tests/conftest.py index cd1a2da..5b80c3d 100644 --- a/tests/conftest.py +++ b/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 diff --git a/tests/test_mocking.py b/tests/test_mocking.py new file mode 100644 index 0000000..45310ff --- /dev/null +++ b/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