jamescooke/flake8-aaa

Act block with context not recognised correctly

k0nG opened this issue · 6 comments

k0nG commented

Describe the bug
When I've got a with statement providing context as part of an act block and I have a arrange block above I get flake8-aaa errors when I don't think I should.

I encountered this when upgrading from flake8-aaa 0.11.1 to 0.12.2

Share debugging output

With # act comment:

------+------------------------------------------------------------------------
24 DEF|def test_with():
25 ARR|    a_class = AClass()
26 BL |
       ^ AAA05 blank line in block
27 ARR|    with freeze_time("2021-02-02 12:00:02"):  # act
28 ACT|        result = a_class.action('test')
               ^ AAA03 expected 1 blank line before Act block, found none
29 BL |
30 ASS|    assert result == 'test'
------+------------------------------------------------------------------------
    2 | ERRORS
======+========================================================================
        FAILED with 2 ERRORS

without # act comment:

------+------------------------------------------------------------------------
24 DEF|def test_with():
25 ARR|    a_class = AClass()
26 BL |
       ^ AAA05 blank line in block
27 ARR|    with freeze_time("2021-02-02 12:00:02"):
28 ACT|        result = a_class.action('test')
               ^ AAA03 expected 1 blank line before Act block, found none
29 BL |
30 ASS|    assert result == 'test'
------+------------------------------------------------------------------------
    2 | ERRORS
======+========================================================================
        FAILED with 2 ERRORS

Flake8:

4.0.1 (aaa: 0.12.2, mccabe: 0.6.1, pycodestyle: 2.8.0, pyflakes: 2.4.0) CPython 3.8.5 on Darwin

Please share the Python version and platform you are using:

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

Expected behavior
Expect to distinguish an act block with a with statement wrapping the test action. This seems to happen for me with with freeze_time(""): but it's ok when using with pytest.raises

Additional context
Using:
freezegun = "1.2.1"

Source code:

from freezegun import freeze_time


class AClass():

    def action(a):
        return a


def test_with():
    a_class = AClass()

    with freeze_time("2021-02-02 12:00:02"): 
        result = a_class.action('test')

    assert result == 'test'

Thanks for raising this - it probably should be better documented in the "rules and error codes" which will be handled by #191

This behaviour was changed in #146 < which contains further examples.

TL;DR the context manager starting with the with clause is considered arrangement by Flake8-AAA. In this case, with freeze_time() is adjusting underlying Python time-related return values, so it's arranging for the test. The debug output is hinting at that when it shows you ARR on the line here:

27 ARR|    with freeze_time("2021-02-02 12:00:02"):

Flake8-AAA wants all the ARR lines together, then a blank line, then the ACT lines, so reformatting your test would give:

def test_with():
    a_class = AClass()
    with freeze_time("2021-02-02 12:00:02"): 

        result = a_class.action('test')

    assert result == 'test'

Does that formatting work in your project?

k0nG commented

Thanks for the really quick reply.

Yes, that does indeed work. Sorry, I did try something like that after reading #146 , but for some reason it didn't work.

I've updated my tests now and they are all passing in flake8-aaa .

Thanks!

As of Black 23.1.0 (and possibly earlier versions), the solution of separating the context manager into the Arrange block and keeping the Act block simple won't work because Black now complains about the blank line after the context manager:

def test(cli_runner, audit_path: pathlib.Path) -> None:
     with mock.patch("validate.__main__.download", new=fake_download(audit_path)):
-
         result = cli_runner.invoke(main, ["all", "201901", "-v"])
 
     assert result.exit_code == 0

Black wants to remove the blank line.

I'm not going to reopen this, but will create a new ticket.

Paper trail:

Hi @jamescooke thank you for handling the black issue (it's driving me crazy) can you put a link here to the newly created issue once it's done so we can follow it?

Thanks

New issue for Black and context managers is here: #200