Add error for over-complex Act block
jamescooke opened this issue ยท 5 comments
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.
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.
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?)
@ROpdebee Thanks for pointing out that extraction is more elegant ๐ - I completely agree ๐
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.
Reassess this after #146 is done. ๐
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.