Add error for over-complex Act block

[jamescooke]

An Act block can be overly complex when parent nodes of the Act node contain "extra" lines.

In a simple example, an Act node with a context manager ('pytest_raises' or 'unittest_raises' types) can wrap other nodes "below" it.

def test()
    with pytest.raises(ValueError):  # < Act node, Act block first line
        do_thing()
        do_other_thing()  # < Act block last line

In this case do_other_thing() can be "smuggled" into the Act block because it's wrapped in the pytest.raises() context manager.

A more complex example, extra statements can be wrapped by context managers which are found that are parents of the Act node:

def test():
    with mock.patch('thing.thinger') as mock_thinger:  # < Act block first line
        with mock.patch('other_thing.thinger'):
            with pytest.raises(ValueError):  # < Act node
                do_thing()

            do_other_thing2()
        do_other_thing3()  # < Act block last line

    assert mock_thinger.call_count == 1

In this case, do_other_thing2() and do_other_thing3() are wrapped by the mock.patch() context managers and are then "smuggled" into the Act block.

Requirements

• Add non-blocking rule for over-complex act block. Point error raised at the additional lines. Resolution is to move the additional lines outside of any act block, maybe to extract them to a fixture or setUp() function.

• Checking of Act block should be wired in to happen after Act block is linted.

• Add documentation and tests.

[jamescooke]

Extracted this test case from Block.build_assert():

with mock.patch(thing):
    with pytest.raises(ValueError):
        do_thing()
    print('hi')

Assert that print('hi') is correctly grabbed by the Act block, but that this raises the new "over complex Act block" error.

[ROpdebee]

My two cents in regards to the example with the context managers: I think extracting the mocking into a fixture would be more elegant:

@pytest.fixture()
def patched_thing_thinger():
    with mock.patch('thing.thinger') as mock_thinger:
        yield mock_thinger

@pytest.fixture()
def patched_other_thing_thinger():
    with mock.patch('other_thing.thinger') as mock_thinger:
        yield mock_thinger

def test(patched_thing_thinger, patched_other_thing_thinger):
    with pytest.raises(ValueError):
        do_thing()
    do_other_thing2()  # < shouldn't be in Act
    do_other_thing3()  # < shouldn't be in Act

    assert patched_thing_thinger.call_count == 1

Applicability may vary though (is do_other_thing3() okay to be invoked when other_thing.thinger is patched?)

[jamescooke]

@ROpdebee Thanks for pointing out that extraction is more elegant :+1: - I completely agree :blush:

Sometimes I find it hard to write failing examples that are bad enough and real world enough both in Issues and in the https://github.com/jamescooke/flake8-aaa/tree/master/examples/bad dir... Which is why you see function names like do_thing() happening a bit too much.

[jamescooke]

Reassess this after #146 is done. :eyes:

[jamescooke]

Extra example that just came up at work - a false positive, where the assert was being skipped:

def test_assert_ddex_equal_failed(
    audit_test_class, input1, input2, err_msg
) -> None:
    with pytest.raises(AssertionError) as excinfo:
        audit_test_class.assertDDEXEqual(input1, input2)
        assert err_msg in str(excinfo.value)

assert is inside the pytest.raises() and so it not hit when an exception happens.