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

Tadaboody · 2018-10-14T16:28:45Z

Thanks for submitting a PR, your contribution is really appreciated!

Here's a quick checklist that should be present in PRs (you can delete this text from the final description, this is
just a guideline):

Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.
Target the master branch for bug fixes, documentation updates and trivial changes.
Target the features branch for new features and removals/deprecations.
Include documentation when adding new features.
Include new tests or update existing tests when applicable.

Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:

Add yourself to AUTHORS in alphabetical order;

Closes #3191

nicoddemus · 2018-10-14T18:40:05Z

testing/test_warnings.py

+                assert (1,2)
+            """
+        )
+        with pytest.warns(pytest.PytestWarning):


This will never happen, as runpytest_subprocess() runs pytest in a separate process, which won't trigger a PytestWarning. You should check stdout instead (and no need to run in a separate process, you can use just runpytest().

Gotcha, Will change. I inferred this by snooping around other tests and got it wrong.
Where in the documentation are all these methods specified?
I think that a section in the contributing guide describing the inner use APIs or linking to a separate "On testing tests" guide will go a long way

You are right, created #4151 to track this. 👍

Here are the full docs to testdir, although they could use some improvement.

codecov · 2018-10-15T07:54:40Z

Codecov Report

Merging #4146 into features will increase coverage by <.01%.
The diff coverage is 96.66%.

@@             Coverage Diff              @@
##           features    #4146      +/-   ##
============================================
+ Coverage     95.75%   95.75%   +<.01%     
============================================
  Files           111      111              
  Lines         24867    24897      +30     
  Branches       2455     2457       +2     
============================================
+ Hits          23812    23841      +29     
  Misses          746      746              
- Partials        309      310       +1

Flag	Coverage Δ
#docs	`29.38% <23.33%> (+0.06%)`	⬆️
#doctesting	`29.38% <23.33%> (+0.06%)`	⬆️
#linting	`29.38% <23.33%> (+0.06%)`	⬆️
#linux	`95.58% <96.66%> (-0.01%)`	⬇️
#nobyte	`92.46% <96.66%> (-0.01%)`	⬇️
#numpy	`93.21% <96.66%> (ø)`	⬆️
#pexpect	`41.73% <23.33%> (-0.03%)`	⬇️
#py27	`93.8% <96.66%> (-0.01%)`	⬇️
#py34	`91.97% <96.66%> (+0.07%)`	⬆️
#py35	`91.99% <96.66%> (+0.06%)`	⬆️
#py36	`92.01% <96.66%> (+0.06%)`	⬆️
#py37	`93.94% <96.66%> (+0.01%)`	⬆️
#trial	`93.21% <96.66%> (ø)`	⬆️
#windows	`94% <96.66%> (ø)`	⬆️
#xdist	`93.85% <96.66%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
testing/test_warnings.py	`98.81% <100%> (+0.16%)`	⬆️
src/_pytest/assertion/rewrite.py	`95.6% <88.88%> (-0.1%)`	⬇️
src/_pytest/cacheprovider.py	`96.15% <0%> (-0.97%)`	⬇️
src/_pytest/capture.py	`93.87% <0%> (+0.45%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5db46d2...5ebacc4. Read the comment docs.

nicoddemus · 2018-10-15T15:24:05Z

testing/test_warnings.py

+            """
+        )
+        result = testdir.runpytest()
+        assert self.result_warns(result)


I suggest to use stdout.fnmatch_lines instead:

result = testdir.runpytest() result.stdout.fnmatch_lines(["*PytestWarning*"])

Tadaboody · 2018-10-22T17:13:45Z

@nicoddemus so we because we look at function returns we need to emit the warning at test runtime, where exactly does that happen? I'm getting a little bit lost in the project structure - is there somewhere in the docs I can look to understand a bit more under the hood?

nicoddemus · 2018-10-23T20:21:18Z

Hi @Tadaboody,

Sorry for taking so long to get back to this.

@nicoddemus so we because we look at function returns we need to emit the warning at test runtime, where exactly does that happen? I'm getting a little bit lost in the project structure - is there somewhere in the docs I can look to understand a bit more under the hood?

I think the solution is to change how we write the asserts to emit a warning if the expression evaluated is None.

For example, take a look at pytest-ast-back-to-python, it lets you see the exact rewritten assert code generated by pytest:

def test_simple():
    a = 1
    b = 2
    assert a + b == 3

Becomes:

def test_simple():
    a = 1
    b = 2
    @py_assert0 = 1
    @py_assert2 = 2
    @py_assert4 = @py_assert0 + @py_assert2
    @py_assert6 = 3
    @py_assert5 = @py_assert4 == @py_assert6
    if not @py_assert5:
        @py_format8 = @pytest_ar._call_reprcompare(('==',), (@py_assert5,), ('(%(py1)s + %(py3)s) == %(py7)s',), (@py_assert4, @py_assert6)) % {'py3': @pytest_ar._saferepr(@py_assert2), 'py1': @pytest_ar._saferepr(@py_assert0), 'py7': @pytest_ar._saferepr(@py_assert6)}
        @py_format10 = ('' + 'assert %(py9)s') % {'py9': @py_format8}
        raise AssertionError(@pytest_ar._format_explanation(@py_format10))
    @py_assert0 = @py_assert2 = @py_assert4 = @py_assert5 = @py_assert6 = None

We need to change that generated code into something like this:

def test_simple():
    a = 1
    b = 2
    @py_assert0 = 1
    @py_assert2 = 2
    @py_assert4 = @py_assert0 + @py_assert2
    @py_assert6 = 3
    @py_assert5 = @py_assert4 == @py_assert6
    if not @py_assert5:
        @py_format8 = @pytest_ar._call_reprcompare(('==',), (@py_assert5,), ('(%(py1)s + %(py3)s) == %(py7)s',), (@py_assert4, @py_assert6)) % {'py3': @pytest_ar._saferepr(@py_assert2), 'py1': @pytest_ar._saferepr(@py_assert0), 'py7': @pytest_ar._saferepr(@py_assert6)}
        @py_format10 = ('' + 'assert %(py9)s') % {'py9': @py_format8}        
        if @py_assert5 is None:
            none_msg = '  (expression is None, make sure this is correct)'
        else:
            none_msg = ''
        raise AssertionError(@pytest_ar._format_explanation(@py_format10) + none_msg)
    @py_assert0 = @py_assert2 = @py_assert4 = @py_assert5 = @py_assert6 = None

#3479 by @Sup3rGeo is an example which changes the generated code and might be a good reference.

@RonnyPfannschmidt might have some suggestions here as well.

Sup3rGeo · 2018-10-23T20:32:03Z

@Tadaboody so basic the main place to play with is the visit_Assert function in rewrite.py:

pytest/src/_pytest/assertion/rewrite.py

Line 799 in 72d98a7

def visit_Assert(self, assert_):

I suggest that you play changing the ast statements and then checking the resulting python code (probably using pytest-ast-back-to-python as @nicoddemus pointed out, although I was doing this manually using astunparse), so you can understand how to create valid ast and then generate something like what @nicoddemus has shown.

Tadaboody · 2018-10-24T21:16:04Z

Edit: This got fixed itself after I woke up. Is this what people call a bed bug?

Thanks for the help @nicoddemus @Sup3rGeo I think I finally got What I'm supposed to do!
I'm a bit stuck though, when trying to run on

# stub.py
def test_foo():
    assert None

To try and see what the ast looks like now I keep on getting

  File "/Users/tomer/Forks/pytest/src/_pytest/assertion/rewrite.py", line 422, in _rewrite_test
    co = compile(tree, fn.strpath, "exec", dont_inherit=True)
TypeError: required field "lineno" missing from expr

From what I gather,
https://github.com/Tadaboody/pytest/blob/a07ddf8e6900cd6d612dd87a054aec39cc5ebfc1/src/_pytest/assertion/rewrite.py#L873
should be setting the lineno of all the nodes, recursively. so what's actually happening?

Full traceback:

rm -rf __pycache__/ && py.test --show-ast-as-python stub.py
Traceback (most recent call last):
  File "<frozen importlib._bootstrap>", line 888, in _find_spec
AttributeError: 'AssertionRewritingHook' object has no attribute 'find_spec'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/tomer/Forks/pytest/venv/bin/py.test", line 11, in <module>
    load_entry_point('pytest', 'console_scripts', 'py.test')()
  File "/Users/tomer/Forks/pytest/src/_pytest/config/__init__.py", line 49, in main
    config = _prepareconfig(args, plugins)
  File "/Users/tomer/Forks/pytest/src/_pytest/config/__init__.py", line 186, in _prepareconfig
    pluginmanager=pluginmanager, args=args
  File "/Users/tomer/Forks/pytest/venv/lib/python3.6/site-packages/pluggy/hooks.py", line 258, in __call__
    return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
  File "/Users/tomer/Forks/pytest/venv/lib/python3.6/site-packages/pluggy/manager.py", line 67, in _hookexec
    return self._inner_hookexec(hook, methods, kwargs)
  File "/Users/tomer/Forks/pytest/venv/lib/python3.6/site-packages/pluggy/manager.py", line 61, in <lambda>
    firstresult=hook.spec_opts.get('firstresult'),
  File "/Users/tomer/Forks/pytest/venv/lib/python3.6/site-packages/pluggy/callers.py", line 196, in _multicall
    gen.send(outcome)
  File "/Users/tomer/Forks/pytest/src/_pytest/helpconfig.py", line 89, in pytest_cmdline_parse
    config = outcome.get_result()
  File "/Users/tomer/Forks/pytest/venv/lib/python3.6/site-packages/pluggy/callers.py", line 76, in get_result
    raise ex[1].with_traceback(ex[2])
  File "/Users/tomer/Forks/pytest/venv/lib/python3.6/site-packages/pluggy/callers.py", line 180, in _multicall
    res = hook_impl.function(*args)
  File "/Users/tomer/Forks/pytest/src/_pytest/config/__init__.py", line 656, in pytest_cmdline_parse
    self.parse(args)
  File "/Users/tomer/Forks/pytest/src/_pytest/config/__init__.py", line 828, in parse
    self._preparse(args, addopts=addopts)
  File "/Users/tomer/Forks/pytest/src/_pytest/config/__init__.py", line 780, in _preparse
    self.pluginmanager.load_setuptools_entrypoints("pytest11")
  File "/Users/tomer/Forks/pytest/venv/lib/python3.6/site-packages/pluggy/manager.py", line 253, in load_setuptools_entrypoints
    plugin = ep.load()
  File "/Users/tomer/Forks/pytest/venv/lib/python3.6/site-packages/pkg_resources/__init__.py", line 2332, in load
    return self.resolve()
  File "/Users/tomer/Forks/pytest/venv/lib/python3.6/site-packages/pkg_resources/__init__.py", line 2338, in resolve
    module = __import__(self.module_name, fromlist=['__name__'], level=0)
  File "<frozen importlib._bootstrap>", line 971, in _find_and_load
  File "<frozen importlib._bootstrap>", line 955, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 656, in _load_unlocked
  File "<frozen importlib._bootstrap>", line 626, in _load_backward_compatible
  File "/Users/tomer/Forks/pytest/src/_pytest/assertion/rewrite.py", line 304, in load_module
    six.exec_(co, mod.__dict__)
  File "/Users/tomer/Forks/pytest/venv/lib/python3.6/site-packages/pytest_ast_back_to_python.py", line 7, in <module>
    import codegen
  File "<frozen importlib._bootstrap>", line 971, in _find_and_load
  File "<frozen importlib._bootstrap>", line 951, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 890, in _find_spec
  File "<frozen importlib._bootstrap>", line 864, in _find_spec_legacy
  File "/Users/tomer/Forks/pytest/src/_pytest/assertion/rewrite.py", line 172, in find_module
    source_stat, co = _rewrite_test(self.config, fn_pypath)
  File "/Users/tomer/Forks/pytest/src/_pytest/assertion/rewrite.py", line 422, in _rewrite_test
    co = compile(tree, fn.strpath, "exec", dont_inherit=True)
TypeError: required field "lineno" missing from expr

Tadaboody · 2018-10-25T10:55:44Z

Should I rebase to fix the dirty history or will this be squashed anyway?

Sup3rGeo · 2018-10-25T11:06:44Z

@Tadaboody not sure if I can help you without really going deep into your code. Yes in theory you should not have to worry about lineno of the statements you are adding.

I see you got already some helpers and delegated the warning for some functions. I don't know if you did it like this, but I would start manually adding one valid ast statement at a time from the stock code + running/testing, so when things break like this you can pinpoint the code causing problems more easily.

Tadaboody · 2018-10-25T11:11:15Z

@Sup3rGeo That's more or less what I did, maybe next time I'll do it slower.
It seems to have fixed itself overnight, so it might've been my tiredness or some weird caching problem. thanks anyway!

nicoddemus · 2018-10-30T19:35:34Z

src/_pytest/assertion/rewrite.py

+warn_explicit(
+    PytestWarning('assertion the value None, Please use "assert is None"'),
+    category=None,
+    filename='{filename}',


I get this error on Windows:

File "c:\users\bruno\pytest\src\_pytest\assertion\rewrite.py", line 843, in visit_Assert top_condition, module_path=self.module_path, lineno=assert_.lineno File "c:\users\bruno\pytest\src\_pytest\assertion\rewrite.py", line 904, in warn_about_none_ast filename=str(module_path), lineno=lineno File "C:\Users\bruno\AppData\Local\Programs\Python\Python36-32\lib\ast.py", line 35, in parse return compile(source, filename, mode, PyCF_ONLY_AST) File "<unknown>", line 7 SyntaxError: (unicode error) 'unicodeescape' codec can't decode bytes in position 2-3: truncated \uXXXX escape

The problem is using str(module_path) here, because this will generate a string with \ characters and potential invalid escapes.

We should use this instead:

warn_explicit( PytestWarning('assertion the value None, please use "assert is None"'), category=None, filename={filename!r}, lineno={lineno}, ) """.format( filename=module_path, lineno=lineno

Ooh I was wondering how to fix that! Python string formatting is truly a pit with no end

Heh!

Also, I just realized that you should use filename=module_path.strpath, otherwise we will get the py.path.local representation in the code, which is not what we want.

nicoddemus · 2018-10-30T19:37:26Z

testing/test_warnings.py

+        result = testdir.runpytest()
+        self.assert_result_warns(result)
+
+    @pytest.mark.xfail(strict=True)


Instead of using xfail, please use a proper assertion that we are not receiving any warnings, something like:

result.stdout.fnmatch_lines(["*1 passed in*"])

Because warnings will generate a different summary ("1 passed, 1 warnings in"), which would fail the test. Same for test_false_function_no_warn below.

Right! I saw that later with other tests in the same module, I'll get to it

nicoddemus

We are almost there. 😁

Please take a look at my comments.

nicoddemus · 2018-10-30T19:38:45Z

Oops I also broke linting when I edited the changelog using the GH web UI, sorry about that! 🙇

Tadaboody · 2018-12-04T11:34:08Z

It looks like I broke something with test collections? I'm not quite sure what the test I broke is supposed to do

blueyed · 2018-12-05T00:46:46Z

@Tadaboody
This is #4346 - please try if rebasing fixes it (it makes sense anyway I guess). Also consider squashing commits together then if it makes sense.

As requested by review. :ok_hand: Address code review for tests

🐛Fix warn ast bugs 🐛Fix inner-ast imports by using importFrom Alternetavly ast_call_helper could be retooled to use ast.attribute(...)

Maybe there should be a warning about that too?

in py2 it's a ast.Name where in py3 it's a ast.NamedConstant Fixes namespace by using import from

Edited the changelog for extra clarity, and to fire off auto-formatting Oddly enough, keeping `filename='{filename!r}'` caused an error while collecting tests, but getting rid of the single ticks fixed it Hopefully closes #3191

Tadaboody · 2018-12-05T11:36:56Z

🎉 Passed! thanks @blueyed. I squashed a bit but looking again I can squash some even more if needed. Waiting for a re-review by @nicoddemus

nicoddemus

I think a few more things to sort out before we can merge this. 😁

nicoddemus · 2018-12-05T14:44:30Z

changelog/3191.feature.rst

+This warning will not be issued when ``None`` is explicitly checked
+    assert none_returning_fun() is None
+
+will not issue the warning


This line seems to be out of place.

Great changelog entry btw!

That's because you wrote it - 26d27df
I think I added that line to trigger the pre-commit auto format not the best plan in hindsight

I reworded this section, hopefully it's clearer. If you still think it's out of place I'll delete it

Ahh forgot about that, well at least I'm consistent then. It would be amusing if I looked at it and request a lot of changes. 😆

Looks good, thanks!

nicoddemus · 2018-12-05T14:45:35Z

src/_pytest/assertion/rewrite.py

+            warnings.warn_explicit(
+                PytestWarning('assertion the value None, Please use "assert is None"'),
+                category=None,
+                # filename=str(self.module_path),


yup. should I keep the docstring at all?

A good point, let's change the docstring to a short description then, it will probably be better. 👍

nicoddemus · 2018-12-05T14:48:31Z

src/_pytest/assertion/rewrite.py

+    lineno={lineno},
+)
+            """.format(
+                filename=str(module_path), lineno=lineno


We need to use filename=module_path.strpath here instead: on Python 2 and unicode filenames the str() call will blow up.

nicoddemus

Awesome work @Tadaboody, thanks!

Tadaboody · 2018-12-05T16:38:45Z

now I remember why I didn't keep module_path.strpath! it's not always included
I don't think these cases are relevant for emitting warnings, should I just not inject the warning when the assertion isn't from a module? (aka when module_path is None)

nicoddemus · 2018-12-05T16:46:44Z

Yeah I think it's reasonable to skip those warnings when module_path is None 👍

blueyed · 2018-12-05T18:23:40Z

Great stuff.
I've pushed two fixup commits.

blueyed · 2018-12-05T18:25:14Z

If this turns out to be green and my changes are OK, please squash this into a single commit, and force-push (we could squash-merge it but that is disabled).

Tadaboody changed the title ~~[#3191] Set up tests to confirm warnings~~ WIP:[#3191] Set up tests to confirm warnings Oct 14, 2018

Tadaboody changed the title ~~WIP:[#3191] Set up tests to confirm warnings~~ WIP:[#3191] Give hints when an assertion value is None instead of a boolean Oct 14, 2018

nicoddemus reviewed Oct 14, 2018

View reviewed changes

nicoddemus mentioned this pull request Oct 14, 2018

Add a simple example on how to use pytester to the CONTRIBUTING guide #4151

Closed

nicoddemus reviewed Oct 15, 2018

View reviewed changes

Tadaboody changed the title ~~WIP:[#3191] Give hints when an assertion value is None instead of a boolean~~ [#3191] Give hints when an assertion value is None instead of a boolean Oct 25, 2018

nicoddemus reviewed Oct 30, 2018

View reviewed changes

nicoddemus requested changes Oct 30, 2018

View reviewed changes

Tadaboody and others added 8 commits December 5, 2018 10:41

[#3191] Set up tests to confirm warnings

1654b77

Fix tests and add aditional cases

9fc9b29

As requested by review. :ok_hand: Address code review for tests

Update changelog

aaf7f7f

Check for 'assert None' and warn appropriately

59a11b6

🐛Fix warn ast bugs 🐛Fix inner-ast imports by using importFrom Alternetavly ast_call_helper could be retooled to use ast.attribute(...)

Fix tests not to assert a function that already asserts

e0c2ab1

Maybe there should be a warning about that too?

Simplify warning creation by using ast.parse

3e6f1fa

in py2 it's a ast.Name where in py3 it's a ast.NamedConstant Fixes namespace by using import from

Improve changelog message

26d27df

👌 Address code review

e1e4b22

Edited the changelog for extra clarity, and to fire off auto-formatting Oddly enough, keeping `filename='{filename!r}'` caused an error while collecting tests, but getting rid of the single ticks fixed it Hopefully closes #3191

nicoddemus requested changes Dec 5, 2018

View reviewed changes

Tadaboody added 2 commits December 5, 2018 17:18

Address code review

41031fc

Shorten docstring for warn_about_none_ast

7a7ad0c

nicoddemus approved these changes Dec 5, 2018

View reviewed changes

Tadaboody and others added 3 commits December 5, 2018 19:49

Don't insert warnings when not in a module

8fd6048

Revisit changelog entry [ci skip]

8a0ed7e

Harden tests, fix doc/msg

5ebacc4

nicoddemus merged commit 76884c7 into pytest-dev:features Dec 5, 2018

oscarbenjamin mentioned this pull request Jan 20, 2019

Warning about assert None from XFAIL tests #4639

Closed

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

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

Conversation

Tadaboody commented Oct 14, 2018 • edited by Zac-HD Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 15, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Tadaboody commented Oct 22, 2018

nicoddemus commented Oct 23, 2018

Sup3rGeo commented Oct 23, 2018

Tadaboody commented Oct 24, 2018 • edited Loading

Edit: This got fixed itself after I woke up. Is this what people call a bed bug?

Tadaboody commented Oct 25, 2018

Sup3rGeo commented Oct 25, 2018

Tadaboody commented Oct 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus left a comment

Choose a reason for hiding this comment

nicoddemus commented Oct 30, 2018

Tadaboody commented Dec 4, 2018

blueyed commented Dec 5, 2018

Tadaboody commented Dec 5, 2018

nicoddemus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tadaboody Dec 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus left a comment

Choose a reason for hiding this comment

Tadaboody commented Dec 5, 2018

nicoddemus commented Dec 5, 2018

blueyed commented Dec 5, 2018

blueyed commented Dec 5, 2018 • edited Loading

Tadaboody commented Oct 14, 2018 •

edited by Zac-HD

Loading

codecov bot commented Oct 15, 2018 •

edited

Loading

Tadaboody commented Oct 24, 2018 •

edited

Loading

Tadaboody Dec 5, 2018 •

edited

Loading

blueyed commented Dec 5, 2018 •

edited

Loading