Merge pull request #144 from edx/iamsobanjaved/BOM-2339

Added unittest assertion checker -- BOM-2339

CHANGELOG.rst

@@ -13,6 +13,22 @@ Change Log
 Unreleased
 ~~~~~~~~~~
 
+[4.1.0] - 2021-02-24
+~~~~~~~~~~~~~~~~~~~~
+
+Added unittest_assert module (optional plugin for unittest assertion checks)
+
+To use this plugin, you should add this to your pylintrc
+
+.. code-block:: python
+
+    load-plugins=edx_lint.pylint.unittest_assert
+
+[4.0.1] - 2021-02-04
+~~~~~~~~~~~~~~~~~~~~
+
+edx-lint will now ignore the logging-fstring-interpolation warning in pylint.
+
 [4.0.0] - 2021-01-28
 ~~~~~~~~~~~~~~~~~~~~
 

edx_lint/__init__.py

@@ -2,4 +2,4 @@
 edx_lint standardizes lint configuration and additional plugins for use in
 Open edX code.
 """
-VERSION = "4.0.1"
+VERSION = "4.1.0"

edx_lint/pylint/unittest_assert/__init__.py

@@ -0,0 +1,11 @@
+"""edx_lint unittest_assert module (optional plugin for unittest assertion checks).
+
+Add this to your pylintrc::
+
+    load-plugins=edx_lint.pylint.unittest_assert
+
+"""
+
+from .unittest_assert_check import register_checkers
+
+register = register_checkers

edx_lint/pylint/unittest_assert/unittest_assert_check.py

@@ -0,0 +1,105 @@
+"""Checker for using pytest assertion instead of unittest assertion."""
+import astroid
+
+from pylint.interfaces import IAstroidChecker
+from pylint.checkers import BaseChecker, utils
+
+from edx_lint.pylint.common import BASE_ID, check_visitors
+
+
+def register_checkers(linter):
+    """Register checkers."""
+    linter.register_checker(UnittestAssertChecker(linter))
+
+
+@check_visitors
+class UnittestAssertChecker(BaseChecker):
+    """
+    Checks if a unit test assertion is used, Trigger warning to
+    replace it with pytest assertions
+    """
+
+    __implements__ = (IAstroidChecker,)
+
+    name = "unittest-assert-checker"
+
+    UNITTEST_ASSERTS = [
+        "assertTrue",
+        "assertFalse",
+        "assertEqual",
+        "assertEquals",
+        "assertNotEqual",
+        "assertNotEquals",
+        "assert_",
+        "assertIn",
+        "assertNotIn",
+        "assertLess",
+        "assertLessEqual",
+        "assertGreater",
+        "assertGreaterEqual",
+        "assertAlmostEqual",
+        "assertNotAlmostEqual",
+        "assertIs",
+        "assertIsNot",
+        "assertIsNone",
+        "assertIsNotNone",
+        "assertIsInstance",
+        "assertNotIsInstance",
+        "assertRaises",
+    ]
+
+    ASSERT_MAPPING = {
+        "assertEqual": "assert arg1 == arg2",
+        "assertEquals": "assert arg1 == arg2",
+        "assertNotEqual": "assert arg1 != arg2",
+        "assertNotEquals": "assert arg1 != arg2",
+        "assert_": "assert arg1",
+        "assertTrue": "assert arg1",
+        "assertFalse": "assert not arg1",
+        "assertIn": "assert arg1 in arg2",
+        "assertNotIn": "assert arg1 not in arg2",
+        "assertIs": "assert arg1 is arg2",
+        "assertIsNot": "assert arg1 is not arg2",
+        "assertIsNone": "assert arg1 is None",
+        "assertIsNotNone": "assert arg1 is not None",
+        "assertIsInstance": "assert isinstance(arg1, arg2)",
+        "assertNotIsInstance": "assert not isinstance(arg1, arg2)",
+        "assertLess": "assert arg1 < arg2",
+        "assertLessEqual": "assert arg1 <= arg2",
+        "assertGreater": "assert arg1 > arg2",
+        "assertGreaterEqual": "assert arg1 >= arg2",
+        "assertAlmostEqual": "assert math.isclose(arg1, arg2)",
+        "assertNotAlmostEqual": "assert not math.isclose(arg1, arg2)",
+        "assertRaises": "pytest.raises(arg) or with pytest.raises(arg) as optional_var:",
+    }
+
+    MESSAGE_ID = "avoid-unittest-asserts"
+    msgs = {
+        ("C%d99" % BASE_ID): (
+            "%s",
+            MESSAGE_ID,
+            "Avoid using unittest's assertion methods when using pytest, instead use the 'assert' statement"
+        )
+    }
+
+    @utils.check_messages(MESSAGE_ID)
+    def visit_call(self, node):
+        """
+        Check that unittest assertions are not used.
+        """
+        if not isinstance(node.func, astroid.Attribute):
+            # If it isn't a getattr ignore this. All the assertMethods are
+            # attributes of self:
+            return
+
+        if node.func.attrname not in self.UNITTEST_ASSERTS:
+            # Not an attribute we care about
+            return
+
+        converted_assert = self.ASSERT_MAPPING.get(node.func.attrname, None)
+
+        self.add_message(
+            self.MESSAGE_ID,
+            args=f"{node.func.attrname} should be replaced with a pytest assertion something like `{converted_assert}`",
+            node=node
+        )

test/plugins/pylint_test.py

@@ -42,7 +42,7 @@ def _display(self, layout):
         pass
 
 
-def run_pylint(source, msg_ids):
+def run_pylint(source, msg_ids, *cmd_args):
     """Run pylint on some source, collecting specific messages.
 
     `source` is the literal text of the program to check. It is
@@ -51,6 +51,9 @@ def run_pylint(source, msg_ids):
     `msg_ids` is a comma-separated string of msgids we are interested
     in.  Use "all" to enable all messages.
 
+    `*cmd_args` is the optional list of command line arguments. If calling
+    function wants to send some additional command line arguments.
+
     Returns a set of messages.  Each message is a string, formatted
     as "line:msg-id:message".  "line" will be the line number of the
     message, or if the source line has a comment like "#=Slug", then
@@ -63,7 +66,7 @@ def run_pylint(source, msg_ids):
 
     reporter = SimpleReporter()
 
-    pylint_args = ["source.py", "--disable=all", f"--enable={msg_ids}"]
+    pylint_args = ["source.py", "--disable=all", f"--enable={msg_ids}", *cmd_args]
     if pylint_numversion >= (2, 0):
         kwargs = dict(do_exit=False)
     else:

test/plugins/test_unittest_assert_check.py

@@ -0,0 +1,101 @@
+"""Test unittest_assert_check.py"""
+
+import pytest
+
+from .pylint_test import run_pylint
+
+
+def test_bad_asserts():
+    source = """\
+        import unittest
+
+        class TestUnittestAssertions(unittest.TestCase):
+            def test_wrong_usage(self):
+                # This is the usage of unittest assert functions which shouldn't be used.
+                self.assertEqual('foo'.upper(), 'FOO')
+
+                true = True
+                self.assertTrue(true)
+                self.assertFalse(not true)
+
+                self.assertIn("a", "lala")
+                self.assertNotIn("b", "lala")
+
+                self.assertGreater(1, 0)
+                self.assertLess(1, 2)
+        """
+    messages = run_pylint(source, "avoid-unittest-asserts", '--load-plugins=edx_lint.pylint.unittest_assert')
+    assert messages
+
+
+def test_good_asserts():
+    source = """\
+        import unittest
+
+        class TestPytestAssertions(unittest.TestCase):
+            def test_right_usage(self):
+                # This is the usage of pytest assert functions.
+                assert 'foo'.upper() == 'FOO'
+
+                true = True
+                assert true
+                assert not true
+
+                assert "a" in "lala"
+                assert "b" not in "lala"
+
+                assert 1 > 0
+                assert 1 < 2
+        """
+    messages = run_pylint(source, "avoid-unittest-asserts", '--load-plugins=edx_lint.pylint.unittest_assert')
+    assert not messages
+
+
+@pytest.mark.parametrize(
+    "code, error",
+    [
+        (
+            "assertTrue('foo'.upper() == 'FOO')",
+            "assertTrue should be replaced with a pytest assertion something like `assert arg1`"
+        ),
+        (
+            "assertFalse(500 == 501)",
+            "assertFalse should be replaced with a pytest assertion something like `assert not arg1`"
+        ),
+        (
+            "assertIn('a', 'lala')",
+            "assertIn should be replaced with a pytest assertion something like `assert arg1 in arg2`"
+        ),
+        (
+            "assertIsInstance(1, int)",
+            "assertIsInstance should be replaced with a pytest assertion something like `assert isinstance(arg1, arg2)`"
+        ),
+        (
+            "assertEqual('lala', 'lala')",
+            "assertEqual should be replaced with a pytest assertion something like `assert arg1 == arg2`"
+        ),
+        (
+            "assertAlmostEqual(6.999, 7)",
+            "assertAlmostEqual should be replaced with a pytest assertion something like `assert math.isclose(arg1, "
+            "arg2)`"
+        ),
+        (
+            "assertIsNone(somevar)",
+            "assertIsNone should be replaced with a pytest assertion something like `assert arg1 is None`"
+        ),
+    ],
+)
+def test_assert_hints(code, error):
+    source = (
+        """\
+        import unittest
+
+        class TestUnittestAssertions(unittest.TestCase):
+            def test_wrong_usage(self):
+                self.{}      #=A
+        """
+    ).format(code)
+    messages = run_pylint(source, "avoid-unittest-asserts", '--load-plugins=edx_lint.pylint.unittest_assert')
+
+    expected = {f"A:avoid-unittest-asserts:{error}"}
+    assert expected == messages