Merge pull request #4146 from Tadaboody/give_hints_when_an_assertion_value_is_None_instead_of_a_boolean_3191

[#3191] Give hints when an assertion value is None instead of a boolean

changelog/3191.feature.rst

@@ -0,0 +1,17 @@
+A warning is now issued when assertions are made for ``None``.
+
+This is a common source of confusion among new users, which write::
+
+    assert mocked_object.assert_called_with(3, 4, 5, key='value')
+
+When they should write::
+
+    mocked_object.assert_called_with(3, 4, 5, key='value')
+
+Because the ``assert_called_with`` method of mock objects already executes an assertion.
+
+This warning will not be issued when ``None`` is explicitly checked. An assertion like::
+
+    assert variable is None
+
+will not issue the warning.

src/_pytest/assertion/rewrite.py

@@ -51,6 +51,19 @@ else:
         return ast.Call(a, b, c, None, None)
 
 
+def ast_Call_helper(func_name, *args, **kwargs):
+    """
+    func_name: str
+    args: Iterable[ast.expr]
+    kwargs: Dict[str,ast.expr]
+    """
+    return ast.Call(
+        ast.Name(func_name, ast.Load()),
+        list(args),
+        [ast.keyword(key, val) for key, val in kwargs.items()],
+    )
+
+
 class AssertionRewritingHook(object):
     """PEP302 Import hook which rewrites asserts."""
 
@@ -828,6 +841,13 @@ class AssertionRewriter(ast.NodeVisitor):
         self.push_format_context()
         # Rewrite assert into a bunch of statements.
         top_condition, explanation = self.visit(assert_.test)
+        # If in a test module, check if directly asserting None, in order to warn [Issue #3191]
+        if self.module_path is not None:
+            self.statements.append(
+                self.warn_about_none_ast(
+                    top_condition, module_path=self.module_path, lineno=assert_.lineno
+                )
+            )
         # Create failure message.
         body = self.on_failure
         negation = ast.UnaryOp(ast.Not(), top_condition)
@@ -858,6 +878,33 @@ class AssertionRewriter(ast.NodeVisitor):
             set_location(stmt, assert_.lineno, assert_.col_offset)
         return self.statements
 
+    def warn_about_none_ast(self, node, module_path, lineno):
+        """
+        Returns an AST issuing a warning if the value of node is `None`.
+        This is used to warn the user when asserting a function that asserts
+        internally already.
+        See issue #3191 for more details.
+        """
+
+        # Using parse because it is different between py2 and py3.
+        AST_NONE = ast.parse("None").body[0].value
+        val_is_none = ast.Compare(node, [ast.Is()], [AST_NONE])
+        send_warning = ast.parse(
+            """
+from _pytest.warning_types import PytestWarning
+from warnings import warn_explicit
+warn_explicit(
+    PytestWarning('asserting the value None, please use "assert is None"'),
+    category=None,
+    filename={filename!r},
+    lineno={lineno},
+)
+            """.format(
+                filename=module_path.strpath, lineno=lineno
+            )
+        ).body
+        return ast.If(val_is_none, send_warning, [])
+
     def visit_Name(self, name):
         # Display the repr of the name if it's a local variable or
         # _should_repr_global_name() thinks it's acceptable.

testing/test_warnings.py

@@ -623,3 +623,63 @@ def test_removed_in_pytest4_warning_as_error(testdir, change_default):
     else:
         assert change_default in ("ini", "cmdline")
         result.stdout.fnmatch_lines(["* 1 passed in *"])
+
+
+class TestAssertionWarnings:
+    @staticmethod
+    def assert_result_warns(result, msg):
+        result.stdout.fnmatch_lines(["*PytestWarning: %s*" % msg])
+
+    def test_tuple_warning(self, testdir):
+        testdir.makepyfile(
+            """
+            def test_foo():
+                assert (1,2)
+            """
+        )
+        result = testdir.runpytest()
+        self.assert_result_warns(
+            result, "assertion is always true, perhaps remove parentheses?"
+        )
+
+    @staticmethod
+    def create_file(testdir, return_none):
+        testdir.makepyfile(
+            """
+            def foo(return_none):
+                if return_none:
+                    return None
+                else:
+                    return False
+
+            def test_foo():
+                assert foo({return_none})
+            """.format(
+                return_none=return_none
+            )
+        )
+
+    def test_none_function_warns(self, testdir):
+        self.create_file(testdir, True)
+        result = testdir.runpytest()
+        self.assert_result_warns(
+            result, 'asserting the value None, please use "assert is None"'
+        )
+
+    def test_assert_is_none_no_warn(self, testdir):
+        testdir.makepyfile(
+            """
+            def foo():
+                return None
+
+            def test_foo():
+                assert foo() is None
+            """
+        )
+        result = testdir.runpytest()
+        result.stdout.fnmatch_lines(["*1 passed in*"])
+
+    def test_false_function_no_warn(self, testdir):
+        self.create_file(testdir, False)
+        result = testdir.runpytest()
+        result.stdout.fnmatch_lines(["*1 failed in*"])