From b921c4de513c9cc624d6ecf68e4f4493e6e72c0d Mon Sep 17 00:00:00 2001 From: Sean McGinnis Date: Wed, 22 Jul 2020 11:09:08 -0500 Subject: [PATCH] Add H216 to flag use of third party mock Many projects use mocking in their unit tests, and most do not realize that there is a difference between "import mock" and "import unittest.mock", assuming that both use a standard part of the Python library. We've seen many cases where mock is not listed in the project's requirements, but the code imports the third party mock instead of unittest.mock. We've also seen a few break due to this, once their dependencies have stopped pulling in that package for them. There have also been several projects that have taken the effort to switch all of there "import mock" statements over to "import unittest.mock", as well as removing mock from their requirements, only to then accidentally merge a patch that does "import mock" again because it is hard to notice in code reviews. This check is on by default. If a project is using the mock lib, then they are able to explicitly do so by disabling this check. Otherwise, projects don't need to take any action to get this protection, since this is now the recommended default. Change-Id: I8d255a00792a19279074703a8209a3699b480fd0 Signed-off-by: Sean McGinnis --- HACKING.rst | 18 +++++++++++ hacking/checks/mock_checks.py | 35 ++++++++++++++++++++ hacking/tests/checks/test_except_checks.py | 2 +- hacking/tests/checks/test_mock.py | 37 ++++++++++++++++++++++ lower-constraints.txt | 1 - setup.cfg | 1 + test-requirements.txt | 1 - 7 files changed, 92 insertions(+), 3 deletions(-) create mode 100644 hacking/tests/checks/test_mock.py diff --git a/HACKING.rst b/HACKING.rst index ece20f3c..46530e6e 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -361,6 +361,24 @@ exception possible should be used. assertEqual(A in B, False) or assertEqual(False, A in B) to the more specific assertIn/NotIn(A, B) +- [H216] Make sure unittest.mock is used instead of the third party mock + library. On by default. + + Starting with Python 3.3 and later, the mock module was added under unittest. + Previously, this functionality was only available by using the third party + ``mock`` library. + + Most users are not aware of this subtle distinction. This results in issues + where the project does not declare the ``mock`` library in its requirements + file, but the code does an ``import mock`` assuming that the module is + present. This may work initially if one of the project's dependencies ends up + pulling that dependency in indirectly, but then can cause things to suddenly + break if that transitive dependency goes away. + + Since this third party library usage is done without being aware of it, this + check is enabled by default to make sure those projects that actually do + intend to use the ``mock`` library are doing so explicitly. + OpenStack Trademark ------------------- diff --git a/hacking/checks/mock_checks.py b/hacking/checks/mock_checks.py index 51efc178..9204f8c3 100644 --- a/hacking/checks/mock_checks.py +++ b/hacking/checks/mock_checks.py @@ -11,6 +11,7 @@ # under the License. import ast +import re from hacking import core @@ -139,3 +140,37 @@ class FunctionNameFinder(ast.NodeVisitor): if isinstance(node, ast.Call): return super(FunctionNameFinder, self).visit(node.func) return super(FunctionNameFinder, self).visit(node) + + +third_party_mock = re.compile('^import.mock') +from_third_party_mock = re.compile('^from.mock.import') + + +@core.flake8ext +def hacking_no_third_party_mock(logical_line, noqa): + """Check for use of mock instead of unittest.mock. + + Projects have had issues with using mock without including it in their + requirements, thinking it is the standard library version. This makes it so + these projects need to explicitly turn off this check to make it clear they + intended to use it. + + Okay: from unittest import mock + Okay: from unittest.mock import patch + Okay: import unittest.mock + H216: import mock + H216: from mock import patch + Okay: try: import mock + Okay: import mock # noqa + """ + msg = ('H216: The unittest.mock module should be used rather than the ' + 'third party mock package unless actually needed. If so, disable ' + 'the H216 check in hacking config and ensure mock is declared in ' + "the project's requirements.") + + if noqa: + return + + if (re.match(third_party_mock, logical_line) or + re.match(from_third_party_mock, logical_line)): + yield (0, msg) diff --git a/hacking/tests/checks/test_except_checks.py b/hacking/tests/checks/test_except_checks.py index 49a154ba..49789f61 100644 --- a/hacking/tests/checks/test_except_checks.py +++ b/hacking/tests/checks/test_except_checks.py @@ -12,8 +12,8 @@ # limitations under the License. import textwrap +from unittest import mock -import mock import pycodestyle from hacking.checks import except_checks diff --git a/hacking/tests/checks/test_mock.py b/hacking/tests/checks/test_mock.py new file mode 100644 index 00000000..fc1309dd --- /dev/null +++ b/hacking/tests/checks/test_mock.py @@ -0,0 +1,37 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +# implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import ddt + +from hacking.checks import mock_checks +from hacking import tests + + +@ddt.ddt +class MockingTestCase(tests.TestCase): + """This tests hacking checks related to the use of mocking.""" + + @ddt.unpack + @ddt.data( + (1, 'import mock', None), + (0, 'from unittest import mock', None), + (1, 'from mock import patch', None), + (0, 'from unittest.mock import patch', None), + (0, 'from mock', '# noqa')) + def test_H216_hacking_no_third_party_mock(self, err_count, line, noqa): + if err_count > 0: + self.assertCheckFails(mock_checks.hacking_no_third_party_mock, + line, noqa) + else: + self.assertCheckPasses(mock_checks.hacking_no_third_party_mock, + line, noqa) diff --git a/lower-constraints.txt b/lower-constraints.txt index 28cf9f3c..8f70e1a1 100644 --- a/lower-constraints.txt +++ b/lower-constraints.txt @@ -12,7 +12,6 @@ Jinja2==2.10 linecache2==1.0.0 MarkupSafe==1.0 mccabe==0.6.0 -mock==3.0.0 pycodestyle==2.4.0 pyflakes==2.1.1 Pygments==2.2.0 diff --git a/setup.cfg b/setup.cfg index 1bddf3eb..98c312d6 100644 --- a/setup.cfg +++ b/setup.cfg @@ -45,6 +45,7 @@ flake8.extension = H213 = hacking.checks.except_checks:hacking_assert_raises_regexp H214 = hacking.checks.except_checks:hacking_assert_true_or_false_with_in H215 = hacking.checks.except_checks:hacking_assert_equal_in + H216 = hacking.checks.mock_checks:hacking_no_third_party_mock H231 = hacking.checks.python23:hacking_python3x_except_compatible H232 = hacking.checks.python23:hacking_python3x_octal_literals H233 = hacking.checks.python23:hacking_python3x_print_function diff --git a/test-requirements.txt b/test-requirements.txt index c52331a3..dd21ce5f 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -3,7 +3,6 @@ # process, which may cause wedges in the gate later. coverage!=4.4,>=4.0 # Apache-2.0 fixtures>=3.0.0 # Apache-2.0/BSD -mock>=3.0.0 # BSD python-subunit>=1.0.0 # Apache-2.0/BSD stestr>=2.0.0 # Apache-2.0 testscenarios>=0.4 # Apache-2.0/BSD