jamescooke/flake8-aaa

Act blocks with type hints raise false positive

dataframing opened this issue · 6 comments

dataframing commented

Describe the bug
Hi there. I've been using flake8-aaa for a few projects now, and it's proven super useful! Thank you for your hard work.

I noticed, however, that the AAA01 no Act block found in test rule raises a false positive when you type-hint the result assignment. For example:

def add_one(x: int) -> int:
    return x + 1


class DummyTest:
    def test_add_one(self) -> None:
        # Arrange.
        data: int = 0

        # Act.
        result: int = add_one(data)

        # Assert.
        assert result != data
        assert result == data + 1

will cause flake8-aaa to claim the aforementioned rule. However, if you remove the int type hint on result, it passes (as expected). I can also add the # act pragma to the line and keep the type hint.

Share debugging output

------+------------------------------------------------------------------------
15 DEF|    def test_add_one(self) -> None:
           ^ AAA01 no Act block found in test
16 ???|        # Arrange.
17 ???|        data: int = 0
18 BL |
19 ???|        # Act.
20 ???|        result: int = add_one(data)
21 BL |
22 ???|        # Assert.
23 ???|        assert result != data
24 ???|        assert result == data + 1
------+------------------------------------------------------------------------
    1 | ERROR
======+========================================================================
        FAILED with 1 ERROR

If you are running via Flake8, please send Flake8's version signature - that's the output of flake8 --version:

3.7.9 (aaa: 0.7.1, mccabe: 0.6.1, pycodestyle: 2.5.0, pyflakes: 2.1.1) CPython 3.7.6 on Darwin

Please share the Python version and platform you are using:

  • Python version (output of python --version): Python 3.7.6
  • Platform (GNU Linux / Mac OSX / Windows): MacOS

Expected behavior
I figured that adding type hints to any section – arrange, act, and/or assert – would not flip the AAA linter.

Additional context

  • Please share your test file if possible. See above
  • Please share the content of your Python environment (the output of pip freeze) N/A.
jamescooke commented

Thanks for opening this Danny! 🙌

I figured that adding type hints to any section – arrange, act, and/or assert – would not flip the AAA linter.

I agree 👍

I've done some quick copypasta and got failing test examples for py36, py37 and py38:

The reason this happens is because the check that looks for a result = result assignment Act block is just a very simple startswith('result =') - this could definitely be improved. My memory of writing the startswith() check is that it was "good enough" for now and would need revisiting - thanks for triggering that.

My guess is that this check should probably be improved to use the line's tokens to check for an assignment to a variable called "result".

dataframing commented

Of course! Let me know if you need any help; happy to contribute. 👍

jamescooke commented

@dataframing I know this is off-topic of this bug, but it's hard to have conversation on GitHub outside of Issues...

How was your experience filling in the Issue template? Were there any bits that could be improved? If you have any detailed changes / improvements, then a PR would be super. Alternatively, you could drop some comments on the commit that created it: 8931388

I'd previously written this issue with the plan that false positives and false negatives should have separate templates: #99 - but I'm not sure if that's really required.

dataframing commented

I thought it was fairly straightforward. I didn't bother with running pip freeze and sharing the file since I gave a minimally reproducible example. Maybe that would be an addition to the issue?

I know the R community has reprex for sharing minimally reproducible examples.

  • 👍1
jamescooke commented

@dataframing There's a fix on master for this bug - could you try it out before I cut a release?

Something like the following should work - first make sure that the old version fails:

$ pip install flake8 flake8-aaa
$ cat > test_x.py
def test():
    result: int = 1

    assert result == 1
^d
$ flake8 test_x.py
test_x.py:1:1: AAA01 no Act block found in test

Now update with what's on master:

$ git clone git@github.com:jamescooke/flake8-aaa
$ pip install -e flake8-aaa
$ flake8 test_x.py

... and all should "Just work ™️ "

If you're happy, then close this up and I'll release v0.7.2 🤞

jamescooke commented

v0.7.2 is now released (see CHANGELOG https://github.com/jamescooke/flake8-aaa/blob/master/CHANGELOG.rst#072---20200224) which includes this fix.

I'm going to close this, but please reopen if something's not working.