diff --git a/changelog/3191.feature.rst b/changelog/3191.feature.rst new file mode 100644 index 000000000..7eb4c3a15 --- /dev/null +++ b/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. diff --git a/src/_pytest/assertion/rewrite.py b/src/_pytest/assertion/rewrite.py index d1231b774..78b8edcd8 100644 --- a/src/_pytest/assertion/rewrite.py +++ b/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. diff --git a/testing/test_warnings.py b/testing/test_warnings.py index 53d9c71cd..655c89f4c 100644 --- a/testing/test_warnings.py +++ b/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*"])