Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Act block with context not recognised correctly #192

Closed
k0nG opened this issue Mar 31, 2022 · 6 comments
Closed

Act block with context not recognised correctly #192

k0nG opened this issue Mar 31, 2022 · 6 comments

Comments

@k0nG
Copy link

k0nG commented Mar 31, 2022

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'
@jamescooke
Copy link
Owner

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
Copy link
Author

k0nG commented Mar 31, 2022

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!

@k0nG k0nG closed this as completed Mar 31, 2022
@jamescooke
Copy link
Owner

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.

@lyz-code
Copy link

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

@jamescooke
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants